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

Enabling Keras download stats #860

Merged
merged 1 commit into from May 4, 2022
Merged

Enabling Keras download stats #860

merged 1 commit into from May 4, 2022

Conversation

merveenoyan
Copy link
Contributor

This PR adds library_name and library_version to snapshot_download in Keras mixin.

I think existing from_pretrained_keras tests should cover this so I didn't write an additional test.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 3, 2022

The documentation is not available anymore as the PR was closed or merged.

@merveenoyan
Copy link
Contributor Author

I'm checking out why test_filter_models_with_complex_query and test_tags are failing.

@adrinjalali
Copy link
Contributor

What is the consequence of this change?

@merveenoyan
Copy link
Contributor Author

(Leaving notes to myself)
I don't think it's related to my PR honestly but:

(Pdb) api.get_model_tags()
*** KeyError: 'language'

(called from __init__ of ModelSearchArguments() that is called inside the test)
but I do have "language" in the response returned from get_model_tags() 🧐

@merveenoyan
Copy link
Contributor Author

merveenoyan commented May 3, 2022

@adrinjalali when we add library name and version to snapshot download it sends a request with the info inside the header that later gives us statistics about Keras downloads.

@adrinjalali
Copy link
Contributor

Ah I see, so it doesn't really change anything on the client side. Got it.

@merveenoyan
Copy link
Contributor Author

There's a problem with dataset tags. I'm fixing it.

@merveenoyan
Copy link
Contributor Author

merveenoyan commented May 3, 2022

There's a dataset that is empty after the dataset batterypapers, (this is the dict of that dataset
{'id': 'dataset:', 'label': '', 'type': 'dataset'},).
It breaks _unpack_and_assign_dictionary() which causes a bug in the tests (and initialization of DatasetTags class in general).
To whom can I talk to? Maybe @osanseviero and @lhoestq.

@adrinjalali
Copy link
Contributor

adrinjalali commented May 3, 2022

Should we be ignoring those datasets then? As in, if there's an issue creating the class, ignore the entry. In general, we should be safe against malformed data that people put on the hub.

@merveenoyan
Copy link
Contributor Author

@adrinjalali if @osanseviero approves. Zach said he reached out to owners of dataset to remove them.

@osanseviero
Copy link
Member

This internal PR from @julien-c will prevent this situation in the future https://github.com/huggingface/moon-landing/pull/2822

@merveenoyan
Copy link
Contributor Author

@osanseviero I re-ran and problems seem to be gone.

@osanseviero
Copy link
Member

Yes, we chatted with the model authors and fixed in the backend. Thanks!

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 good! Thanks 🔥

@osanseviero osanseviero merged commit 7042df3 into huggingface:main May 4, 2022
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

4 participants