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

Wip/translation api #211

Merged
merged 7 commits into from
Oct 12, 2021
Merged

Wip/translation api #211

merged 7 commits into from
Oct 12, 2021

Conversation

Mehrad0711
Copy link
Member

@Mehrad0711 Mehrad0711 assigned Mehrad0711 and unassigned Mehrad0711 Oct 5, 2021
@Mehrad0711 Mehrad0711 marked this pull request as draft October 5, 2021 21:43
@Mehrad0711 Mehrad0711 force-pushed the wip/translation-api branch from 98a4959 to 3402f2a Compare October 6, 2021 07:31
Copy link
Contributor

@gcampax gcampax left a comment

Choose a reason for hiding this comment

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

This looks good I think.

genienlp/server.py Show resolved Hide resolved
@Mehrad0711 Mehrad0711 force-pushed the wip/translation-api branch from 3402f2a to a4212d9 Compare October 6, 2021 23:23
@Mehrad0711 Mehrad0711 force-pushed the wip/translation-api branch from a4212d9 to 6f61f48 Compare October 7, 2021 00:33
instead change ids on the go to be unique, lifting the burden from user or dataset
@Mehrad0711 Mehrad0711 force-pushed the wip/translation-api branch from 6f61f48 to 122928f Compare October 8, 2021 03:12
setup.py Outdated
@@ -64,7 +64,7 @@
'mosestokenizer~=1.1',
'pathos==0.2.8',
# for kf:
'kfserving>=0.5.0',
'kfserving==0.5.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do this. kfserving depends on ray, but doesn't specify a version bound on it. ray has broken APIs before, breaking kfserving, and us transitively. If we pin kfserving, we remain broken even if kfserving releases a fixed version, until we change genienlp. This creates a cascade effect and takes a lot of work to fix.
(yes python packaging is the worst)

Copy link
Member Author

Choose a reason for hiding this comment

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

what is the solution then? pinning "ray" instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no good solution, but at least if we leave kfserving unpinned we'll get any fix they make as soon as they make it.

Copy link
Member Author

Choose a reason for hiding this comment

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

but that will leave our tests broken till then. Apparently ray 1.7.0 was released 3 days ago and had major changes. I suggest pinning ray to 1.6.0 until they patch 1.7.0. what do you think?

Recent release of ray (1.7.0) is breaking our tests due to some import errors.
Will revert this change once a new patch is pushed.
@Mehrad0711
Copy link
Member Author

Ok @gcampax, I pinned ray for now until they patch it. I'm gonna merge this branch and rebase the other one.

@Mehrad0711 Mehrad0711 marked this pull request as ready for review October 12, 2021 02:56
@Mehrad0711 Mehrad0711 merged commit 513b409 into master Oct 12, 2021
@Mehrad0711 Mehrad0711 deleted the wip/translation-api branch November 1, 2021 18:57
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.

2 participants