-
Notifications
You must be signed in to change notification settings - Fork 181
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 Japanese language dependencies #1511
base: master
Are you sure you want to change the base?
Conversation
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.
great work! this is really giving my team trouble. would love to see this merged.
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.
Many thanks for your contribution, I left some comments. I suggest that these extra libraries are optional as they are not required for all cases. Also I left some suggestions to tidy up the tests.
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
@sakoush I believe I have addressed your comments. Let me know if I have misunderstood something! |
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.
Thanks for acting on the suggestions, I left a minor suggestion to parametrise the test case.
Also could you please update the huggingface runtime docs with to explain how to use your addition?
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
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.
Thanks for the contribution so far, I left a few minor suggestions.
Apologies for the slow response due to the holiday season.
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
runtimes/huggingface/tests/test_tasks/test_japanese_mask_fill.py
Outdated
Show resolved
Hide resolved
@sakoush Hey! Hope your holidays went well. Just bumping this to your attention because I think it's ready if not close to ready to merge. Let me know if I misunderstood some of your comments. Looking forward to your feedback. |
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.
Many thanks for the contribution, I left a minor doc change.
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.
lgtm
Hey @sakoush , the pipeline is failing on TF v2.14 dropping support for Python 3.8. Would you rather drop support for Python 3.8 for this runtime or have me deprecate TF to v2.13? |
…ldonIO#1692) Bumps [joblib](https://github.com/joblib/joblib) from 1.3.2 to 1.4.0. - [Release notes](https://github.com/joblib/joblib/releases) - [Changelog](https://github.com/joblib/joblib/blob/main/CHANGES.rst) - [Commits](joblib/joblib@1.3.2...1.4.0) --- updated-dependencies: - dependency-name: joblib dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…eldonIO#1695) Bumps [pandas](https://github.com/pandas-dev/pandas) from 2.2.1 to 2.2.2. - [Release notes](https://github.com/pandas-dev/pandas/releases) - [Commits](pandas-dev/pandas@v2.2.1...v2.2.2) --- updated-dependencies: - dependency-name: pandas dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@jbauer2718 it looks like that merge conflicts have not been resolved correctly, at least by looking at the changes in |
@sakoush Sorry for missing that. Should be good now. |
Resolves #1506