Smarter namespace pkg detection in pip_install (issue #381) #483

thundergolfer · 2021-06-08T08:20:58Z

PR Checklist

Please check if your PR fulfills the following requirements:

Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: #381

(Issue contains a lot of detail so is worth reading for background)

What is the new behavior?

Packages like ciso8601 which previously weren't handled correctly (see #381) now work correctly. Previously pip_install took any non-empty directory without __init__.py to be a namespace package, which is wrong. A namespace package is a directory that either includes Python module files as immediate children, or is the parent directory of a directory which is a package (may be namespace or regular).

Demo: https://github.com/thundergolfer-playground/rules_python-issue-381

Does this PR introduce a breaking change?

Yes
No

…pkgs

thundergolfer · 2021-06-08T08:21:35Z

python/pip_install/extract_wheels/lib/namespace_pkgs_test.py

+        actual = namespace_pkgs.implicit_namespace_packages(directory.root())
+        self.assertEqual(actual, expected)
+
+    def test_skips_ignored_directories(self):


Test case is not really related to these changes, but should have been added before.

hrfuller

LGTM thanks.

hrfuller · 2021-06-09T20:36:30Z

python/pip_install/extract_wheels/lib/namespace_pkgs.py

+        dir_includes_py_modules = _includes_python_modules(filenames)
+        parent_of_namespace_pkg = any(str(pathlib.Path(dirpath, d)) in namespace_pkg_dirs for d in dirnames)
+        parent_of_standard_pkg = any(str(pathlib.Path(dirpath, d)) in standard_pkg_dirs for d in dirnames)
+        parent_of_pkg = parent_of_namespace_pkg or parent_of_standard_pkg


nit: if this is the only use of this variable it seems cleaner to put it in the conditional where it is used.

ssiano · 2021-07-02T03:17:56Z

@thundergolfer, the azure namespace packages were working for me with rules_python 0.2.0, but quit working with rules_python 0.3.0, which includes this PR.

I am not sure if that is the expected behavior, so I'd like to report the results here instead of creating a new issue.

In short, the __init__.py that contains

__path__ = __import__('pkgutil').extend_path(__path__, __name__)

is no longer being added to:

$(bazel info output_base)/external/python3_deps_pypi__azure_core/azure/
$(bazel info output_base)/external/python3_deps_pypi__azure_storage_blob/azure/
$(bazel info output_base)/external/python3_deps_pypi__azure_storage_blob/azure/storage/

Thus bazel test :test_mlflow_import (see attached files) fails with

azure: /home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_
mlflow_import.runfiles/python3_deps_pypi__azure_core/azure/__init__.py
Traceback (most recent call last):
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python_namespace_test/test_mlflow_import.py", line 10, in <module>
    import mlflow
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test
_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/__init__.py", line 34, in <module>
    import mlflow.tracking._model_registry.fluent
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/tracking/__init__.py", line 8, in <module>
    from mlflow.tracking.client import MlflowClient
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test
_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/tracking/client.py", line 25, in <module>
    from mlflow.tracking._tracking_service.client import TrackingServiceClient
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/tracking/_tracking_service/client.py", line 22, in <module>
    from mlflow.store.artifact.artifact_repository_registry import get_artifact_repository
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/store/artifact/artifact_repository_registry.py", line 6, in <module>
    from mlflow.store.artifact.dbfs_artifact_repo import dbfs_artifact_repo_factory
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d
[python_namespace_package.zip](https://github.com/bazelbuild/rules_python/files/6751846/python_namespace_package.zip)
/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/store/artifact/dbfs_artifact_repo.py", line 10, in <module>
    from mlflow.store.artifact.databricks_artifact_repo import DatabricksArtifactRepository
  File "/home/ssiano/.cache/bazel/_bazel_ssiano/650b0d5eefd734d4c566f5a6fe2ac49d/sandbox/linux-sandbox/9/execroot/python_namespace_test/bazel-out/k8-fastbuild/bin/test_mlflow_import.runfiles/python3_deps_pypi__mlflow/mlflow/store/artifact/databricks_artifact_repo.py", line 9, in <module>
    from azure.storage.blob import BlobClient
ModuleNotFoundError: No module named 'azure.storage'

I've attached python_namespace_package.zip which contains five files (BUILD, requirements.in, requirements.txt, test_mlflow_import.py, and WORKSPACE) for reproducing this issue. You can use commenting to switch them between rules_python 0.2.0 and 0.3.0.

Please let me know if rules_python 0.3.0 is behaving as expected, or if I should create a new issue.

ssiano · 2021-07-02T03:20:58Z

python_namespace_package.zip

thundergolfer · 2021-07-06T01:25:43Z

Thanks for this @ssiano. Should be easy for me to add a test case and see if the new logic fails with this package structure.

Can I quickly check if you're using pip_parse or pip_install? The former may very well have been hit with a regression in this area. See #499

ssiano · 2021-07-06T16:38:05Z

@thundergolfer, I was using pip_parse.

Thanks for pointing out #499. I checked pip_install with rules_python 0.3.0, and it worked as expected, so this pip_parse issue does seem to be due to the same regression as reported in #499.

For the test case, you can start with mlflow==1.13.1 and setuptools==54.0.0 in requirements.in and use compile_pip_requirements to generate requirements.txt. For the py_test, import mlflow. The import will fail with ModuleNotFoundError: No module named 'azure.storage' when using pip_parse.

Jonathon Belotti added 6 commits June 6, 2021 22:06

implement _includes_python_modules to help properly detect namespace …

1176320

…pkgs

Resolve #381 (testcase additions included)

c361e78

add failing testcases for unhandled scenario

cc04407

handle case where ns-pkg is parent of regular pkg(s)

c04fdc1

minor addition to testcase

9a33262

Rename var

fffaa06

thundergolfer requested review from brandjon and lberki as code owners June 8, 2021 08:20

google-cla bot added the cla: yes label Jun 8, 2021

thundergolfer commented Jun 8, 2021

View reviewed changes

Jonathon Belotti added 2 commits June 9, 2021 09:06

Merge branch 'master' into jonathon--issue-381-may2021

5aae10f

Merge branch 'master' into jonathon--issue-381-may2021

c286069

thundergolfer requested a review from hrfuller June 8, 2021 23:48

hrfuller approved these changes Jun 9, 2021

View reviewed changes

Merge branch 'master' into jonathon--issue-381-may2021

fefef1f

thundergolfer merged commit 03c4523 into master Jun 16, 2021

thundergolfer deleted the jonathon--issue-381-may2021 branch June 16, 2021 04:25

ssiano mentioned this pull request Jul 9, 2021

Fix regression in pip parse for finding implicit namespace packages. #504

Merged

13 tasks

hajimes mentioned this pull request Aug 24, 2023

(Bazel) AttributeError: module 'mmh3' has no attribute 'hash' hajimes/mmh3#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Smarter namespace pkg detection in pip_install (issue #381) #483

Smarter namespace pkg detection in pip_install (issue #381) #483

Uh oh!

thundergolfer commented Jun 8, 2021

Uh oh!

thundergolfer Jun 8, 2021

Uh oh!

hrfuller left a comment

Uh oh!

hrfuller Jun 9, 2021

Uh oh!

ssiano commented Jul 2, 2021 •

edited

Loading

Uh oh!

ssiano commented Jul 2, 2021

Uh oh!

thundergolfer commented Jul 6, 2021

Uh oh!

ssiano commented Jul 6, 2021

Uh oh!

Uh oh!

Uh oh!

Smarter namespace pkg detection in pip_install (issue #381) #483

Smarter namespace pkg detection in pip_install (issue #381) #483

Uh oh!

Conversation

thundergolfer commented Jun 8, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

thundergolfer Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

hrfuller left a comment

Choose a reason for hiding this comment

Uh oh!

hrfuller Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

ssiano commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssiano commented Jul 2, 2021

Uh oh!

thundergolfer commented Jul 6, 2021

Uh oh!

ssiano commented Jul 6, 2021

Uh oh!

Uh oh!

ssiano commented Jul 2, 2021 •

edited

Loading