Update constants imports with module level access #1172

huggingface / huggingface_hub

The official Python client for the Huggingface Hub.

https://huggingface.co/docs/huggingface_hub

Apache License 2.0

2.02k stars 531 forks source link

Update constants imports with module level access #1172 #2469

Closed WizKnight closed 1 month ago

WizKnight commented 1 month ago

This PR addresses issue #1172 by updating how constants are imported and used in the Hugging Face Hub codebase. It introduces module-level imports for better organization while maintaining backward compatibility.

Key changes:

Introduced module-level import from . import constants for improved code organization and maintainability.
Retained existing specific imports with # noqa: F401 comments for backward compatibility.

WizKnight commented 1 month ago

Hey @Wauplin, the required changes were done in this PR.

Though for ENDPOINT, both import styles were used to serve as a temporary workaround. from . import constants from .constants import ENDPOINT # For backward compatibility

Because:

When I import ENDPOINT directly from .constants import ENDPOINT, the @patch decorator in test_hf_api.py might be successfully replacing the value of ENDPOINT within the hf_api module, leading to all the tests passing.
However, when I import the entire constants module from . import constants and access ENDPOINT as constants.ENDPOINT, the @patch decorator might not be able to correctly patch this attribute, resulting in 64 tests failing.

Wauplin commented 1 month ago

Hi @WizKnight, thanks for the details. I just pushed https://github.com/huggingface/huggingface_hub/pull/2469/commits/603d6d8001a193b640cc304493c1cc32fc7248f2 that should fix it in a more future-proof way:

we use constants.ENDPOINT everywhere in hf_api.py module as all other constants
and I updated the patch decorator in the tests to patch the correct value from constants instead of hf_api

It is not a problem to update the tests like this when it's only to modify something in the test tooling (i.e. we are not changing an actual test behavior).

So now let's wait for the CI to get green and we should be good to merge :)

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.

Wauplin commented 1 month ago

And... merged! Thanks @WizKnight for the work and sorry again for the back and forth! Next PRs should be easier now :)

WizKnight commented 1 month ago

And... merged! Thanks @WizKnight for the work and sorry again for the back and forth! Next PRs should be easier now :)

It's alright, I got to learn a lot. Thanks for this opportunity @Wauplin :)

So, should I open a new PR and update all the instances of constants throughout the repository or Would you like to assign me another task??

Wauplin commented 1 month ago

So, should I open a new PR and update all the instances of constants throughout the repository or Would you like to assign me another task??

Now that you've already worked on this task, I think that yes it makes sense for you to update all the instances of constants throughout the codebase. Thanks!

WizKnight commented 1 month ago

Now that you've already worked on this task, I think that yes it makes sense for you to update all the instances of constants throughout the codebase. Thanks!

Alright then, I'm on it!!