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

Add an implementation of HF model and an example sentiment analysis a… #194

Merged
merged 16 commits into from
Sep 10, 2023

Conversation

mhawasly
Copy link
Contributor

@mhawasly mhawasly commented Aug 31, 2023

No description provided.

Copy link
Collaborator

@fdalvi fdalvi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for starting this, will be very good to have in the package; I left a few comments, and have some more higher level questions:

  • Is this tied to a specific class of models? I see in the Inference docs https://huggingface.co/docs/api-inference/detailed_parameters that the parameters differ per task type? If not lets leave a comment (perhaps module level) that tells the users what they are expected to put in the "prompt fn"
  • Currently, there is no "input" key in the payload we send to the API, while all samples in their docs have the key - maybe "text" was an older API that is not official anymore?
  • Perhaps we can make things even clearer by changing the name of the model to HuggingFaceInferenceAPIModel; I know thats quite a mouthful but there are HuggingFace Inference Endpoints that are paid, so we should disambiguate from that (+ also disambiguate from general HuggingFace models that run locally)

Let me know what you think and if something is unclear!

llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
@mhawasly
Copy link
Contributor Author

mhawasly commented Sep 4, 2023

Thanks a lot for starting this, will be very good to have in the package; I left a few comments, and have some more higher level questions:

* Is this tied to a specific class of models? I see in the Inference docs `https://huggingface.co/docs/api-inference/detailed_parameters` that the parameters differ per task type? If not lets leave a comment (perhaps module level) that tells the users what they are expected to put in the "prompt fn"

Indeed, different task types have different input/output formats and types. I tried only an example from the classification type.

* Currently, there is no "input" key in the payload we send to the API, while all samples in their docs have the key - maybe "text" was an older API that is not official anymore?

Interesting. I did try that but "inputs" does not work, at least for the model I used in the asset. It returns {"error":"You need to specify either textortext_target.","warnings":["There was an inference error: You need to specify either textortext_target."]}'

* Perhaps we can make things even clearer by changing the name of the model to `HuggingFaceInferenceAPIModel`; I know thats quite a mouthful but there are HuggingFace Inference Endpoints that are paid, so we should disambiguate from that (+ also disambiguate from general HuggingFace models that run locally)

Sure!

Let me know what you think and if something is unclear!

Copy link
Collaborator

@fdalvi fdalvi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes; The only big thing I'm worried about is the mismatching APIs ("inputs" in the docs vs "text" that seems to work for us) - we should get this committed nevertheless, but if you have some time lets try to dig into this and see why this is happening

llmebench/models/HuggingFace.py Outdated Show resolved Hide resolved
)
if not response.ok:
if response.status_code == 503: # model loading
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason for the sleep here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hoping to give the model some time to load before retrying?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The retry mechanism has an inherent random delay (that gets sampled from an exponentially increasing range), so we don't need to worry about it here

@fdalvi fdalvi merged commit 07f4bf6 into main Sep 10, 2023
4 checks passed
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.

3 participants