Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Update constants imports with module level access #1172 #2469

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

WizKnight
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

Wauplin commented Aug 21, 2024

Hi @WizKnight, thanks for the details. I just pushed 603d6d8 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

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.

Comment on lines 47 to 50
REPO_TYPE_DATASET, # noqa: F401 # kept for backward compatibility
REPO_TYPE_MODEL, # noqa: F401 # kept for backward compatibility
REPO_TYPE_SPACE, # noqa: F401 # kept for backward compatibility
SPACES_SDK_TYPES, # noqa: F401 # kept for backward compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to keep backward compatibility in tests since tests are never imported from another library

@Wauplin Wauplin merged commit ec5812d into huggingface:main Aug 21, 2024
12 of 14 checks passed
@Wauplin
Copy link
Contributor

Wauplin commented Aug 21, 2024

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

@WizKnight
Copy link
Contributor Author

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
Copy link
Contributor

Wauplin commented Aug 22, 2024

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
Copy link
Contributor Author

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!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants