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

Document Collection API #973

Merged
merged 5 commits into from
Sep 28, 2023
Merged

Document Collection API #973

merged 5 commits into from
Sep 28, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Sep 27, 2023

Follow-up after the revamp of the endpoints documentation (#962).

This PR:

Updated page: https://moon-ci-docs.huggingface.co/docs/hub/pr_973/en/api.
EDIT: generated docs seems broken to me 😕 Same for you? Will investigate that.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 27, 2023

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

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Nice work! 👏

docs/hub/api.md Show resolved Hide resolved
docs/hub/api.md Show resolved Hide resolved
docs/hub/api.md Show resolved Hide resolved
docs/hub/api.md Outdated Show resolved Hide resolved
This is equivalent to `huggingface_hub.whoami()`.
Copy link
Member

Choose a reason for hiding this comment

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

It may be nice to add a link to it :)

Suggested change
This is equivalent to `huggingface_hub.whoami()`.
This is equivalent to [`huggingface_hub.whoami`].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better but auto-generated links won't work here as it's not in the same docs (hub-docs vs huggingface_hub docs). So for now I suggest we keep it like this.

(same below)

```

This is equivalent to `huggingface_hub.update_collection_metadata()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is equivalent to `huggingface_hub.update_collection_metadata()`.
This is equivalent to [`huggingface_hub.update_collection_metadata`].

Return a collection. This is a non-revertible operation. A deleted collection cannot be restored.

This is equivalent to `huggingface_hub.delete_collection()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is equivalent to `huggingface_hub.delete_collection()`.
This is equivalent to [`huggingface_hub.delete_collection`].

```

This is equivalent to `huggingface_hub.add_collection_item()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is equivalent to `huggingface_hub.add_collection_item()`.
This is equivalent to [`huggingface_hub.add_collection_item`].

```

This is equivalent to `huggingface_hub.update_collection_item()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is equivalent to `huggingface_hub.update_collection_item()`.
This is equivalent to [`huggingface_hub.update_collection_item`].

Remove an item from a collection. You must know the item object id which is different from the repo_id/paper_id provided when adding the item to the collection. The `item_id` can be retrieved by fetching the collection.

This is equivalent to `huggingface_hub.delete_collection_item()`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is equivalent to `huggingface_hub.delete_collection_item()`.
This is equivalent to [`huggingface_hub.delete_collection_item`].

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.

very small OCD/nit but i would have done two different PRs (one for the formatting, one for the Collections doc)

looks good from a quick glance but left a few comms more on the API design side of things

docs/hub/api.md Outdated
This is equivalent to `huggingface_hub.delete_collection()`.

### POST /api/collections/{namespace}/{slug}-{id}/items
Copy link
Member

Choose a reason for hiding this comment

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

those endpoints don't accept just {id}, they also require the slug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can implem that server side (in a future PR) if it improves DX significantly (maybe not worth it?)

Copy link
Contributor Author

@Wauplin Wauplin Sep 28, 2023

Choose a reason for hiding this comment

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

Not sure it'll help much. In any case the user will have to copy paste something right? (and copy-pasting username/slug-id from a URL is easier than copy-pasting username and id separately). And if not copy-pasting then it'll be programmatically done and in that case the server returns the full version (username/slug-id).

(but yeah, wouldn't cost must to support it)

Copy link
Member

Choose a reason for hiding this comment

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

good point about the copy-pasting

### POST /api/collections

Create a new collection on the Hub with a title, a description (optional) and a first item (optional). An item is defined by a type (`model`, `dataset`, `space` or `paper`) and an id (repo_id or paper_id on the Hub).
Copy link
Member

Choose a reason for hiding this comment

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

it's weird to me that you can create a collection with a first item (but not several items, i assume?) vs. creating a collection then adding an item from a differnt call.

(this comment is about the API design, not really about the documentation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree. I assume this mostly comes from the fact that on the Hub we can create a collection from a model page (which means creating it with a single item).

FYI, what I did in huggingface_hub is to not support creating a collection with an item directly. Users have to create collection and then add items to it.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, perfect if you did that in hfh then

docs/hub/api.md Outdated Show resolved Hide resolved
Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>
Co-authored-by: Julien Chaumond <julien@huggingface.co>
docs/hub/api.md Show resolved Hide resolved
docs/hub/api.md Show resolved Hide resolved
@Wauplin Wauplin merged commit 23a02aa into main Sep 28, 2023
1 check passed
@Wauplin Wauplin deleted the document-collection-api branch September 28, 2023 11:27
@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 28, 2023

Cool, merging this. Thanks @stevhliu @julien-c for the review. I'm closing the related issue (#967) as well.

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.

4 participants