-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add MT tutorial #129
Add MT tutorial #129
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
- Coverage 88.12% 88.07% -0.05%
==========================================
Files 273 273
Lines 16034 16039 +5
==========================================
- Hits 14130 14127 -3
- Misses 1904 1912 +8 ☔ View full report in Codecov by Sentry. |
why are some trg and some tgt? I thought we were only doing trg... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @johnml1135)
machine/jobs/huggingface/hugging_face_nmt_model_factory.py
line 80 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
why are some trg and some tgt? I thought we were only doing trg...
I was trying to make all of the parameter names consistent for the Huggingface classes, but I didn't want to break the integration with Serval, so I left trg_lang
alone. I shouldn't have changed add_unk_trg_tokens
in the settings.yaml
, since that is potentially a breaking change with Serval and also makes it inconsistent with the trg_lang
setting, so I changed it back. Unfortunately, that means that the jobs settings and the Huggingface classes are now inconsistent, but I willing to live with that, so we don't break Serval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)
This change is