Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a helpful errorwhen commit_message is empty in create_commit #962

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

sgugger
Copy link
Contributor

@sgugger sgugger commented Aug 1, 2022

If you call create_commit with an empty commit_message (either None or "") you will get an obscure error from the server. This PR catches it earlier to send a more informative error message.

@sgugger sgugger requested a review from LysandreJik August 1, 2022 15:07
@LysandreJik LysandreJik requested a review from SBrandeis August 1, 2022 15:07
@osanseviero
Copy link
Contributor

Out of curiosity, do you have a copy of the obscure server error you got?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 1, 2022

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

@sgugger
Copy link
Contributor Author

sgugger commented Aug 1, 2022

@osanseviero Here you go (just ignore the top level of the traceback which is from Transformers):

---------------------------------------------------------------------------
HTTPError                                 Traceback (most recent call last)
/tmp/ipykernel_862308/25031559.py in <module>
----> 1 model.push_to_hub("sgugger/test-upload", commit_message="")

~/git/transformers/src/transformers/utils/hub.py in push_to_hub(self, repo_id, use_temp_dir, commit_message, private, use_auth_token, max_shard_size, create_pr, **deprecated_kwargs)
   1031             self.save_pretrained(work_dir, max_shard_size=max_shard_size)
   1032 
-> 1033             return self._upload_modified_files(
   1034                 work_dir, repo_id, files_timestamps, commit_message=commit_message, token=token, create_pr=create_pr
   1035             )

~/git/transformers/src/transformers/utils/hub.py in _upload_modified_files(self, working_dir, repo_id, files_timestamps, commit_message, token, create_pr)
    947             operations.append(CommitOperationAdd(path_or_fileobj=os.path.join(working_dir, file), path_in_repo=file))
    948         logger.info(f"Uploading the following files to {repo_id}: {','.join(modified_files)}")
--> 949         return create_commit(
    950             repo_id=repo_id, operations=operations, commit_message=commit_message, token=token, create_pr=create_pr
    951         )

~/anaconda3/lib/python3.9/site-packages/huggingface_hub/hf_api.py in create_commit(self, repo_id, operations, commit_message, commit_description, token, repo_type, revision, create_pr, num_threads)
   1842             params={"create_pr": 1} if create_pr else None,
   1843         )
-> 1844         _raise_for_status(commit_resp)
   1845         return commit_resp.json().get("pullRequestUrl", None)
   1846 

~/anaconda3/lib/python3.9/site-packages/huggingface_hub/utils/_errors.py in _raise_for_status(request)
     82         )
     83 
---> 84     _raise_with_request_id(request)
     85 
     86 

~/anaconda3/lib/python3.9/site-packages/huggingface_hub/utils/_errors.py in _raise_with_request_id(request)
     93             e.args = (e.args[0] + f" (Request ID: {request_id})",) + e.args[1:]
     94 
---> 95         raise e

~/anaconda3/lib/python3.9/site-packages/huggingface_hub/utils/_errors.py in _raise_with_request_id(request)
     88     request_id = request.headers.get("X-Request-Id")
     89     try:
---> 90         request.raise_for_status()
     91     except Exception as e:
     92         if request_id is not None and len(e.args) > 0 and isinstance(e.args[0], str):

~/anaconda3/lib/python3.9/site-packages/requests/models.py in raise_for_status(self)
    951 
    952         if http_error_msg:
--> 953             raise HTTPError(http_error_msg, response=self)
    954 
    955     def close(self):

HTTPError: 400 Client Error: Bad Request for url: https://huggingface.co/api/models/sgugger/test-upload/commit/main (Request ID: z_PAThlH9bJUSaszL4Rc0)

@julien-c
Copy link
Member

julien-c commented Aug 1, 2022

could also be solved with a better error from server (cc @SBrandeis)

@sgugger
Copy link
Contributor Author

sgugger commented Aug 1, 2022

Works for me too!

@SBrandeis
Copy link
Contributor

The server already returns an error message in the HTTP response:

{"error": "\"summary\" is not allowed to be empty"}

But it's not obvious to match summary (expected by the server) and commit_message (client API). The proposed solution is probably a better UX.

@@ -1858,6 +1858,8 @@ def create_commit(
If `create_pr` is `True`, returns the URL to the newly created Pull Request
on the Hub. Otherwise returns `None`.
"""
if commit_message is None or len(commit_message) == 0:
Copy link
Contributor

@SBrandeis SBrandeis Aug 2, 2022

Choose a reason for hiding this comment

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

Suggested change
if commit_message is None or len(commit_message) == 0:
if not commit_message:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have been burned too many times by relying on Python bool-conversion magic so I prefer explicit tests :-) But feel free to merge the suggestion if it fits the style of huggingface_hub better.

@SBrandeis SBrandeis requested a review from Wauplin August 2, 2022 12:35
@sgugger sgugger merged commit 3c24c2d into main Aug 2, 2022
@sgugger sgugger deleted the error_on_empty_commit_message branch August 2, 2022 15:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants