huggingface / huggingface_hub

The official Python client for the Huggingface Hub.
https://huggingface.co/docs/huggingface_hub
Apache License 2.0
1.83k stars 471 forks source link

Fix ModelHubMixin coders #2291

Closed gorold closed 3 weeks ago

gorold commented 1 month ago

This PR implements the fixes as discussed in #2283 and the relevant test cases. @Wauplin

HuggingFaceDocBuilderDev commented 1 month ago

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.

gorold commented 3 weeks ago

😅 sorry.. i'll test it on 3.8 before pushing haha...

gorold commented 3 weeks ago

This should fix it. 3.8 was still failing because the interpreter doesn't recognise the pipe operator with types, so we need to guard it to only branch to those tests in >=3.10.

There seem to be 2 test cases which fail, but they don't seem like related tests?

tests/test_offline_utils.py::test_offline_with_timeout tests/test_utils_http.py::TestConfigureSession::test_get_session_in_forked_process

Wauplin commented 3 weeks ago

Thanks for fixing the tests @gorold! The two remaining ones are indeed unrelated (I'm currently having a look at them in a separate PR). About the solution to test the pipe operator, could you add from __future__ import annotations at the beginning of the test module so that all tests are run on all Python versions? It would make the test file easier to read I think.

gorold commented 3 weeks ago

I tried from __future__ import annotations but it failed, I think because the pipe operator appears not as an annotation, but as an argument for the test function.

The other solution I have now is to convert the types into a string and use the eval function:

@pytest.mark.skipif(sys.version_info < (3, 10), reason="requires python3.10 or higher")
@pytest.mark.parametrize(
    "optional_type, inner_type",
    [
        ("None | int", int),
        ("int | None", int),
        ("None | CustomType", CustomType),
        ("CustomType | None", CustomType),
    ],
)
def test_unwrap_simple_optional_type_pipe(optional_type: str, inner_type: Type):
    assert unwrap_simple_optional_type(eval(optional_type)) is inner_type
Wauplin commented 3 weeks ago

Ah true, thanks for trying it. I prefer the eval solution better if you don't mind? At least tests are explicitly skipped instead of not defined. Last round of changes, I promise! :smile:

gorold commented 3 weeks ago

No problem, made the changes.