From 8b334bf3c726920991a32644be5899648ec3f08b Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 09:35:52 +0200 Subject: [PATCH 01/11] Add autocomplete + tests + type checking --- .github/workflows/python-quality.yml | 28 ++- setup.py | 8 +- src/huggingface_hub/__init__.py | 355 +++++++++++++++++---------- tests/conftest.py | 21 ++ tests/test_init_lazy_loading.py | 120 +++++++++ 5 files changed, 393 insertions(+), 139 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/test_init_lazy_loading.py diff --git a/.github/workflows/python-quality.yml b/.github/workflows/python-quality.yml index 1ab5714905..2e27390ffd 100644 --- a/.github/workflows/python-quality.yml +++ b/.github/workflows/python-quality.yml @@ -18,15 +18,19 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - name: Set up Python - uses: actions/setup-python@v2 - with: - python-version: 3.9 - - name: Install dependencies - run: | - pip install --upgrade pip - pip install .[dev] - - run: black --check tests src - - run: isort --check-only tests src - - run: flake8 tests src + - uses: actions/checkout@v2 + - name: Set up Python + uses: actions/setup-python@v2 + with: + python-version: 3.9 + - name: Install dependencies + run: | + pip install --upgrade pip + pip install .[dev] + - run: black --check tests src + - run: isort --check-only tests src + - run: flake8 tests src + + # Run type checking at least on huggingface_hub root file to check all modules + # that can be lazy-loaded actually exist. + - run: mypy src/huggingface_hub/__init__.py diff --git a/setup.py b/setup.py index e310583370..7e49fb0f32 100644 --- a/setup.py +++ b/setup.py @@ -36,18 +36,20 @@ def get_version() -> str: extras["tensorflow"] = ["tensorflow", "pydot", "graphviz"] extras["testing"] = [ + "datasets", + "jedi", + "Jinja2", "pytest", "pytest-cov", - "datasets", "soundfile", - "Jinja2", ] extras["quality"] = [ "black==22.3", - "isort>=5.5.4", "flake8>=3.8.3", "flake8-bugbear", + "isort>=5.5.4", + "mypy", ] extras["all"] = extras["testing"] + extras["quality"] diff --git a/src/huggingface_hub/__init__.py b/src/huggingface_hub/__init__.py index 55b9ff72a5..e05ab9764b 100644 --- a/src/huggingface_hub/__init__.py +++ b/src/huggingface_hub/__init__.py @@ -1,7 +1,3 @@ -# flake8: noqa -# There's no way to ignore "F401 '...' imported but unused" warnings in this -# module, but to preserve other warnings. So, don't check this module at all. - # Copyright 2020 The HuggingFace Team. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -17,18 +13,141 @@ # limitations under the License. # *********** -# vendored from https://github.com/scientific-python/lazy_loader +# `huggingface_hub` init has 2 modes: +# - Normal usage: +# If imported to use it, all modules and functions are lazy-loaded. This means +# they exist at top level in module but are imported only the first time they are +# used. This way, `from huggingface_hub import something` will import `something` +# quickly without the hassle of importing all the features from `huggingface_hub`. +# - Static check: +# If statically analyzed, all modules and functions are loaded normally. This way +# static typing check works properly as well as autocomplete in text editors and +# IDEs. +# +# The static model imports are done inside the `if TYPE_CHECKING:` statement at +# the bottom of this file. Since module/functions imports are duplicated, it is +# mandatory to make sure to add them twice when adding one. This is tested in +# `./tests/test_init_lazy_loading.py` module. +# +# To update the static imports, please run the following command and commit the changes. +# ``` +# pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file +# ``` +# +# *********** +# Lazy loader vendored from https://github.com/scientific-python/lazy_loader import importlib -import importlib.util -import inspect import os import sys -import types -import warnings +from typing import TYPE_CHECKING + +__version__ = "0.10.0.dev0" -class _LazyImportWarning(Warning): - pass +_SUBMOD_ATTRS = { + "commands.user": ["notebook_login"], + "constants": [ + "CONFIG_NAME", + "FLAX_WEIGHTS_NAME", + "HUGGINGFACE_CO_URL_HOME", + "HUGGINGFACE_CO_URL_TEMPLATE", + "PYTORCH_WEIGHTS_NAME", + "REPO_TYPE_DATASET", + "REPO_TYPE_MODEL", + "REPO_TYPE_SPACE", + "TF2_WEIGHTS_NAME", + "TF_WEIGHTS_NAME", + ], + "fastai_utils": [ + "_save_pretrained_fastai", + "from_pretrained_fastai", + "push_to_hub_fastai", + ], + "file_download": [ + "cached_download", + "hf_hub_download", + "hf_hub_url", + "try_to_load_from_cache", + ], + "hf_api": [ + "CommitOperation", + "CommitOperationAdd", + "CommitOperationDelete", + "DatasetSearchArguments", + "HfApi", + "HfFolder", + "ModelSearchArguments", + "change_discussion_status", + "comment_discussion", + "create_commit", + "create_discussion", + "create_pull_request", + "create_repo", + "dataset_info", + "delete_file", + "delete_repo", + "edit_discussion_comment", + "get_dataset_tags", + "get_discussion_details", + "get_full_repo_name", + "get_model_tags", + "get_repo_discussions", + "list_datasets", + "list_metrics", + "list_models", + "list_repo_files", + "merge_pull_request", + "model_info", + "move_repo", + "rename_discussion", + "repo_type_and_id_from_hf_id", + "set_access_token", + "space_info", + "unset_access_token", + "update_repo_visibility", + "upload_file", + "upload_folder", + "whoami", + ], + "hub_mixin": ["ModelHubMixin", "PyTorchModelHubMixin"], + "inference_api": ["InferenceApi"], + "keras_mixin": [ + "KerasModelHubMixin", + "from_pretrained_keras", + "push_to_hub_keras", + "save_pretrained_keras", + ], + "repository": ["Repository"], + "_snapshot_download": ["snapshot_download"], + "utils": [ + "logging", + "CachedFileInfo", + "CachedRepoInfo", + "CachedRevisionInfo", + "CorruptedCacheException", + "HFCacheInfo", + "scan_cache_dir", + ], + "utils.endpoint_helpers": ["DatasetFilter", "ModelFilter"], + "repocard": [ + "metadata_eval_result", + "metadata_load", + "metadata_save", + "metadata_update", + "ModelCard", + "DatasetCard", + ], + "community": [ + "Discussion", + "DiscussionWithDetails", + "DiscussionEvent", + "DiscussionComment", + "DiscussionStatusChange", + "DiscussionCommit", + "DiscussionTitleChange", + ], + "repocard_data": ["CardData", "ModelCardData", "DatasetCardData", "EvalResult"], +} def _attach(package_name, submodules=None, submod_attrs=None): @@ -114,118 +233,106 @@ def __dir__(): return __getattr__, __dir__, list(__all__) -# ************ - -__version__ = "0.10.0.dev0" - - __getattr__, __dir__, __all__ = _attach( - __name__, - submodules=[], - submod_attrs={ - "commands.user": ["notebook_login"], - "constants": [ - "CONFIG_NAME", - "FLAX_WEIGHTS_NAME", - "HUGGINGFACE_CO_URL_HOME", - "HUGGINGFACE_CO_URL_TEMPLATE", - "PYTORCH_WEIGHTS_NAME", - "REPO_TYPE_DATASET", - "REPO_TYPE_MODEL", - "REPO_TYPE_SPACE", - "TF2_WEIGHTS_NAME", - "TF_WEIGHTS_NAME", - ], - "fastai_utils": [ - "_save_pretrained_fastai", - "from_pretrained_fastai", - "push_to_hub_fastai", - ], - "file_download": [ - "cached_download", - "hf_hub_download", - "hf_hub_url", - "try_to_load_from_cache", - ], - "hf_api": [ - "CommitOperation", - "CommitOperationAdd", - "CommitOperationDelete", - "DatasetSearchArguments", - "HfApi", - "HfFolder", - "ModelSearchArguments", - "change_discussion_status", - "comment_discussion", - "create_commit", - "create_discussion", - "create_pull_request", - "create_repo", - "dataset_info", - "delete_file", - "delete_repo", - "edit_discussion_comment", - "get_dataset_tags", - "get_discussion_details", - "get_full_repo_name", - "get_model_tags", - "get_repo_discussions", - "list_datasets", - "list_metrics", - "list_models", - "list_repo_files", - "login", - "logout", - "merge_pull_request", - "model_info", - "move_repo", - "rename_discussion", - "repo_type_and_id_from_hf_id", - "set_access_token", - "space_info", - "unset_access_token", - "update_repo_visibility", - "upload_file", - "upload_folder", - "whoami", - ], - "hub_mixin": ["ModelHubMixin", "PyTorchModelHubMixin"], - "inference_api": ["InferenceApi"], - "keras_mixin": [ - "KerasModelHubMixin", - "from_pretrained_keras", - "push_to_hub_keras", - "save_pretrained_keras", - ], - "repository": ["Repository"], - "_snapshot_download": ["snapshot_download"], - "utils": [ - "logging", - "CachedFileInfo", - "CachedRepoInfo", - "CachedRevisionInfo", - "CorruptedCacheException", - "HFCacheInfo", - "scan_cache_dir", - ], - "utils.endpoint_helpers": ["DatasetFilter", "ModelFilter"], - "repocard": [ - "metadata_eval_result", - "metadata_load", - "metadata_save", - "metadata_update", - "ModelCard", - "DatasetCard", - ], - "community": [ - "Discussion", - "DiscussionWithDetails", - "DiscussionEvent", - "DiscussionComment", - "DiscussionStatusChange", - "DiscussionCommit", - "DiscussionTitleChange", - ], - "repocard_data": ["CardData", "ModelCardData", "DatasetCardData", "EvalResult"], - }, + __name__, submodules=[], submod_attrs=_SUBMOD_ATTRS ) + +# WARNING: any content below this statement is generated automatically. Any manual edit +# will be lost when re-generating this file ! +# +# To update the static imports, please run the following command and commit the changes. +# ``` +# pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file +# ``` +if TYPE_CHECKING: + from ._snapshot_download import snapshot_download # noqa: F401 + from .commands.user import notebook_login # noqa: F401 + from .community import Discussion # noqa: F401 + from .community import DiscussionComment # noqa: F401 + from .community import DiscussionCommit # noqa: F401 + from .community import DiscussionEvent # noqa: F401 + from .community import DiscussionStatusChange # noqa: F401 + from .community import DiscussionTitleChange # noqa: F401 + from .community import DiscussionWithDetails # noqa: F401 + from .constants import CONFIG_NAME # noqa: F401 + from .constants import FLAX_WEIGHTS_NAME # noqa: F401 + from .constants import HUGGINGFACE_CO_URL_HOME # noqa: F401 + from .constants import HUGGINGFACE_CO_URL_TEMPLATE # noqa: F401 + from .constants import PYTORCH_WEIGHTS_NAME # noqa: F401 + from .constants import REPO_TYPE_DATASET # noqa: F401 + from .constants import REPO_TYPE_MODEL # noqa: F401 + from .constants import REPO_TYPE_SPACE # noqa: F401 + from .constants import TF2_WEIGHTS_NAME # noqa: F401 + from .constants import TF_WEIGHTS_NAME # noqa: F401 + from .fastai_utils import _save_pretrained_fastai # noqa: F401 + from .fastai_utils import from_pretrained_fastai # noqa: F401 + from .fastai_utils import push_to_hub_fastai # noqa: F401 + from .file_download import cached_download # noqa: F401 + from .file_download import hf_hub_download # noqa: F401 + from .file_download import hf_hub_url # noqa: F401 + from .file_download import try_to_load_from_cache # noqa: F401 + from .hf_api import CommitOperation # noqa: F401 + from .hf_api import CommitOperationAdd # noqa: F401 + from .hf_api import CommitOperationDelete # noqa: F401 + from .hf_api import DatasetSearchArguments # noqa: F401 + from .hf_api import HfApi # noqa: F401 + from .hf_api import HfFolder # noqa: F401 + from .hf_api import ModelSearchArguments # noqa: F401 + from .hf_api import change_discussion_status # noqa: F401 + from .hf_api import comment_discussion # noqa: F401 + from .hf_api import create_commit # noqa: F401 + from .hf_api import create_discussion # noqa: F401 + from .hf_api import create_pull_request # noqa: F401 + from .hf_api import create_repo # noqa: F401 + from .hf_api import dataset_info # noqa: F401 + from .hf_api import delete_file # noqa: F401 + from .hf_api import delete_repo # noqa: F401 + from .hf_api import edit_discussion_comment # noqa: F401 + from .hf_api import get_dataset_tags # noqa: F401 + from .hf_api import get_discussion_details # noqa: F401 + from .hf_api import get_full_repo_name # noqa: F401 + from .hf_api import get_model_tags # noqa: F401 + from .hf_api import get_repo_discussions # noqa: F401 + from .hf_api import list_datasets # noqa: F401 + from .hf_api import list_metrics # noqa: F401 + from .hf_api import list_models # noqa: F401 + from .hf_api import list_repo_files # noqa: F401 + from .hf_api import merge_pull_request # noqa: F401 + from .hf_api import model_info # noqa: F401 + from .hf_api import move_repo # noqa: F401 + from .hf_api import rename_discussion # noqa: F401 + from .hf_api import repo_type_and_id_from_hf_id # noqa: F401 + from .hf_api import set_access_token # noqa: F401 + from .hf_api import space_info # noqa: F401 + from .hf_api import unset_access_token # noqa: F401 + from .hf_api import update_repo_visibility # noqa: F401 + from .hf_api import upload_file # noqa: F401 + from .hf_api import upload_folder # noqa: F401 + from .hf_api import whoami # noqa: F401 + from .hub_mixin import ModelHubMixin # noqa: F401 + from .hub_mixin import PyTorchModelHubMixin # noqa: F401 + from .inference_api import InferenceApi # noqa: F401 + from .keras_mixin import KerasModelHubMixin # noqa: F401 + from .keras_mixin import from_pretrained_keras # noqa: F401 + from .keras_mixin import push_to_hub_keras # noqa: F401 + from .keras_mixin import save_pretrained_keras # noqa: F401 + from .repocard import DatasetCard # noqa: F401 + from .repocard import ModelCard # noqa: F401 + from .repocard import metadata_eval_result # noqa: F401 + from .repocard import metadata_load # noqa: F401 + from .repocard import metadata_save # noqa: F401 + from .repocard import metadata_update # noqa: F401 + from .repocard_data import CardData # noqa: F401 + from .repocard_data import DatasetCardData # noqa: F401 + from .repocard_data import EvalResult # noqa: F401 + from .repocard_data import ModelCardData # noqa: F401 + from .repository import Repository # noqa: F401 + from .utils import CachedFileInfo # noqa: F401 + from .utils import CachedRepoInfo # noqa: F401 + from .utils import CachedRevisionInfo # noqa: F401 + from .utils import CorruptedCacheException # noqa: F401 + from .utils import HFCacheInfo # noqa: F401 + from .utils import logging # noqa: F401 + from .utils import scan_cache_dir # noqa: F401 + from .utils.endpoint_helpers import DatasetFilter # noqa: F401 + from .utils.endpoint_helpers import ModelFilter # noqa: F401 diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000000..cdfe2ec5fc --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,21 @@ +from pytest import Parser + + +def pytest_addoption(parser: Parser) -> None: + """Add option to update `huggingface_hub.__init__.py` with new imports. + + If the init file is updated, the test itself will fail. + See `./tests/test_init_lazy_loading.py` for more details. + + Run the following command to update static imports: + ``` + pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file + ``` + """ + parser.addoption( + "--update-init-file", + action="store_true", + help=( + "If True, the root `__init__` file will be updated with new sorted imports." + ), + ) diff --git a/tests/test_init_lazy_loading.py b/tests/test_init_lazy_loading.py new file mode 100644 index 0000000000..671d6e37d4 --- /dev/null +++ b/tests/test_init_lazy_loading.py @@ -0,0 +1,120 @@ +import unittest +from pathlib import Path + +import pytest + +import huggingface_hub +import isort +import jedi +from huggingface_hub import _SUBMOD_ATTRS # which modules/functions are available ? + + +IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING:\n" +SETUP_CFG_PATH = Path(__file__).parent.parent / "setup.cfg" + + +@pytest.fixture +def update_init_file(request): + request.cls.update_init_file = request.config.getoption("--update-init-file") + + +@pytest.mark.usefixtures("update_init_file") +class TestHuggingfaceHubInit(unittest.TestCase): + update_init_file: bool + + def test_static_imports(self) -> None: + """Test all imports are made twice (1 in lazy-loading and 1 in static checks). + + For more explanations, see `./src/huggingface_hub/__init__.py`. + Run the following command to update static imports. + ``` + pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file + ``` + """ + init_path = Path(huggingface_hub.__file__) + with init_path.open() as f: + init_content = f.read() + + init_content_before_static_checks = init_content.split(IF_TYPE_CHECKING_LINE)[0] + + static_imports = [ + f" from .{module} import {attr} # noqa: F401" + for module, attributes in _SUBMOD_ATTRS.items() + for attr in attributes + ] + + expected_init_content_raw = ( + init_content_before_static_checks + + IF_TYPE_CHECKING_LINE + + "\n".join(static_imports) + + "\n" + ) + + expected_init_content_cleaned = isort.code( + expected_init_content_raw, config=isort.Config(settings_path=SETUP_CFG_PATH) + ) + + if init_content != expected_init_content_cleaned: + if self.update_init_file: + with init_path.open("w") as f: + f.write(expected_init_content_cleaned) + + self.fail( + "Pytest was run with '--update-init-file' option and" + " `./src/huggingface_hub/__init__.py` content has been updated. It" + " is most likely that you added a module/function to" + " `_SUBMOD_ATTRS` and did not update the 'static import'-part." + " Please make sure the changes are accurate and if yes, commit them" + " and re-run this test without the '--update-init-file' option." + ) + else: + self.fail( + "Expected content mismatch in `./src/huggingface_hub/__init__.py`." + " It is most likely that you added a module/function to" + " `_SUBMOD_ATTRS` and did not update the 'static import'-part. To" + " do it, please re-run the test suite with '--update-init-file'" + " option and look at the changes. If the changes are accurate," + " commit them and re-run this test without the '--update-init-file'" + " option." + ) + + self.assertEqual(init_content, expected_init_content_cleaned) + + def test_autocomplete_on_root_imports(self) -> None: + """Test autocomplete with `huggingface_hub` works with Jedi. + + Not all autocomplete systems are based on Jedi but if this one works we can + assume others does it as well. + """ + source = """from huggingface_hub import c""" + script = jedi.Script(source, path="example.py") + completions = script.complete(1, len(source)) + + for completion in completions: + if completion.name == "create_commit": + # Assert `create_commit` is suggestion from `huggingface_hub` lib + self.assertEquals(completion.module_name, "huggingface_hub") + + # Assert autocomplete knows where `create_commit` lives + # It would not be the case with a dynamic import. + goto_list = completion.goto() + self.assertEquals(len(goto_list), 1) + + # Assert docstring is find. This means autocomplete can also provide + # the help section. + signature_list = goto_list[0].get_signatures() + self.assertEquals(len(signature_list), 1) + self.assertTrue( + signature_list[0] + .docstring() + .startswith("create_commit(self, repo_id: str") + ) + break + else: + self.fail( + "Jedi autocomplete did not suggest `create_commit` to complete the" + f" line `{source}`. It is most probable that static imports are not" + " correct in `./src/huggingface_hub/__init__.py`. Please run `pytest" + " tests/test_init_lazy_loading.py -k test_static_imports" + " --update-init-file` to fix this." + ) From c544fbce830fbe915c212947a84767157e6965ec Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 09:59:53 +0200 Subject: [PATCH 02/11] add isort as a dev dependency --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index 7e49fb0f32..15dbda7a98 100644 --- a/setup.py +++ b/setup.py @@ -37,6 +37,7 @@ def get_version() -> str: extras["testing"] = [ "datasets", + "isort>=5.5.4", "jedi", "Jinja2", "pytest", From 010fd776f48a3cdf3b8d62d4ad5375a86d5b74a1 Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 10:28:45 +0200 Subject: [PATCH 03/11] also reorder attributes definition for readability --- src/huggingface_hub/__init__.py | 70 ++++++++++++++++++++++----------- tests/test_init_lazy_loading.py | 61 ++++++++++++++++++++++------ 2 files changed, 94 insertions(+), 37 deletions(-) diff --git a/src/huggingface_hub/__init__.py b/src/huggingface_hub/__init__.py index e05ab9764b..7799736fa2 100644 --- a/src/huggingface_hub/__init__.py +++ b/src/huggingface_hub/__init__.py @@ -44,8 +44,25 @@ __version__ = "0.10.0.dev0" +# Alphabetical order of definitions is ensured in tests +# WARNING: any comment added in this dictionary definition will be lost when +# re-generating the file ! _SUBMOD_ATTRS = { - "commands.user": ["notebook_login"], + "_snapshot_download": [ + "snapshot_download", + ], + "commands.user": [ + "notebook_login", + ], + "community": [ + "Discussion", + "DiscussionComment", + "DiscussionCommit", + "DiscussionEvent", + "DiscussionStatusChange", + "DiscussionTitleChange", + "DiscussionWithDetails", + ], "constants": [ "CONFIG_NAME", "FLAX_WEIGHTS_NAME", @@ -109,44 +126,49 @@ "upload_folder", "whoami", ], - "hub_mixin": ["ModelHubMixin", "PyTorchModelHubMixin"], - "inference_api": ["InferenceApi"], + "hub_mixin": [ + "ModelHubMixin", + "PyTorchModelHubMixin", + ], + "inference_api": [ + "InferenceApi", + ], "keras_mixin": [ "KerasModelHubMixin", "from_pretrained_keras", "push_to_hub_keras", "save_pretrained_keras", ], - "repository": ["Repository"], - "_snapshot_download": ["snapshot_download"], + "repocard": [ + "DatasetCard", + "ModelCard", + "metadata_eval_result", + "metadata_load", + "metadata_save", + "metadata_update", + ], + "repocard_data": [ + "CardData", + "DatasetCardData", + "EvalResult", + "ModelCardData", + ], + "repository": [ + "Repository", + ], "utils": [ - "logging", "CachedFileInfo", "CachedRepoInfo", "CachedRevisionInfo", "CorruptedCacheException", "HFCacheInfo", + "logging", "scan_cache_dir", ], - "utils.endpoint_helpers": ["DatasetFilter", "ModelFilter"], - "repocard": [ - "metadata_eval_result", - "metadata_load", - "metadata_save", - "metadata_update", - "ModelCard", - "DatasetCard", - ], - "community": [ - "Discussion", - "DiscussionWithDetails", - "DiscussionEvent", - "DiscussionComment", - "DiscussionStatusChange", - "DiscussionCommit", - "DiscussionTitleChange", + "utils.endpoint_helpers": [ + "DatasetFilter", + "ModelFilter", ], - "repocard_data": ["CardData", "ModelCardData", "DatasetCardData", "EvalResult"], } diff --git a/tests/test_init_lazy_loading.py b/tests/test_init_lazy_loading.py index 671d6e37d4..ffd013ed7a 100644 --- a/tests/test_init_lazy_loading.py +++ b/tests/test_init_lazy_loading.py @@ -1,3 +1,4 @@ +import re import unittest from pathlib import Path @@ -11,6 +12,7 @@ IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING:\n" SETUP_CFG_PATH = Path(__file__).parent.parent / "setup.cfg" +SUBMOD_ATTRS_PATTERN = re.compile("_SUBMOD_ATTRS = {[^}]+}") # match the all dict @pytest.fixture @@ -35,29 +37,60 @@ def test_static_imports(self) -> None: with init_path.open() as f: init_content = f.read() + # Get first half of the `__init__.py` file. + # WARNING: Content after this part will be entirely re-generated which means + # human-edited changes will be lost ! init_content_before_static_checks = init_content.split(IF_TYPE_CHECKING_LINE)[0] + # Search and replace `_SUBMOD_ATTRS` dictionary definition. This ensures modules + # and functions that can be lazy-loaded are alphabetically ordered for readability. + self.assertIsNotNone( + SUBMOD_ATTRS_PATTERN.search(init_content_before_static_checks), + "_SUBMOD_ATTRS dictionary definition not found in" + " `./src/huggingface_hub/__init__.py`.", + ) + _submod_attrs_definition = ( + "_SUBMOD_ATTRS = {" + + "\n" + + "\n".join( + f' "{module}": [' + + "\n" + + "\n".join( + f' "{attr}",' for attr in sorted(_SUBMOD_ATTRS[module]) + ) + + "\n" + + " ]," + for module in sorted(_SUBMOD_ATTRS.keys()) + ) + + "\n" + + "}" + ) + reordered_content_before_static_checks = SUBMOD_ATTRS_PATTERN.sub( + _submod_attrs_definition, init_content_before_static_checks + ) + + # Generate the static imports given the `_SUBMOD_ATTRS` dictionary. static_imports = [ f" from .{module} import {attr} # noqa: F401" for module, attributes in _SUBMOD_ATTRS.items() for attr in attributes ] - expected_init_content_raw = ( - init_content_before_static_checks + # Generate the expected `__init__.py` file content and apply formatter on it. + expected_init_content = isort.code( + reordered_content_before_static_checks + IF_TYPE_CHECKING_LINE + "\n".join(static_imports) - + "\n" - ) - - expected_init_content_cleaned = isort.code( - expected_init_content_raw, config=isort.Config(settings_path=SETUP_CFG_PATH) + + "\n", + config=isort.Config(settings_path=SETUP_CFG_PATH), ) - if init_content != expected_init_content_cleaned: + # If expected `__init__.py` content is different, test fails. If '--update-init-file' + # is used, `__init__.py` file is updated before the test fails. + if init_content != expected_init_content: if self.update_init_file: with init_path.open("w") as f: - f.write(expected_init_content_cleaned) + f.write(expected_init_content) self.fail( "Pytest was run with '--update-init-file' option and" @@ -73,12 +106,14 @@ def test_static_imports(self) -> None: " It is most likely that you added a module/function to" " `_SUBMOD_ATTRS` and did not update the 'static import'-part. To" " do it, please re-run the test suite with '--update-init-file'" - " option and look at the changes. If the changes are accurate," - " commit them and re-run this test without the '--update-init-file'" - " option." + " option: `pytest tests/test_init_lazy_loading.py -k" + " test_static_imports --update-init-file`. Look at the changes and" + " if accurate, commit them and re-run this test without the" + " '--update-init-file' option." ) - self.assertEqual(init_content, expected_init_content_cleaned) + # Should never fail but let's check it just in case + self.assertEqual(init_content, expected_init_content) def test_autocomplete_on_root_imports(self) -> None: """Test autocomplete with `huggingface_hub` works with Jedi. From f2ab5618aa45ff547167b8484afac29ba8427ffb Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 10:33:13 +0200 Subject: [PATCH 04/11] add types for mypy --- setup.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.py b/setup.py index 15dbda7a98..5d722d0040 100644 --- a/setup.py +++ b/setup.py @@ -51,6 +51,8 @@ def get_version() -> str: "flake8-bugbear", "isort>=5.5.4", "mypy", + "types-requests", + "types-urllib3", ] extras["all"] = extras["testing"] + extras["quality"] From 8ccbb603afd0ef0cbe250882150a2cdd36c1a142 Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 10:33:57 +0200 Subject: [PATCH 05/11] add simplejson types --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index 5d722d0040..c6670fb584 100644 --- a/setup.py +++ b/setup.py @@ -52,6 +52,7 @@ def get_version() -> str: "isort>=5.5.4", "mypy", "types-requests", + "types-simplejson", "types-urllib3", ] From 55c2e7a6629221d6718243c9c430464dad04659c Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 10:56:40 +0200 Subject: [PATCH 06/11] do not raise on follow imports with mypy in CI --- .github/workflows/python-quality.yml | 2 +- setup.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/python-quality.yml b/.github/workflows/python-quality.yml index 2e27390ffd..565252bdfd 100644 --- a/.github/workflows/python-quality.yml +++ b/.github/workflows/python-quality.yml @@ -33,4 +33,4 @@ jobs: # Run type checking at least on huggingface_hub root file to check all modules # that can be lazy-loaded actually exist. - - run: mypy src/huggingface_hub/__init__.py + - run: mypy src/huggingface_hub/__init__.py --follow-imports=silent diff --git a/setup.py b/setup.py index c6670fb584..77502d1869 100644 --- a/setup.py +++ b/setup.py @@ -51,8 +51,10 @@ def get_version() -> str: "flake8-bugbear", "isort>=5.5.4", "mypy", + "types-PyYAML", "types-requests", "types-simplejson", + "types-toml", "types-urllib3", ] From 8aeea16debb3bebc82af7449332580cdc841f40c Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 13:53:30 +0200 Subject: [PATCH 07/11] remove unused mypy plugins --- setup.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/setup.py b/setup.py index 77502d1869..15dbda7a98 100644 --- a/setup.py +++ b/setup.py @@ -51,11 +51,6 @@ def get_version() -> str: "flake8-bugbear", "isort>=5.5.4", "mypy", - "types-PyYAML", - "types-requests", - "types-simplejson", - "types-toml", - "types-urllib3", ] extras["all"] = extras["testing"] + extras["quality"] From c4893a1e46e5fee04ef176b91c55869a93f68f4a Mon Sep 17 00:00:00 2001 From: Wauplin Date: Thu, 8 Sep 2022 14:09:01 +0200 Subject: [PATCH 08/11] more readable code generation ? --- tests/test_init_lazy_loading.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/test_init_lazy_loading.py b/tests/test_init_lazy_loading.py index ffd013ed7a..09ed15d2a3 100644 --- a/tests/test_init_lazy_loading.py +++ b/tests/test_init_lazy_loading.py @@ -50,20 +50,16 @@ def test_static_imports(self) -> None: " `./src/huggingface_hub/__init__.py`.", ) _submod_attrs_definition = ( - "_SUBMOD_ATTRS = {" - + "\n" + "_SUBMOD_ATTRS = {\n" + "\n".join( - f' "{module}": [' - + "\n" + f' "{module}": [\n' + "\n".join( f' "{attr}",' for attr in sorted(_SUBMOD_ATTRS[module]) ) - + "\n" - + " ]," + + "\n ]," for module in sorted(_SUBMOD_ATTRS.keys()) ) - + "\n" - + "}" + + "\n}" ) reordered_content_before_static_checks = SUBMOD_ATTRS_PATTERN.sub( _submod_attrs_definition, init_content_before_static_checks From dbef227991de9dd4664879b92ff5342a2597c906 Mon Sep 17 00:00:00 2001 From: Wauplin Date: Fri, 9 Sep 2022 09:55:48 +0200 Subject: [PATCH 09/11] Move static import formatter to a separate util and add it to makefile --- Makefile | 4 +- src/huggingface_hub/__init__.py | 16 +++-- tests/conftest.py | 21 ------ tests/test_init_lazy_loading.py | 114 +---------------------------- utils/check_static_imports.py | 123 ++++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 137 deletions(-) delete mode 100644 tests/conftest.py create mode 100644 utils/check_static_imports.py diff --git a/Makefile b/Makefile index 97d3274367..c0d55320a6 100644 --- a/Makefile +++ b/Makefile @@ -1,17 +1,19 @@ .PHONY: quality style test -check_dirs := tests src setup.py +check_dirs := tests src utils setup.py quality: black --check $(check_dirs) isort --check-only $(check_dirs) flake8 $(check_dirs) + python utils/check_static_imports.py style: black $(check_dirs) isort $(check_dirs) + python utils/check_static_imports.py --update-file test: HUGGINGFACE_CO_STAGING=1 pytest -sv ./tests/ diff --git a/src/huggingface_hub/__init__.py b/src/huggingface_hub/__init__.py index 7799736fa2..a8481e236c 100644 --- a/src/huggingface_hub/__init__.py +++ b/src/huggingface_hub/__init__.py @@ -26,12 +26,16 @@ # # The static model imports are done inside the `if TYPE_CHECKING:` statement at # the bottom of this file. Since module/functions imports are duplicated, it is -# mandatory to make sure to add them twice when adding one. This is tested in -# `./tests/test_init_lazy_loading.py` module. +# mandatory to make sure to add them twice when adding one. This is checked in the +# `make quality` command. # # To update the static imports, please run the following command and commit the changes. # ``` -# pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file +# # Use script +# python utils/check_static_imports.py --update-file +# +# # Or run style on codebase +# make style # ``` # # *********** @@ -264,7 +268,11 @@ def __dir__(): # # To update the static imports, please run the following command and commit the changes. # ``` -# pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file +# # Use script +# python utils/check_static_imports.py --update-file +# +# # Or run style on codebase +# make style # ``` if TYPE_CHECKING: from ._snapshot_download import snapshot_download # noqa: F401 diff --git a/tests/conftest.py b/tests/conftest.py deleted file mode 100644 index cdfe2ec5fc..0000000000 --- a/tests/conftest.py +++ /dev/null @@ -1,21 +0,0 @@ -from pytest import Parser - - -def pytest_addoption(parser: Parser) -> None: - """Add option to update `huggingface_hub.__init__.py` with new imports. - - If the init file is updated, the test itself will fail. - See `./tests/test_init_lazy_loading.py` for more details. - - Run the following command to update static imports: - ``` - pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file - ``` - """ - parser.addoption( - "--update-init-file", - action="store_true", - help=( - "If True, the root `__init__` file will be updated with new sorted imports." - ), - ) diff --git a/tests/test_init_lazy_loading.py b/tests/test_init_lazy_loading.py index 09ed15d2a3..54b747e479 100644 --- a/tests/test_init_lazy_loading.py +++ b/tests/test_init_lazy_loading.py @@ -1,121 +1,14 @@ -import re import unittest -from pathlib import Path -import pytest - -import huggingface_hub -import isort import jedi -from huggingface_hub import _SUBMOD_ATTRS # which modules/functions are available ? - - -IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING:\n" -SETUP_CFG_PATH = Path(__file__).parent.parent / "setup.cfg" -SUBMOD_ATTRS_PATTERN = re.compile("_SUBMOD_ATTRS = {[^}]+}") # match the all dict - - -@pytest.fixture -def update_init_file(request): - request.cls.update_init_file = request.config.getoption("--update-init-file") -@pytest.mark.usefixtures("update_init_file") class TestHuggingfaceHubInit(unittest.TestCase): - update_init_file: bool - - def test_static_imports(self) -> None: - """Test all imports are made twice (1 in lazy-loading and 1 in static checks). - - For more explanations, see `./src/huggingface_hub/__init__.py`. - Run the following command to update static imports. - ``` - pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file - ``` - """ - init_path = Path(huggingface_hub.__file__) - with init_path.open() as f: - init_content = f.read() - - # Get first half of the `__init__.py` file. - # WARNING: Content after this part will be entirely re-generated which means - # human-edited changes will be lost ! - init_content_before_static_checks = init_content.split(IF_TYPE_CHECKING_LINE)[0] - - # Search and replace `_SUBMOD_ATTRS` dictionary definition. This ensures modules - # and functions that can be lazy-loaded are alphabetically ordered for readability. - self.assertIsNotNone( - SUBMOD_ATTRS_PATTERN.search(init_content_before_static_checks), - "_SUBMOD_ATTRS dictionary definition not found in" - " `./src/huggingface_hub/__init__.py`.", - ) - _submod_attrs_definition = ( - "_SUBMOD_ATTRS = {\n" - + "\n".join( - f' "{module}": [\n' - + "\n".join( - f' "{attr}",' for attr in sorted(_SUBMOD_ATTRS[module]) - ) - + "\n ]," - for module in sorted(_SUBMOD_ATTRS.keys()) - ) - + "\n}" - ) - reordered_content_before_static_checks = SUBMOD_ATTRS_PATTERN.sub( - _submod_attrs_definition, init_content_before_static_checks - ) - - # Generate the static imports given the `_SUBMOD_ATTRS` dictionary. - static_imports = [ - f" from .{module} import {attr} # noqa: F401" - for module, attributes in _SUBMOD_ATTRS.items() - for attr in attributes - ] - - # Generate the expected `__init__.py` file content and apply formatter on it. - expected_init_content = isort.code( - reordered_content_before_static_checks - + IF_TYPE_CHECKING_LINE - + "\n".join(static_imports) - + "\n", - config=isort.Config(settings_path=SETUP_CFG_PATH), - ) - - # If expected `__init__.py` content is different, test fails. If '--update-init-file' - # is used, `__init__.py` file is updated before the test fails. - if init_content != expected_init_content: - if self.update_init_file: - with init_path.open("w") as f: - f.write(expected_init_content) - - self.fail( - "Pytest was run with '--update-init-file' option and" - " `./src/huggingface_hub/__init__.py` content has been updated. It" - " is most likely that you added a module/function to" - " `_SUBMOD_ATTRS` and did not update the 'static import'-part." - " Please make sure the changes are accurate and if yes, commit them" - " and re-run this test without the '--update-init-file' option." - ) - else: - self.fail( - "Expected content mismatch in `./src/huggingface_hub/__init__.py`." - " It is most likely that you added a module/function to" - " `_SUBMOD_ATTRS` and did not update the 'static import'-part. To" - " do it, please re-run the test suite with '--update-init-file'" - " option: `pytest tests/test_init_lazy_loading.py -k" - " test_static_imports --update-init-file`. Look at the changes and" - " if accurate, commit them and re-run this test without the" - " '--update-init-file' option." - ) - - # Should never fail but let's check it just in case - self.assertEqual(init_content, expected_init_content) - def test_autocomplete_on_root_imports(self) -> None: """Test autocomplete with `huggingface_hub` works with Jedi. Not all autocomplete systems are based on Jedi but if this one works we can - assume others does it as well. + assume others do as well. """ source = """from huggingface_hub import c""" script = jedi.Script(source, path="example.py") @@ -145,7 +38,6 @@ def test_autocomplete_on_root_imports(self) -> None: self.fail( "Jedi autocomplete did not suggest `create_commit` to complete the" f" line `{source}`. It is most probable that static imports are not" - " correct in `./src/huggingface_hub/__init__.py`. Please run `pytest" - " tests/test_init_lazy_loading.py -k test_static_imports" - " --update-init-file` to fix this." + " correct in `./src/huggingface_hub/__init__.py`. Please run `make" + " style` to fix this." ) diff --git a/utils/check_static_imports.py b/utils/check_static_imports.py new file mode 100644 index 0000000000..0a9453cea0 --- /dev/null +++ b/utils/check_static_imports.py @@ -0,0 +1,123 @@ +# coding=utf-8 +# Copyright 2022-present, the HuggingFace Inc. team. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Contains a tool to reformat static imports in `huggingface_hub.__init__.py`.""" +import argparse +import re +from pathlib import Path +from typing import NoReturn + +import isort +from huggingface_hub import _SUBMOD_ATTRS + + +INIT_FILE_PATH = Path(__file__).parents[1] / "src" / "huggingface_hub" / "__init__.py" +SETUP_CFG_PATH = Path(__file__).parents[1] / "setup.cfg" + +IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING:\n" +SUBMOD_ATTRS_PATTERN = re.compile("_SUBMOD_ATTRS = {[^}]+}") # match the all dict + + +def check_static_imports(update_file: bool) -> NoReturn: + """Check all imports are made twice (1 in lazy-loading and 1 in static checks). + + For more explanations, see `./src/huggingface_hub/__init__.py`. + This script is used in the `make style` and `make quality` checks. + """ + with INIT_FILE_PATH.open() as f: + init_content = f.read() + + # Get first half of the `__init__.py` file. + # WARNING: Content after this part will be entirely re-generated which means + # human-edited changes will be lost ! + init_content_before_static_checks = init_content.split(IF_TYPE_CHECKING_LINE)[0] + + # Search and replace `_SUBMOD_ATTRS` dictionary definition. This ensures modules + # and functions that can be lazy-loaded are alphabetically ordered for readability. + if SUBMOD_ATTRS_PATTERN.search(init_content_before_static_checks) is None: + print( + "Error: _SUBMOD_ATTRS dictionary definition not found in" + " `./src/huggingface_hub/__init__.py`." + ) + exit(1) + + _submod_attrs_definition = ( + "_SUBMOD_ATTRS = {\n" + + "\n".join( + f' "{module}": [\n' + + "\n".join(f' "{attr}",' for attr in sorted(_SUBMOD_ATTRS[module])) + + "\n ]," + for module in sorted(_SUBMOD_ATTRS.keys()) + ) + + "\n}" + ) + reordered_content_before_static_checks = SUBMOD_ATTRS_PATTERN.sub( + _submod_attrs_definition, init_content_before_static_checks + ) + + # Generate the static imports given the `_SUBMOD_ATTRS` dictionary. + static_imports = [ + f" from .{module} import {attr} # noqa: F401" + for module, attributes in _SUBMOD_ATTRS.items() + for attr in attributes + ] + + # Generate the expected `__init__.py` file content and apply formatter on it. + expected_init_content = isort.code( + reordered_content_before_static_checks + + IF_TYPE_CHECKING_LINE + + "\n".join(static_imports) + + "\n", + config=isort.Config(settings_path=SETUP_CFG_PATH), + ) + + # If expected `__init__.py` content is different, test fails. If '--update-init-file' + # is used, `__init__.py` file is updated before the test fails. + if init_content != expected_init_content: + if update_file: + with INIT_FILE_PATH.open("w") as f: + f.write(expected_init_content) + + print( + "✅ Imports have been updated in `./src/huggingface_hub/__init__.py`." + "\n Please make sure the changes are accurate and commit them." + ) + exit(0) + else: + print( + "❌ Expected content mismatch in" + " `./src/huggingface_hub/__init__.py`.\n It is most likely that you" + " added a module/function to `_SUBMOD_ATTRS` and did not update the" + " 'static import'-part.\n Please run `make style` or `python" + " utils/check_static_imports.py --update-file`." + ) + exit(1) + + print("✅ All good!") + exit(0) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + parser.add_argument( + "--update-file", + action="store_true", + help=( + "Whether to fix `./src/huggingface_hub/__init__.py` if a change is" + " detected." + ), + ) + args = parser.parse_args() + + check_static_imports(update_file=args.update_file) From dfe88bfb4e32f679fc9536cb520b949f595e2a3e Mon Sep 17 00:00:00 2001 From: Wauplin Date: Fri, 9 Sep 2022 10:04:06 +0200 Subject: [PATCH 10/11] add check to workflow --- .github/workflows/python-quality.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/python-quality.yml b/.github/workflows/python-quality.yml index 565252bdfd..b185f78c74 100644 --- a/.github/workflows/python-quality.yml +++ b/.github/workflows/python-quality.yml @@ -30,6 +30,7 @@ jobs: - run: black --check tests src - run: isort --check-only tests src - run: flake8 tests src + - run: python utils/check_static_imports.py # Run type checking at least on huggingface_hub root file to check all modules # that can be lazy-loaded actually exist. From ed15e8c133071f1386db654d9cb627eaca80cadf Mon Sep 17 00:00:00 2001 From: Wauplin Date: Fri, 9 Sep 2022 10:07:42 +0200 Subject: [PATCH 11/11] pragma no cover in init.py --- src/huggingface_hub/__init__.py | 2 +- utils/check_static_imports.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/huggingface_hub/__init__.py b/src/huggingface_hub/__init__.py index 4804f2ca47..a5c12f2b82 100644 --- a/src/huggingface_hub/__init__.py +++ b/src/huggingface_hub/__init__.py @@ -275,7 +275,7 @@ def __dir__(): # # Or run style on codebase # make style # ``` -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no cover from ._snapshot_download import snapshot_download # noqa: F401 from .commands.user import notebook_login # noqa: F401 from .community import Discussion # noqa: F401 diff --git a/utils/check_static_imports.py b/utils/check_static_imports.py index 0a9453cea0..5107ad47d5 100644 --- a/utils/check_static_imports.py +++ b/utils/check_static_imports.py @@ -25,7 +25,7 @@ INIT_FILE_PATH = Path(__file__).parents[1] / "src" / "huggingface_hub" / "__init__.py" SETUP_CFG_PATH = Path(__file__).parents[1] / "setup.cfg" -IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING:\n" +IF_TYPE_CHECKING_LINE = "\nif TYPE_CHECKING: # pragma: no cover\n" SUBMOD_ATTRS_PATTERN = re.compile("_SUBMOD_ATTRS = {[^}]+}") # match the all dict