Push to hub in a pull request
State: open

Fixes #5492.

Introduce new kwarg `create_pr` in `push_to_hub`, which is passed to `HFapi.upload_file`.

Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5528). All of your documentation changes will be reflected on that endpoint.

It seems that the parameter `create_pr` is available for [`0.8.0`](https://huggingface.co/docs/huggingface_hub/v0.8.1/en/package_reference/hf_api#huggingface_hub.HfApi.upload_file) (its not here: [`0.7.0`](https://huggingface.co/docs/huggingface_hub/v0.7.0.rc0/en/package_reference/hf_api#huggingface_hub.HfApi.upload_file)) and onwards. I included a warning, informing the user that no PR was created.

@nateraw you are completely right! Actually, the dataset shards is never added to the created pr, only the metadata, as the code is now. Ill look into you suggestion asap. Thank!

@nateraw Nothing more to add, that's a perfect usage of `huggingface_hub` as far as I can tell ! :fire: 

A very nit improvement would be to use the [for .. else ... python statement](https://book.pythontips.com/en/latest/for_-_else.html).
i.e:

```py
if create_pr is True and revision is not None:
    for discussion in get_repo_discussions(repo_id, repo_type='dataset'):
        if discussion.is_pull_request and discussion.git_reference == revision:
            create_pr = False
            break
    else:
        raise ValueError("Provided revision not found")
```
No need for the `revision_found` temporary flag when do so. Yeah ok, it's niche :wink: 

I added the suggestions from @nateraw and @Wauplin .

> Thanks. Some comments/suggestions below...
> 
> Why have you removed the test for create_pr? You could add it again and just add a pytest skipif when version of huggingface_hub is lower than 0.8.1.

I have added the test again. I removed it because i kept getting errors when calling `create_pull_request` with `repo_id=ds_name` where `temporary_repo = ds_name`, and thought i might look more thoroughly at it later. I have added a test called `test_test` showing this, it gives:
```
tests/test_upstream_hub.py:360: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.venv/lib/python3.10/site-packages/huggingface_hub/utils/_validators.py:124: in _inner_fn
    return fn(*args, **kwargs)
.venv/lib/python3.10/site-packages/huggingface_hub/hf_api.py:3451: in create_pull_request
    return self.create_discussion(
.venv/lib/python3.10/site-packages/huggingface_hub/utils/_validators.py:124: in _inner_fn
    return fn(*args, **kwargs)
.venv/lib/python3.10/site-packages/huggingface_hub/hf_api.py:3393: in create_discussion
    hf_raise_for_status(resp)
(...)
E               huggingface_hub.utils._errors.RepositoryNotFoundError: 401 Client Error. (Request ID: Root=1-63ecd2cb-2cf2557a332c86ad27f687b3)
E               
E               Repository Not Found for url: https://huggingface.co/api/models/__DUMMY_TRANSFORMERS_USER__/test-16764648321590/discussions.
E               Please make sure you specified the correct `repo_id` and `repo_type`.
E               If you are trying to access a private or gated repo, make sure you are authenticated.
E               Invalid username or password.
```

> > Thanks. Some comments/suggestions below...
> > Why have you removed the test for create_pr? You could add it again and just add a pytest skipif when version of huggingface_hub is lower than 0.8.1.
> 
> I have added the test again. I removed it because i kept getting errors when calling `create_pull_request` with `repo_id=ds_name` where `temporary_repo = ds_name`, and thought i might look more thoroughly at it later. I have added a test called `test_test` showing this, it gives:
> 
> ```
> tests/test_upstream_hub.py:360: 
> _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
> .venv/lib/python3.10/site-packages/huggingface_hub/utils/_validators.py:124: in _inner_fn
>     return fn(*args, **kwargs)
> .venv/lib/python3.10/site-packages/huggingface_hub/hf_api.py:3451: in create_pull_request
>     return self.create_discussion(
> .venv/lib/python3.10/site-packages/huggingface_hub/utils/_validators.py:124: in _inner_fn
>     return fn(*args, **kwargs)
> .venv/lib/python3.10/site-packages/huggingface_hub/hf_api.py:3393: in create_discussion
>     hf_raise_for_status(resp)
> (...)
> E               huggingface_hub.utils._errors.RepositoryNotFoundError: 401 Client Error. (Request ID: Root=1-63ecd2cb-2cf2557a332c86ad27f687b3)
> E               
> E               Repository Not Found for url: https://huggingface.co/api/models/__DUMMY_TRANSFORMERS_USER__/test-16764648321590/discussions.
> E               Please make sure you specified the correct `repo_id` and `repo_type`.
> E               If you are trying to access a private or gated repo, make sure you are authenticated.
> E               Invalid username or password.
> ```

@albertvillanova, @lhoestq , FYI I have looked at this again, and i haven't figured it out, so the test`test_push_dataset_to_hub_with_pull_request` and the minimal example `test_test` are still failing locally, while the other tests succeed. Do you have any advice?

I tried to move all of the "create pr safely"-logic to a seperate function in `_hf_hub_fixes`. I looked at how the exceptions were raised before `huggingface_hub.utils.RepositoryNotFoundError`existed, and make changes accordingly. 

`create_pr` was set during `push_to_hub`, even though it was `None` from the outset, hence causing tests to fail for older versions of `huggingface_hub`. This is now fixed.

With the implementation of `_hf_hub_fixes.upload_file` the function call expected `commit_message`, `commit_description`. If these are not set we call the function without them, even though we are on a version of `huggingface_hub` where they are not available in `upload_file`.

When `huggingface_hub < 0.5.0` we assume `repo_id` of them form `organisation/name`, so now that we are calling `create_repo` in the tests with `repo_id` not of this form, we need to handle this case, this is now done.

Many tests failed for `dataset_dict` for the above reasons, so the fixes from `arrow_dataset.py` were also added to `dataset_dict.py`. 

**All tests are now passing locally for `huggingface_hub==0.2.0` and `huggingface_hub==0.12.1`…** Im sorry I should have downgraded and went through this a long time ago, but I didn’t realise the extend of these version fixes until recently…

Hi ! FYI bumped the `huggingface-hub` dependency to 0.11 and removed the `_hf_hub_fixes.py` - which should make this PR much easier

Just now finding this - seems like a cool issue to contribute to. If any more help is needed please ping me! @AJDERS