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.

Sign up for GitHub

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

Jump to bottom

Import constants module instead of importing constant values directly #1172

Closed
Wauplin opened this issue Nov 9, 2022 · 12 comments
Closed

Import constants module instead of importing constant values directly #1172

Wauplin opened this issue Nov 9, 2022 · 12 comments
Labels
good first issue Good for newcomers
Milestone
in next release?

Comments

@Wauplin
Copy link
Contributor

Wauplin commented Nov 9, 2022 •
edited
Loading

Best described by @albertvillanova in huggingface/datasets#5196 (comment) and huggingface/datasets#5196 (comment).

Basically, change from

from huggingface_hub.constants import CONSTANT_VALUE

CONSTANT_VALUE
(...)

to

from huggingface_hub import constants

constants.CONSTANT_VALUE
(...)

everywhere.

⚠️ This will most probable brake some monkey-patching in downstream libraries. Let's be careful about how to perform such a change. E.g. let's do it and make some tests before merging.
@Wauplin Wauplin mentioned this issue Nov 9, 2022
Merged
@Wauplin Wauplin added this to the v0.13 milestone Jan 19, 2023
@Wauplin Wauplin added the good first issue Good for newcomers label Feb 29, 2024
@Wauplin Wauplin mentioned this issue Mar 21, 2024
Merged
@dankornas
Copy link

Hi, is this issue still available to work on? If yes, I would like to give it a shot. I have been eager to contribute to HuggingFace, and this would be a nice start.

@Wauplin
Copy link
Contributor Author

Wauplin commented Apr 20, 2024

Hi @dankornas, thanks for proposing to contribute on this one! It is open for contributions yes :) Maybe as a start you can work on a first PR that makes the change only in a few modules (instead of the entire codebase at once) so that we can iterate on it. What do you think? If you have any questions, please let me know!

Thanks in advance 🤗

@dankornas
Copy link

Hey @Wauplin, sounds good to me! Based on the comments that you provided in your first comment, I think I have a pretty good idea of what we want to do here. So, I will start playing around with this whenever I can and once I have something or if I have any questions, I'll be sure to get back to you :)

@Wauplin
Copy link
Contributor Author

Wauplin commented Apr 23, 2024

Good, sounds like we have a plan then :) Feel free to ping me whenever!

@Wauplin Wauplin mentioned this issue Aug 15, 2024
Closed
WizKnight added a commit to WizKnight/huggingface_hub that referenced this issue Aug 15, 2024
@WizKnight
Update constants import to use module-level access (huggingface#1172)
91038ba
@WizKnight WizKnight mentioned this issue Aug 15, 2024
Merged
@WizKnight
Copy link
Contributor

Hey @Wauplin , the task was completed to update constants in hf_api.py, can I work on other files as well or first wait for it to get reviewed and merged

@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 16, 2024

I think it's best to definitively complete the first PR first, which should happen very soon!

WizKnight added a commit to WizKnight/huggingface_hub that referenced this issue Aug 18, 2024
@WizKnight
Update constants import to use module-level access huggingface#1172 h…
e782532 
…uggingface#2453
WizKnight added a commit to WizKnight/huggingface_hub that referenced this issue Aug 19, 2024
@WizKnight
Update constants imports with backward compatibility huggingface#1172, …
5bb271c 
…huggingface#2453
Wauplin added a commit that referenced this issue Aug 20, 2024
@WizKnight @Wauplin
Update constants import to use module-level access #1172 (#2453)
1b9517d 
* Update `constants` import to use module-level access (#1172)

* Update constants import to use module-level access #1172 #2453

* Update constants imports with backward compatibility #1172, #2453

* Update fastai_utils.py

* Update hf_api.py

* remove unecessary changes

---------

Co-authored-by: Lucain Pouget <lucainp@gmail.com>
@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 20, 2024

Hi @WizKnight , I just merged #2453! Thanks for the contributions 🤗 Would you like to open a follow-up PR to address all other constants occurrences? :)

@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 20, 2024

Hi @WizKnight, I'm very sorry, I closed #2453 too quickly. My fault on that.

What I meant in #2453 (comment) is that we should keep all the imports for backward compatibility BUT we should still use constants. ... in the module (for example, keeping from .constants import REPO_TYPES_MAPPING, adding from . import constants and use constants.REPO_TYPES_MAPPING in the code). So most of the things in commit 5bb271c can be reverted. Very sorry about the misunderstanding. Could you open a new PR for that since I merged the previous one? Thanks a lot!

@WizKnight
Copy link
Contributor

WizKnight commented Aug 20, 2024 •
edited
Loading

Hi @WizKnight, I'm very sorry, I closed #2453 too quickly. My fault on that.

What I meant in #2453 (comment) is that we should keep all the imports for backward compatibility BUT we should still use constants. ... in the module (for example, keeping from .constants import REPO_TYPES_MAPPING, adding from . import constants and use constants.REPO_TYPES_MAPPING in the code). So most of the things in commit 5bb271c can be reverted. Very sorry about the misunderstanding. Could you open a new PR for that since I merged the previous one? Thanks a lot!

Okay, I'll open a new PR and will keep this with backward compatibility from .constants import REPO_TYPES_MAPPING, and add from . import constants and use constants.REPO_TYPES_MAPPING in the code.

Right??

@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 20, 2024

and use constants.REPO_TYPES_MAPPING in the code.

And use all other constants as constants.MY_CONSTANT as well. Right yes!

@WizKnight
Copy link
Contributor

and use constants.REPO_TYPES_MAPPING in the code.

And use all other constants as constants.MY_CONSTANT as well. Right yes!

Alright, I'm on it!

WizKnight added a commit to WizKnight/huggingface_hub that referenced this issue Aug 20, 2024
@WizKnight
Update constants imports with module level access and backward compat…
6be1e46 
…ibility huggingface#1172
@WizKnight WizKnight mentioned this issue Aug 20, 2024
Merged
Wauplin added a commit that referenced this issue Aug 21, 2024
@WizKnight @Wauplin
Update constants imports with module level access #1172 (#2469)
ec5812d 
* Update constants imports with module level access and backward compatibility #1172

* Use constants.ENDPOINT and fix patch

* remove useless import

---------

Co-authored-by: Lucain Pouget <lucainp@gmail.com>
Co-authored-by: Lucain <lucain@huggingface.co>
@WizKnight WizKnight mentioned this issue Aug 26, 2024
Merged
@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 27, 2024

Closing this issue as completed by #2489. Thanks @WizKnight!

@Wauplin Wauplin closed this as completed Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
in next release?
Development

No branches or pull requests
3 participants
@Wauplin @dankornas @WizKnight