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

Upgrade eflomal #433

Merged
merged 6 commits into from
Jul 8, 2024
Merged

Upgrade eflomal #433

merged 6 commits into from
Jul 8, 2024

Conversation

mshannon-sil
Copy link
Collaborator

@mshannon-sil mshannon-sil commented Jul 2, 2024

This PR updates eflomal from 0.1 to 2.0.0 to fix #431, modifies the Dockerfile to use the pip installation of eflomal, and adds eflomal to the ALIGNERS dictionary in silnlp/alignment/config.py permanently instead of only when eflomal is installed so that the error message will be eflomal is not installed rather than invalid aligner.


This change is Reviewable

@mshannon-sil mshannon-sil added the bug Something isn't working label Jul 2, 2024
@mshannon-sil mshannon-sil self-assigned this Jul 2, 2024
Copy link
Collaborator

@isaac091 isaac091 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Can you tag the last commit so that a new image is built and published?

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)

@mshannon-sil
Copy link
Collaborator Author

I removed the change to add eflomal to the ALIGNERS dictionary by default, since that's causing experiments to fail when eflomal is not installed, and eflomal is still just an optional requirement.

@mshannon-sil
Copy link
Collaborator Author

Merged in Isaac's commit which allows for the updated error message without crashing tasks that don't need eflomal.

Copy link
Collaborator

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 4 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @isaac091)

Copy link
Collaborator

@isaac091 isaac091 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)

@mshannon-sil mshannon-sil merged commit eef5c6b into master Jul 8, 2024
1 check passed
@mshannon-sil mshannon-sil deleted the upgrade_eflomal branch July 8, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update eflomal to 2.0.0
3 participants