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

Simplify manage to autodetect task+framework if possible. #122

Merged
merged 7 commits into from
Jun 21, 2021

Conversation

Narsil
Copy link
Contributor

@Narsil Narsil commented Jun 18, 2021

No description provided.

@Narsil Narsil requested a review from osanseviero June 18, 2021 14:49
@osanseviero
Copy link
Member

Could you also quickly change the inference api Actions checks to run in pull_request, not only PR? They don't seem to trigger with latest unless I do manually.

pull_request:
    types: [assigned, opened, synchronize, reopened]
    paths:
      - "api-inference-community/**"

Also, make test seems to fail with missing dependency (https://github.com/huggingface/huggingface_hub/pull/122/checks?check_run_id=2859543701)

Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks!

info = HfApi().model_info(model_id)
task = info.pipeline_tag
framework = None
frameworks = {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please sort frameworks alphabetically :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahaah is that really that important ? :D

Copy link
Member

Choose a reason for hiding this comment

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

That's why it's a nit :)

framework = tag
else:
raise Exception(
"This model seems to implement 2 frameworks, we cannot infer"
Copy link
Member

Choose a reason for hiding this comment

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

Just a comment: I wonder if this can become an issue later on with models such as sentence-transformers that work fine in both transformers and sentence-transformers.

modelInfo currently does not return the library, but I think it should. It's already there, we just need to add it to the returned modelInfo. I can send a PR for that next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julien-c is implementing library_name or something similar, so we should be able to switch to that and avoid this edge case.

Copy link
Member

Choose a reason for hiding this comment

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

This is already done now :)

Copy link
Member

Choose a reason for hiding this comment

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

You can do info.library_name

@Narsil Narsil merged commit af528fd into main Jun 21, 2021
@Narsil Narsil deleted the improve_manage branch June 21, 2021 08:44
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

2 participants