Skip to content

Improved huggingface batch logic#1336

Merged
adriangonz merged 8 commits into
SeldonIO:masterfrom
ajsalow:improved_huggingface_batch_logic
Sep 5, 2023
Merged

Improved huggingface batch logic#1336
adriangonz merged 8 commits into
SeldonIO:masterfrom
ajsalow:improved_huggingface_batch_logic

Conversation

@ajsalow

@ajsalow ajsalow commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

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

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

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

ajsalow commented Aug 27, 2023

Copy link
Copy Markdown
Contributor Author

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

@ajsalow ajsalow requested a review from adriangonz August 27, 2023 22:21

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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 ajsalow requested a review from adriangonz September 3, 2023 15:44
@ajsalow

ajsalow commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

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

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