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

added zero shot image classification #1528

Merged
merged 6 commits into from
Jun 29, 2023

Conversation

dulayjm
Copy link
Contributor

@dulayjm dulayjm commented Jun 26, 2023

👋 Hi, first-time potential contributor here! I noticed no method for zero-shot-image-classification in the inference api, and I wrote an implementation to call for that task.

🤗 Solution:

  • Add post response in _client.py to call a zero-shot image classification model (such as CLIP).
  • Added tests and relevant documentation.

TODO:

Test other models

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Hi @dulayjm! Thanks for opening a PR for this task 🎉 Not all tasks are indeed implemented as the client is quite new and we want other tasks to be more community-lead so I'm glad to see your contribution 🤗

I reviewed your PR and made some comments to it. I hope everything's clear but don't hesitate to ping me if not. In general, it would be good to review the inputs/output of the method and adapt the corresponding docs/tests :)

src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
@@ -0,0 +1,43 @@
interactions:
Copy link
Contributor

Choose a reason for hiding this comment

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

This file must be generated automatically (here the format is not correct). To do so, you must have pytest-vcr installed + run pytest tests/test_inference_client.py -k "zero" --vcr-record=all to record the HTTP call. Don't worry if it doesn't work for you, I can take of it :) Just be careful of not committing your HF token in the yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was extremely helpful! I could not find the command to generate this properly. With the typing changes, it passes all tests!

tests/test_inference_client.py Outdated Show resolved Hide resolved
@dulayjm
Copy link
Contributor Author

dulayjm commented Jun 27, 2023

@Wauplin Thank you for the suggested changes!! These were very helpful! In response, I implemented the following:

  • Addressed a typo in the docs
  • Change the input type to List[str] to enforce a precise expected input type for labels
  • Added a raise to ValueError if the length of label responses is less than 2.
  • Changed the output response type to reflect the OpenCLIP model List[ClassificationOutput]
  • Created the VCR testing file
  • Updated a better test in test_inference_client.py to address the typing and expected outputs for the model and task.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Really nice! I made a quick review of the code and left a few minor comments. I'll run some tests and let you know for a final go!

src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
tests/test_inference_client.py Outdated Show resolved Hide resolved
src/huggingface_hub/inference/_client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Perfect! Great job here @dulayjm 👏 I'll merge it as soon as the CI is green 🟢

Would you mind sharing for what project you're interested in using this client? I'd be glad to know more :)

@Wauplin Wauplin merged commit 8e0113e into huggingface:main Jun 29, 2023
@Wauplin Wauplin mentioned this pull request Jun 29, 2023
7 tasks
@dulayjm
Copy link
Contributor Author

dulayjm commented Jun 29, 2023

@Wauplin Thanks for the review! Happy to see this get merged for HF 🤗

I'm using this for a project within my company to quickly deploy CLIP (and a variant of it, fashion-clip to quickly and accurately label items of clothing that our users at Capsule (YC 22 - coming soon!!) can upload to the platform!

@Wauplin
Copy link
Contributor

Wauplin commented Jun 29, 2023

Nice!

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