-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Support loading base64 images in pipelines #25633
Support loading base64 images in pipelines #25633
Conversation
cc @amyeroberts |
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 adding this!
Really useful functionality - main comment is to update all logic of the function e.g. errors raised and make it a bit more defensive.
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
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 adding this!
cc @Narsil for a second opinion on the changes for the pipeline
The PR looks good. I'm uneasy about the actual format suggested, because some stuff are base64 decodable and yet real strings. But I have the same feeling about loading images from URL so.. |
@Narsil Understood! As passing in strings is already supported for loading from url, I think this is an acceptable addition. Is there anything you'd like us to add to this PR to handle that? |
No not really. It's choice. I'm not in favor, but I'm not super strongly opposed to it either. |
Co-authored-by: amyeroberts <[email protected]>
8c63a77
to
134cbfa
Compare
@Narsil I'm inclined to agree. As we already accept strings, I'm going to merge. If continue support of this ends up being a headache (lots of code / lots of if/else logic) then we can think about ways to deprecate the support or reduce its scope. |
* support loading base64 images * add test * mention in docs * remove the logging * sort imports * update error message * Update tests/utils/test_image_utils.py Co-authored-by: amyeroberts <[email protected]> * restructure to catch base64 exception * doesn't like the newline * download files * format * optimize imports * guess it needs a space? * support loading base64 images * add test * remove the logging * sort imports * restructure to catch base64 exception * doesn't like the newline * download files * optimize imports * guess it needs a space? --------- Co-authored-by: amyeroberts <[email protected]>
What does this PR do?
Adds support for loading base64-encoded images in pipelines.
I primarily added this so it can be used by
transformers-cli serve
, since I found having to use an URL or reference to a local file a bit inconvenient for my use-case.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@Narsil