Use new machine eflomal implementation - #336
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 92.02% 92.10% +0.07%
==========================================
Files 389 389
Lines 24485 24559 +74
==========================================
+ Hits 22532 22619 +87
+ Misses 1953 1940 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit made 2 comments.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on Enkidu93).
machine/jobs/nmt_engine_build_job.py line 182 at r1 (raw file):
parallel_training_corpus.get_rows(), ) ) # TODO .lowercase()?
We should perform Unicode and casing normalization.
machine/jobs/nmt_engine_build_job.py line 196 at r1 (raw file):
parallel_pretranslation_rows = list(parallel_pretranslation_corpus) # TODO .lowercase()? alignments = alignment_model.align_batch(parallel_pretranslation_rows)
This will work, but it would be better to use the transductive API, i.e. get_training_alignment. align_batch will be more expensive.
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 made 2 comments.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on ddaspit).
machine/jobs/nmt_engine_build_job.py line 182 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
We should perform Unicode and casing normalization.
OK, thank you 👍, but when we actually go to populate the source and target tokens in the pretranslation data structure, we will want them to be exactly as they were in the original text, right? Otherwise, the marker placement won't work properly. Or should we update the code in the marker placement handler to only look for normalized substrings?
machine/jobs/nmt_engine_build_job.py line 196 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
This will work, but it would be better to use the transductive API, i.e.
get_training_alignment.align_batchwill be more expensive.
Would you like me only allow transductive models or just use the transductive API if the model supports it and otherwise default to this?
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 14 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93).
machine/jobs/nmt_engine_build_job.py line 182 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
OK, thank you 👍, but when we actually go to populate the source and target tokens in the pretranslation data structure, we will want them to be exactly as they were in the original text, right? Otherwise, the marker placement won't work properly. Or should we update the code in the marker placement handler to only look for normalized substrings?
You are correct. I added a commit that inferences on the normalized tokens but returns the unnormalized tokens. It also added transductive API support, a full streaming pipeline, and fixed a bug in flatten.
machine/jobs/nmt_engine_build_job.py line 196 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Would you like me only allow transductive models or just use the transductive API if the model supports it and otherwise default to this?
I added support for the transductive API.
machine/jobs/word_alignment_build_job.py line 86 at r2 (raw file):
return train_corpus_size def _batch_inference(
This should be updated to work similar to NmtEngineBuildJob._align.
68cff34 to
9cdec1a
Compare
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 partially reviewed 1 file and made 3 comments.
Reviewable status: 11 of 14 files reviewed, 1 unresolved discussion (waiting on ddaspit).
machine/jobs/nmt_engine_build_job.py line 182 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You are correct. I added a commit that inferences on the normalized tokens but returns the unnormalized tokens. It also added transductive API support, a full streaming pipeline, and fixed a bug in
flatten.
OK, great 👍
machine/jobs/nmt_engine_build_job.py line 196 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I added support for the transductive API.
Great, thank you
machine/jobs/word_alignment_build_job.py line 86 at r2 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
This should be updated to work similar to
NmtEngineBuildJob._align.
Done. Of course, the word alignment API isn't really set up yet to take advantage of the transductive model. For now, I've just had it use align_batch, but maybe we should make an issue to update how training/inferencing work for word alignment in general to take advantage of this. Even if we allow for training and inferencing on the same data (which we probably just should for word alignment in general?), we'd still need a way to index the training data so we can retrieve the alignments.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 4 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93).
machine/jobs/word_alignment_build_job.py line 97 at r4 (raw file):
) -> None: inference_inputs = self._word_alignment_file_service.get_word_alignment_inputs()
In order to make it worthwhile to spool the data to disk, get_word_alignment_inputs should return a generator instead of a list.
machine/jobs/word_alignment_build_job.py line 135 at r4 (raw file):
check_canceled() segments = [(lowercase(row.source_segment), lowercase(row.target_segment)) for _, row in wa_batch] alignments = alignment_model.align_batch(segments)
Progress is never advanced while inferencing.
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 14 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit).
machine/jobs/word_alignment_build_job.py line 97 at r4 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
In order to make it worthwhile to spool the data to disk,
get_word_alignment_inputsshould return a generator instead of a list.
Done.
machine/jobs/word_alignment_build_job.py line 135 at r4 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Progress is never advanced while inferencing.
Done.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
|
Decided to test this E2E - and I'm glad I did because I hit an error that I wasn't hitting before: Not my top priority right now but just so you know why I haven't merged this yet. |
|
This looks to be caused by having empty pretranslations. Peter is going to take a look on the thot side since he's already making updates to that code. |
- full streaming pipeline for inferencing - fall back to inductive API - fix text id handling in "flatten"
98b9e6f to
ab22a84
Compare
Enkidu93
left a comment
There was a problem hiding this comment.
Alright, the slow-down was from progress-reporting. I've reduced the reporting significantly and now the whole alignment phase takes a few minutes rather than twenty. We could reduce it further or eliminate it altogether if you prefer. I've also updated thot to the new version that fixes the error above. @ddaspit, I think this is ready for re-review.
@Enkidu93 made 1 comment.
Reviewable status: 14 of 18 files reviewed, all discussions resolved (waiting on ddaspit).
ddaspit
left a comment
There was a problem hiding this comment.
Good job tracking this down. What kind of an impact on speed does it have if we remove progress reporting during alignment inferencing altogether?
@ddaspit reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93).
machine/translation/thot/thot_word_alignment_model_trainer.py line 195 at r12 (raw file):
) cur_step = 0 update_frequency = num_steps // 10 if num_steps and num_steps > 10 else None
I remember us running into a similar problem a while ago. We do progress throttling in the get_clearml_progress_caller function. Was that not doing a good enough job? Should that function be updated instead of this? At the very least, we should add a comment explaining the issue.
Enkidu93
left a comment
There was a problem hiding this comment.
It's about comparable if we remove it altogether actually - I just confirmed. It's still taking 2-5 minutes.
@Enkidu93 made 2 comments.
Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on ddaspit).
machine/translation/thot/thot_word_alignment_model_trainer.py line 195 at r12 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I remember us running into a similar problem a while ago. We do progress throttling in the
get_clearml_progress_callerfunction. Was that not doing a good enough job? Should that function be updated instead of this? At the very least, we should add a comment explaining the issue.
Yes, the throttling there doesn't seem to do much. The iteration reporting in ClearML is very slow. We are already throttling NMT training in the trainer similarly: #227. If you'd prefer, we could try to have a uniform way of throttling them in the cleaml progress function. We could also pass the update_frequency to the constructor if you prefer. Part of the reason unifying them and throttling them in one place might be difficult is that you may genuinely want the frequency to differ task to task.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 1 file and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
machine/translation/thot/thot_word_alignment_model_trainer.py line 195 at r12 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Yes, the throttling there doesn't seem to do much. The iteration reporting in ClearML is very slow. We are already throttling NMT training in the trainer similarly: #227. If you'd prefer, we could try to have a uniform way of throttling them in the cleaml progress function. We could also pass the
update_frequencyto the constructor if you prefer. Part of the reason unifying them and throttling them in one place might be difficult is that you may genuinely want the frequency to differ task to task.
I'm good with tailoring the throttling for each task.
Fixes #320, fixes sillsdev/serval#948
Also:
This update is a little difficult to test. I have tested it E2E and the Serval E2E tests do pass. The scores are populated properly. I compared the alignments with the previous version for the E2E test data and they seem mostly similar - maybe the old ones are a little better 😬 - but it's difficult to judge. I'm going to trust that, as long as I am using the API properly, switching to the new implementation and training the aligner on the training data is an improvement. My outstanding questions are:
lowercase()- see my TODO comments.This change is