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

Adding api-inference-community to huggingface_hub. #48

Merged
merged 9 commits into from
May 25, 2021

Conversation

Narsil
Copy link
Contributor

@Narsil Narsil commented May 20, 2021

  • The idea is to make all external contributions centralized.
  • Moved the quality checks (could make them uniform later)
  • api-inference-community should be relatively independant from huggingface_hub still.

@Narsil Narsil requested a review from julien-c May 20, 2021 14:28
@julien-c julien-c requested review from LysandreJik and n1t0 May 20, 2021 14:34
@julien-c
Copy link
Member

Thanks! Looks good to me but adding @LysandreJik and @n1t0 to the party

@julien-c
Copy link
Member

Does the CI take a long time? If it does, maybe we'll want to restrict the Inference-API's CI to changes to that folder.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Cool, looking forward to having everything centralized.

@Narsil
Copy link
Contributor Author

Narsil commented May 21, 2021

Not sure how easy to run conditional actions but if it's easy then yes let's do it !

Otherwise the CI is super fast as we only run the package CI, everything else is considered "slow" as it would require model downloads (and I don't want to force users to test against dummy models as it is a bit more complex to do).

Before deployment, we run the test_dockers slow CI which actually build the container + run the test suite on those with a live model. We can do that framework per framework making it livable (running tests against all frameworks all the time seems a bit like a waste since we're relying on docker to make sure no unforeseen change happens within the API)

Conditional full docker tests would be the best so that PR creators can be alerted early if something doesn't fit.

.github/workflows/python-api-quality.yaml Outdated Show resolved Hide resolved
.github/workflows/python-api-tests.yaml Outdated Show resolved Hide resolved
.github/workflows/python-api-quality.yaml Outdated Show resolved Hide resolved
.github/workflows/python-api-tests.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Narsil and others added 8 commits May 21, 2021 11:56
Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
@Narsil
Copy link
Contributor Author

Narsil commented May 25, 2021

Hi @julien-c do you think the changes made are OK?

I added a simple docker test only for allennlp (afaik, I would need one for each framework, because we can't have multiple on clauses in a single yaml file).

Also I am not 100% sure about this docker because it does not seem to run on this PR, is that normal ? I though GH actions should run on current PR, but maybe I'm wrong.

@julien-c
Copy link
Member

not sure why the CI suite doesn't run, maybe it will after we merge this first PR? or maybe add a pull_request: event clause with the same paths?

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Narsil!

I'm pretty sure it's normal that the CI doesn't run as long as this isn't on master, we had the same issue on transformers a while back.

@Narsil Narsil merged commit c5542db into main May 25, 2021
@Narsil Narsil deleted the add_api_inference_community branch May 25, 2021 15:40
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.

None yet

3 participants