huggingface-cli upload - Validate README.md before file hashing

[hlky]

• README.md validation is moved from create_commit to _prepare_upload_folder_additions.
• _prepare_upload_folder_additions is moved into HfApi class, this provides access to _build_hf_headers.
• repo_type and token are added to _prepare_upload_folder_additions's parameters, these are needed for the request to /api/validate-yaml endpoint.
• Result from filter_repo_objects stored in filtered_repo_objects (converted to a list, as it's a generator) to avoid recalculation as it is now used to find README.md then later to return the CommitOperationAdds.
• If found, a temporary CommitOperationAdd is created for README.md to provide as_file.
• break added after README.md validation to avoid iterating other files.

Closes #2451

[Wauplin]

Hi @hlky, thanks for the PR and detailed description. I've left a couple comments to address before merging. The two main things are:

• I'd prefer to separate the "yaml validation" from the "list and filter files" logic
• we have to validate the yaml in create_commit in any case (not a problem to do it twice if upload_folder has been called)

[Wauplin]

And then, instead of the for ... in ...: if ... == "README.md": .... ... ... break logic, could you use relpath_to_abspath instead like this:

if "README.md" in filtered_repo_objects:
    self._validate_yaml(
        content=relpath_to_abspath["README.md"].read_text(),
        repo_type=repo_type,
        token=token,
    )

This way, it's not even needed to build the CommitOperationAdd just to read the file.

[hlky]

Thanks, much better to use read_text from pathlib and avoid the for loop.

[Wauplin]

Thanks for the prompt changes @hlky! Except for the docstring, I think we are close to merge this PR :) I'll also trigger the CI to check nothing broke.

[Wauplin]

Suggested change

	f"{ENDPOINT}/api/validate-yaml",
	f"{self.endpoint}/api/validate-yaml",

I just realized we must use self.endpoint (configurable by user) rather than ENDPOINT here! It was a mistake in existing implementation but better to fix it as part of this PR :)

[Wauplin]

Adding a small docstring would be great yes. Thanks in advance!

[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]

Thank you for the clear docstring! Looks ready to be merged! I'll just wait for the CI to pass before doing so :)

[hlky]

The only other thing I can think of is that there will be duplicate calls to the endpoint with upload_folder path, once during _prepare_upload_folder_additions then again during create_commit, if the extra load to the endpoint will be an issue there could be something like a boolean self.yaml_validated to skip the validation in create_commit.

[Wauplin]

Yeah, I thought the same as well - thanks for raising the question. Honestly, I don't think the extra overhead is really problematic. We could also have a small @functools.cache decorator on _validate_yaml (though it would be nice to cache only based on content+repo_type, not the token)

[hlky]

The fastai integration test failure appears to be because repo_type is None. It hasn't triggered before repo_type is set to default value in create_commit. I'd suggest we duplicate that to upload_folder.

import requests

content = '---\ntags:\n- fastai\n---\n\n# Amazing!\n\n🥳 Congratulations on hosting your fastai model on the Hugging Face Hub!\n\...n## Intended uses & limitations\nMore information needed\n\n## Training and evaluation data\nMore information needed\n'
url = "https://hub-ci.huggingface.co/api/validate-yaml"
r = requests.post(url, {"content": content, "repoType": None}) # {'error': '"value" does not match any of the allowed types'}

r = requests.post(url, {"content": content, "repoType": "model"}) # {'errors': [], 'warnings': []}

[Wauplin]

Thanks for looking into it. Yes, I would simply duplicate this line in def _validate_yaml as you mentioned.

repo_type = repo_type if repo_type is not None else REPO_TYPE_MODEL

[Wauplin]

I would move this to _validate_yaml directly to prevent errors in the future in case we use it somewhere else

[hlky]

Thanks, I've moved it to _validate_yaml.

[Wauplin]

Perfect! Thanks for the iterations on this @hlky! Failing tests are unrelated to this PR so we are good to merge now! 🎉