-
Notifications
You must be signed in to change notification settings - Fork 470
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
Inference API wrapper client #65
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.
I'm fine with adding wrapper over the inference but as already said elsewhere, I am a bit concerned with maintenance of this as it could really go out of sync, as the groundtruth for transformers is really the pipelines
part of transformers.
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.
looks neat, but do we actually want to perform validation client-side, vs. calling the API and displaying a clear validation error here?
Co-authored-by: Julien Chaumond <julien@huggingface.co>
My initial discussion with @LysandreJik led to the idea of client-side validation, but based on the comments, I think we should remove it. I checked the returned values of the inference API when missing inputs and they are very explicit. Benefits of client-side validation:
Cons of client-side validation
Based on this and the discussion, I removed all client-side validation :) |
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 PR, this looks cool!
Regarding client-side validation, I am quite bullish on IntelliSense being a big component of the wrapper; but if it explodes complexity/maintainability then we can definitely overlook it in this first version and revisit if it makes sense later on.
also tagging @cbensimon with whom we've discussed this wrapper recently |
Co-authored-by: Julien Chaumond <julien@huggingface.co>
…into inference_wrapper
…ggingface_hub into inference_wrapper
# TODO: Decide if we should raise an error instead of | ||
# returning the json. |
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.
As mentioned offline with @osanseviero, I think handling exceptions here is the most pythonic way to handle things - and that without such logic there's little value in having a Python wrapper for the inference API if it's a glorified cURL
utility.
I would argue that the wrapper's ability to return appropriate errors such as raise ModelNotLoadedError
or raise MissingKeyError
, which inherit from the appropriate canonic errors (ValueError
, OSError
), is very important API-wise.
…into inference_wrapper
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.
Looking forward to the next PR to address the comments - agree to merge this now so that Flax users can use it for their spaces
This is still WIP, but I would like to get some feedback early on. We use the validation from the inference API to validate the inputs are ok.
Ideally this would be in a single call, but the API can expect both inputs and params (see
zero-shot
), so I addedset_inputs
andset_params
methods. Let me know what you think@julien-c @Narsil FYI