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

Support expand parameter in xxx_info and list_xxxs (model/dataset/Space) #2333

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Jun 13, 2024

Solve #2325.

EDIT: added support for model_info/dataset_info/space_info as well as list_models/list_datasets/list_spaces. Added tests and docstrings for all of them.


First draft of what we discussed in #2325 (comment) cc @osanseviero @julien-c . In the end I went for "put everything in ModelInfo and have all attributes optional". For now I only implemented expand for models helpers but it can be easily extended to datasets/spaces.

In order to better document the available parameters, I used a Literal type annotations listing all valid keys. Since it's "just" a type annotation, passing any string will still be backward compatible in the future as long as the server handles it. The cons is that the list has to be manually maintained to stay up to date with server changes (not happening very often). I have added a test for that. At least an update is pushed server-side, we will notice it in our CI runs.

@osanseviero @julien-c let me know if you are opinionated on something, otherwise I'll continue with it :)

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Wauplin Wauplin changed the title [WIP] Support expand parameterin model_info and list_models Support expand parameter in xxx_info and list_xxxs (model/dataset/Space) Jun 27, 2024
@Wauplin Wauplin marked this pull request as ready for review June 27, 2024 09:56
@Wauplin Wauplin requested review from LysandreJik, osanseviero and julien-c and removed request for LysandreJik June 27, 2024 09:57
Copy link
Member

@julien-c julien-c 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 from quick glance!

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.

oooh that's super nice! Love the change

@Wauplin
Copy link
Contributor Author

Wauplin commented Jul 11, 2024

Thanks for the reviews! 🤗

@Wauplin Wauplin merged commit 4af0294 into main Jul 11, 2024
16 of 17 checks passed
@Wauplin Wauplin deleted the 2325-support-expand-parameter branch July 11, 2024 14:47
Wauplin added a commit that referenced this pull request Jul 11, 2024
* Use extended path on Windows when downloading to local dir

Change the path of the local dir to an extended path by prepending
"\\?\" to the absolute path, when the absolute path is longer
than 255 characters on Windows.

Also fixed a small typo.

* Use extended path on Windows when downloading to local dir

Change the path of the local dir to an extended path by prepending
"\\?\" to the absolute path, when the absolute path is longer
than 255 characters on Windows.

Also fixed a small typo.

* Move path handling to `get_local_download_paths()` for robustness

On Windows we check the length of `lock_path` and if it is longer than
255 characters we prepend the `\\?\` prefix to all paths if it does not
already exist.

We only need to check the length of `lock_path` because it is guaranteed
to be the longest path.

* `safetensors[torch]` (#2371)

* Fix token=False not respected in file download (#2386)

* Fix token=False not respected in file download

* lint

* Handle shared layers in `save_torch_state_dict` + add `save_torch_model` (#2373)

* Handle shared layers in save_torch_state_dict + save_torch_model + some helpers

* fix pytest rerun

* more reruns

* Support `expand` parameter in `xxx_info` and `list_xxxs` (model/dataset/Space) (#2333)

* First draft to support `expand` parameter for models

* add expand support for dataset

* add expand support for Space

* Use extended path on Windows when downloading to local dir

Change the path of the local dir to an extended path by prepending
"\\?\" to the absolute path, when the absolute path is longer
than 255 characters on Windows.

Also fixed a small typo.

* Move path handling to `get_local_download_paths()` for robustness

On Windows we check the length of `lock_path` and if it is longer than
255 characters we prepend the `\\?\` prefix to all paths if it does not
already exist.

We only need to check the length of `lock_path` because it is guaranteed
to be the longest path.

* Use extended path on Windows when downloading to local dir

Change the path of the local dir to an extended path by prepending
"\\?\" to the absolute path, when the absolute path is longer
than 255 characters on Windows.

Also fixed a small typo.

* Removed old path handling

* Reorder path check; add tests

* Skip test if opn Windows

The test now shows up a `skipped` if executed on a non-Windows machine

Co-authored-by: Lucain <lucainp@gmail.com>

* Fix indentation for test_local_folder.py

* Fix code style

---------

Co-authored-by: Lucain <lucain@huggingface.co>
Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com>
Co-authored-by: Lucain <lucainp@gmail.com>
"downloads",
"downloadsAllTime",
"gated",
"gitalyUid",
Copy link
Member

Choose a reason for hiding this comment

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

hmm this is internal only i don't think any user will ever need it

Copy link
Member

Choose a reason for hiding this comment

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

i'd suggest removing that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #2395

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