Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CV2-5122 add paraphrase multilingual #109

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

DGaffney
Copy link
Collaborator

Description

Adds Paraphrase-multilingual model to Presto

Reference: CV2-5122

How has this been tested?

Not yet tested locally, should Just Work

Are there any external dependencies?

None

Have you considered secure coding practices when writing this code?

None Applicable

Copy link
Contributor

@skyemeedan skyemeedan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea text model!

looks like the filenames for indian sbert vs multilingual got swapped

I think it is time to add integration tests (in addition to the mocked unittests) that actually call the models for real to know if the setup is working.

test/lib/model/test_indian_sbert.py Outdated Show resolved Hide resolved
test/lib/model/test_indian_sbert.py Outdated Show resolved Hide resolved
@DGaffney DGaffney requested a review from skyemeedan August 26, 2024 14:16
Copy link
Contributor

@skyemeedan skyemeedan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is time to add basic integration tests (in addition to the mocked unittests) that call the models for real to know if the setup is working

@DGaffney
Copy link
Collaborator Author

I think it is time to add basic integration tests (in addition to the mocked unittests) that call the models for real to know if the setup is working

I disagree - @computermacgyver please make a call on this.

@computermacgyver
Copy link
Collaborator

Presto in general will benefit from integration tests, and they serve as great documentation. I believe, however, that we can add those tests under a separate PR/ticket rather than this one. I'd like an approach that applies to all of our Presto models: hence a separate ticket that can be more holistic than text alone.

@DGaffney DGaffney requested a review from skyemeedan August 26, 2024 16:06
@DGaffney DGaffney merged commit aedfe2a into master Aug 26, 2024
2 checks passed
@DGaffney DGaffney deleted the cv2-5122-add-paraphrase-multilingual branch August 26, 2024 16:07
@skyemeedan
Copy link
Contributor

created https://meedan.atlassian.net/browse/CV2-5132 for presto integration tests

@DGaffney
Copy link
Collaborator Author

created https://meedan.atlassian.net/browse/CV2-5132 for presto integration tests

Perfect - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants