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

WizKnight · 2024-08-15T18:14:58Z

This PR addresses issue #1172 by updating the way constants are imported and used throughout the Hugging Face Hub codebase. Instead of directly importing specific constants, we now import the entire constants module and access constants as its attributes.

Wauplin

Thanks @WizKnight, that's it yes! Let's wait for the CI to pass and then we are good to merge this one :)

Wauplin · 2024-08-16T11:13:22Z

@WizKnight code quality seems to be complaining. To fix this, you must run make style locally which will fix the issues. Then you can run make quality to check everything's good. Finally, commit and push the changes.

HuggingFaceDocBuilderDev · 2024-08-16T11:14:39Z

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 · 2024-08-16T14:11:21Z

src/huggingface_hub/hf_api.py

-from .constants import (
-    DEFAULT_ETAG_TIMEOUT,
-    DEFAULT_REQUEST_TIMEOUT,
-    DEFAULT_REVISION,
-    DISCUSSION_STATUS,
-    DISCUSSION_TYPES,
-    ENDPOINT,
-    INFERENCE_ENDPOINTS_ENDPOINT,
-    REGEX_COMMIT_OID,
-    REPO_TYPE_MODEL,
-    REPO_TYPES,
-    REPO_TYPES_MAPPING,
-    REPO_TYPES_URL_PREFIXES,
-    SAFETENSORS_INDEX_FILE,
-    SAFETENSORS_MAX_HEADER_LENGTH,
-    SAFETENSORS_SINGLE_FILE,
-    SPACES_SDK_TYPES,
-    WEBHOOK_DOMAIN_T,
-    DiscussionStatusFilter,
-    DiscussionTypeFilter,
-)


@WizKnight it seems this PR broke something in the tests. There are 178 failed test with the same error:

FAILED ../tests/test_file_download.py::TestNormalizeEtag::test_resolve_endpoint_on_regular_file - AttributeError: <module 'huggingface_hub.hf_api' from '/home/runner/work/huggingface_hub/huggingface_hub/src/huggingface_hub/hf_api.py'> does not have the attribute 'ENDPOINT'

Makes me realize we should keep all those previously existing constants in the module even if we don't use them. You can do that by adding

# noqa: F401 # kept for backward compatibility

to each of them. Since huggingface_hub.hf_api is "public", we can't assume that no-one in the ecosystem has imported a constant from this module. It's unfortunately a bit ugly but we have to do that for backward compatibility.

@Wauplin Yeah, it seems so and all those tests have just one common AttributeError: 'ENDPOINT'. I worked on those failed tests, I got it down to 94 failed, 189 passed, 6 skipped, 3 errors.
Though for make quality: mypy src
Success: no issues found in 106 source files

So now how should I proceed hereafter??

@Wauplin All the tests passed!!! Just for ENDPOINT I reverted back to the previous import. It resolved the issue.

What I meant is that we should keep the imports with

from .constants import ( DEFAULT_ETAG_TIMEOUT, # noqa: F401 # kept for backward compatibility DEFAULT_REQUEST_TIMEOUT, # noqa: F401 # kept for backward compatibility DEFAULT_REVISION, # noqa: F401 # kept for backward compatibility DISCUSSION_STATUS, # noqa: F401 # kept for backward compatibility DISCUSSION_TYPES, # noqa: F401 # kept for backward compatibility ENDPOINT, # noqa: F401 # kept for backward compatibility INFERENCE_ENDPOINTS_ENDPOINT, # noqa: F401 # kept for backward compatibility REGEX_COMMIT_OID, # noqa: F401 # kept for backward compatibility REPO_TYPE_MODEL, # noqa: F401 # kept for backward compatibility REPO_TYPES, # noqa: F401 # kept for backward compatibility REPO_TYPES_MAPPING, # noqa: F401 # kept for backward compatibility REPO_TYPES_URL_PREFIXES, # noqa: F401 # kept for backward compatibility SAFETENSORS_INDEX_FILE, # noqa: F401 # kept for backward compatibility SAFETENSORS_MAX_HEADER_LENGTH, # noqa: F401 # kept for backward compatibility SAFETENSORS_SINGLE_FILE, # noqa: F401 # kept for backward compatibility SPACES_SDK_TYPES, # noqa: F401 # kept for backward compatibility WEBHOOK_DOMAIN_T, # noqa: F401 # kept for backward compatibility DiscussionStatusFilter, # noqa: F401 # kept for backward compatibility DiscussionTypeFilter, # noqa: F401 # kept for backward compatibility )

Ugly? yes. But backward compatible and might save us some headaches.

Okay, I'll add backward compatibility to the imports

…uggingface#2453

Wauplin

Hi @WizKnight, I've made a new review. I'm not sure about the purpose of most of the new changes. It seems that you had some issues with your local setup. I'd advice you to install and upgrade all dev dependencies in a new virtual environment. If you have any question, please let me know.

Wauplin · 2024-08-19T13:39:36Z

Makefile

+	python3 utils/check_inference_input_params.py
+	python3 utils/check_contrib_list.py
+	python3 utils/check_static_imports.py
+	python3 utils/generate_async_inference_client.py


These changes should not be committed. If you struggle with python version, I advice you to use a virtual environment. See installation guide for more details: https://huggingface.co/docs/huggingface_hub/installation#install-with-pip

I'm using virtual environment with python 3.10 and all the required dependencies installed.
Sure I'll fix the changes

Wauplin · 2024-08-19T13:39:43Z

Makefile

+	python3 utils/check_contrib_list.py --update
+	python3 utils/check_static_imports.py --update
+	python3 utils/generate_async_inference_client.py --update
+	ruff format src/huggingface_hub/hf_api.py


same comment here

Wauplin · 2024-08-19T13:40:40Z

src/huggingface_hub/__init__.py

@@ -143,14 +143,11 @@
        "CommitOperationAdd",
        "CommitOperationCopy",
        "CommitOperationDelete",
-        "DatasetInfo",


All of these objects must not be removed from __init__.py (ModelInfo, SpaceInfo, get_user_overview, ... as well). Can you put them back? Thanks!

Yes, I will

Wauplin · 2024-08-19T13:54:48Z

src/huggingface_hub/hf_api.py


        # Optimize operations: if some files will be overwritten, we don't need to delete them first
        if len(add_operations) > 0:
            added_paths = set(op.path_in_repo for op in add_operations)
            delete_operations = [
                delete_op for delete_op in delete_operations if delete_op.path_in_repo not in added_paths
            ]
-        commit_operations = delete_operations + add_operations
+            commit_operations = cast(List[CommitOperation], delete_operations) + cast(


Why that? If it's because of an issue when running make quality locally, then you might not have the correct requirements installed.

Okay, I'll redo it from scratch. Even after that if problem persists, I'll inform you.

Wauplin · 2024-08-19T13:54:54Z

src/huggingface_hub/hf_api.py

+        # Assuming CommitOperationBase is the common base class
+        CommitOperation = Union[CommitOperationAdd, CommitOperationDelete]


Wauplin · 2024-08-19T13:56:14Z

src/huggingface_hub/hub_mixin.py

-            if is_dataclass(cls):
-                for key in cls.__dataclass_fields__:
+            if is_dataclass(cls) and isinstance(cls, type):
+                # Get type hints for the class
+                cls_hints = get_type_hints(cls)
+            if "__dataclass_fields__" in cls_hints:
+                for key in cls_hints["__dataclass_fields__"]:
                    if key not in model_kwargs and key in config:
                        model_kwargs[key] = config[key]
-            elif any(param.kind == inspect.Parameter.VAR_KEYWORD for param in cls._hub_mixin_init_parameters.values()):
-                for key, value in config.items():
-                    if key not in model_kwargs:
-                        model_kwargs[key] = value


If this is to fix an unrelated issue, better to open a separate PR.

This was for the Error: Module "_typeshed" has no attribute "DataclassInstance" &
Error: "Type[T]" has no attribute "__dataclass_fields__"

Wauplin · 2024-08-19T13:56:29Z

src/huggingface_hub/utils/_fixes.py

@@ -92,21 +92,23 @@ def WeakFileLock(lock_file: Union[str, Path]) -> Generator[BaseFileLock, None, N

    An INFO log message is emitted every 10 seconds if the lock is not acquired immediately.
    """
-    lock = FileLock(lock_file, timeout=constants.FILELOCK_LOG_EVERY_SECONDS)
+    lock_file_str = str(lock_file)  # Convert to string


is it necessary?

For this:
src/huggingface_hub/utils/_fixes.py:95: error: Argument 1 to "UnixFileLock" has incompatible type "Union[str, Path]"; expected "str"
src/huggingface_hub/utils/_fixes.py:108: error: Incompatible types in assignment (expression has type "SoftFileLock", variable has type "UnixFileLock")
src/huggingface_hub/utils/_fixes.py:108: error: Argument 1 to "SoftFileLock" has incompatible type "Union[str, Path]"; expected "str"

Wauplin · 2024-08-19T13:59:44Z

src/huggingface_hub/hf_api.py

-from .constants import (
-    DEFAULT_ETAG_TIMEOUT,
-    DEFAULT_REQUEST_TIMEOUT,
-    DEFAULT_REVISION,
-    DISCUSSION_STATUS,
-    DISCUSSION_TYPES,
-    ENDPOINT,
-    INFERENCE_ENDPOINTS_ENDPOINT,
-    REGEX_COMMIT_OID,
-    REPO_TYPE_MODEL,
-    REPO_TYPES,
-    REPO_TYPES_MAPPING,
-    REPO_TYPES_URL_PREFIXES,
-    SAFETENSORS_INDEX_FILE,
-    SAFETENSORS_MAX_HEADER_LENGTH,
-    SAFETENSORS_SINGLE_FILE,
-    SPACES_SDK_TYPES,
-    WEBHOOK_DOMAIN_T,
-    DiscussionStatusFilter,
-    DiscussionTypeFilter,
-)


What I meant is that we should keep the imports with

from .constants import ( DEFAULT_ETAG_TIMEOUT, # noqa: F401 # kept for backward compatibility DEFAULT_REQUEST_TIMEOUT, # noqa: F401 # kept for backward compatibility DEFAULT_REVISION, # noqa: F401 # kept for backward compatibility DISCUSSION_STATUS, # noqa: F401 # kept for backward compatibility DISCUSSION_TYPES, # noqa: F401 # kept for backward compatibility ENDPOINT, # noqa: F401 # kept for backward compatibility INFERENCE_ENDPOINTS_ENDPOINT, # noqa: F401 # kept for backward compatibility REGEX_COMMIT_OID, # noqa: F401 # kept for backward compatibility REPO_TYPE_MODEL, # noqa: F401 # kept for backward compatibility REPO_TYPES, # noqa: F401 # kept for backward compatibility REPO_TYPES_MAPPING, # noqa: F401 # kept for backward compatibility REPO_TYPES_URL_PREFIXES, # noqa: F401 # kept for backward compatibility SAFETENSORS_INDEX_FILE, # noqa: F401 # kept for backward compatibility SAFETENSORS_MAX_HEADER_LENGTH, # noqa: F401 # kept for backward compatibility SAFETENSORS_SINGLE_FILE, # noqa: F401 # kept for backward compatibility SPACES_SDK_TYPES, # noqa: F401 # kept for backward compatibility WEBHOOK_DOMAIN_T, # noqa: F401 # kept for backward compatibility DiscussionStatusFilter, # noqa: F401 # kept for backward compatibility DiscussionTypeFilter, # noqa: F401 # kept for backward compatibility )

Ugly? yes. But backward compatible and might save us some headaches.

…huggingface#2453

Wauplin · 2024-08-20T08:49:08Z

Hi @WizKnight, I've checked out locally your changes to test typing issues you were having. I can't reproduce them and the CI either. I suspect that we are not on the same version (currently using mypy==1.5.1 in CI). For now I reverted the typing-related changes from this PR (see 62af016). I'll have a look to what you mention though but keeping it for a separate one.

CI is now passing except for unrelated errors. Time to merge! Thanks again for the contribution!

Update constants import to use module-level access (huggingface#1172)

91038ba

WizKnight changed the title ~~Update constants import to use module-level access (#1172)~~ Update constants import to use module-level access #1172 Aug 15, 2024

Wauplin self-requested a review August 16, 2024 11:03

Wauplin approved these changes Aug 16, 2024

View reviewed changes

Wauplin reviewed Aug 16, 2024

View reviewed changes

Update constants import to use module-level access huggingface#1172 h…

e782532

…uggingface#2453

Wauplin reviewed Aug 19, 2024

View reviewed changes

WizKnight added 3 commits August 20, 2024 04:00

Update constants imports with backward compatibility huggingface#1172, …

5bb271c

…huggingface#2453

Update fastai_utils.py

4773244

Update hf_api.py

9ba7b2d

This comment was marked as resolved.

Sign in to view

remove unecessary changes

62af016

Wauplin merged commit 1b9517d into huggingface:main Aug 20, 2024
12 of 14 checks passed

Wauplin mentioned this pull request Aug 20, 2024

Import constants module instead of importing constant values directly #1172

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

WizKnight commented Aug 15, 2024 •

edited

Loading

Wauplin left a comment

Wauplin commented Aug 16, 2024

HuggingFaceDocBuilderDev commented Aug 16, 2024

Wauplin Aug 16, 2024 •

edited

Loading

WizKnight Aug 17, 2024 •

edited

Loading

WizKnight Aug 17, 2024 •

edited

Loading

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024

Wauplin left a comment

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024 •

edited

Loading

Wauplin Aug 19, 2024

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024

Wauplin Aug 19, 2024

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024 •

edited

Loading

Wauplin Aug 19, 2024

WizKnight Aug 19, 2024

Wauplin Aug 19, 2024

This comment was marked as resolved.

Wauplin commented Aug 20, 2024 •

edited

Loading

		# Assuming CommitOperationBase is the common base class
		CommitOperation = Union[CommitOperationAdd, CommitOperationDelete]

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

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

Conversation

WizKnight commented Aug 15, 2024 • edited Loading

Wauplin left a comment

Choose a reason for hiding this comment

Wauplin commented Aug 16, 2024

HuggingFaceDocBuilderDev commented Aug 16, 2024

Wauplin Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

WizKnight Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

WizKnight Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WizKnight Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WizKnight Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Wauplin commented Aug 20, 2024 • edited Loading

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

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

WizKnight commented Aug 15, 2024 •

edited

Loading

Wauplin Aug 16, 2024 •

edited

Loading

WizKnight Aug 17, 2024 •

edited

Loading

WizKnight Aug 17, 2024 •

edited

Loading

WizKnight Aug 19, 2024 •

edited

Loading

WizKnight Aug 19, 2024 •

edited

Loading

Wauplin commented Aug 20, 2024 •

edited

Loading