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

Improved huggingface batch logic #1336

Merged

Conversation

ajsalow
Copy link
Contributor

@ajsalow ajsalow commented Aug 15, 2023

Addresses issues described in #1249 where batch size greater than 0 causes some hugging face models to fail loading.

@ajsalow ajsalow marked this pull request as ready for review August 15, 2023 19:26
Copy link
Contributor

@adriangonz adriangonz left a comment

Choose a reason for hiding this comment

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

Changes look good! Thanks for taking the time to contribute this patch @ajsalow.

Before merging, could you add a test covering the new behaviour? Besides that, there also seem to be some conflicts with latest master.

@ajsalow
Copy link
Contributor Author

ajsalow commented Aug 27, 2023

Thanks for taking a look @adriangonz. I've added some test coverage, let me know if anything else is needed.

Copy link
Contributor

@adriangonz adriangonz left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for adding that test @ajsalow 👍

Once tests are green, we should be able to merge this one.

@adriangonz
Copy link
Contributor

Hey @ajsalow ,

Just had a look at the test failures. I think from those, the lint failure is probably the key one - everything else seems to be just flaky. So once you get a chance to address the linter error, we should be good to merge this one:

./runtimes/huggingface/mlserver_huggingface/common.py:72:89: E501 line too long (120 > 88 characters)

@ajsalow
Copy link
Contributor Author

ajsalow commented Sep 5, 2023

@adriangonz apologies, I incorrectly assumed linting was using pylint. make lint is now passing for me locally.

@adriangonz adriangonz merged commit 93b2bcb into SeldonIO:master Sep 5, 2023
27 checks passed
adriangonz pushed a commit that referenced this pull request Sep 7, 2023
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.

None yet

2 participants