Skip to content

Make word_align_corpus train lazily - #344

Merged
ddaspit merged 1 commit into
mainfrom
lazy-word-align-corpus
Aug 12, 2026
Merged

Make word_align_corpus train lazily#344
ddaspit merged 1 commit into
mainfrom
lazy-word-align-corpus

Conversation

@ddaspit

@ddaspit ddaspit commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • Properly cleans up the created model

This change is Reviewable

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

This isn't used anywhere in silnlp, I don't believe. Is there anywhere else we should check / anyone else we should inform about this change in behavior?

@Enkidu93 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Transductive support was just added. I don't think anyone is using this function yet.

@ddaspit made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm probably missing something: Now when you call word_align_corpus(), the model will potentially be trained every time you call get_rows() on the returned corpus rather than once, right? Won't that affect callers who are using word_align_corpus() already?

@Enkidu93 made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct. It is a change in behavior, but it wasn't really "correct" before, because there was no way to clean up the model. I don't think any callers would be calling it multiple times anyway.

@ddaspit made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK, right. I confirmed it isn't used like this in silnlp - just wanted to make sure there wasn't anything else we should check. But yes, a caller really shouldn't be calling get_rows() repeatedly anyways.

@Enkidu93 made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

- Properly cleans up the created model
@pmachapman
pmachapman force-pushed the lazy-word-align-corpus branch from de98a38 to 068b32e Compare August 12, 2026 19:28
@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.90164% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.01%. Comparing base (deb112b) to head (068b32e).

Files with missing lines Patch % Lines
machine/translation/corpus_ops.py 89.79% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   91.94%   92.01%   +0.07%     
==========================================
  Files         387      387              
  Lines       24381    24458      +77     
==========================================
+ Hits        22416    22505      +89     
+ Misses       1965     1953      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

I don't think any callers would be calling it multiple times anyway.

This should be true for the dotnet implementation, as it is IEnumerable, and dotnet gives you a code analysis warning if you iterate over an IEnumerable multiple times. (I'm not aware of a similar feature in Python.)

@pmachapman reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on ddaspit).

@ddaspit
ddaspit merged commit 2d9f122 into main Aug 12, 2026
17 checks passed
@ddaspit
ddaspit deleted the lazy-word-align-corpus branch August 12, 2026 21:02
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.

4 participants