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

feat(gazelle): Include types/stubs packages #2425

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ewianda
Copy link
Contributor

@ewianda ewianda commented Nov 18, 2024

This PR adds logic that checks if a package has a corresponding types or stubs package and automatically adds that to the BUILD file. This is useful for typeckers e.g pyright , mypy

@aignas
Copy link
Collaborator

aignas commented Nov 18, 2024

Thank you @ewianda for the contribution. @dougthor42, would you have time to look at this?

Copy link
Contributor

@dougthor42 dougthor42 left a comment

Choose a reason for hiding this comment

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

I'll probably be able to test this out sometime this week.

Until then, here are some review comments. Also, please add a flag to turn this on or off. It should be off by default for now, and then in a couple releases we can consider making it on by default.

gazelle/python/resolve.go Outdated Show resolved Hide resolved
gazelle/python/resolve.go Outdated Show resolved Hide resolved
gazelle/modules_mapping/test_generator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dougthor42 dougthor42 left a comment

Choose a reason for hiding this comment

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

Please add documentation to gazelle/README.md


Running with the default include_stub_packages = False didn't cause any problems with our large project running gazelle, so 👍 there.

Running with include_stub_packages = True didn't add any *_types or _stubs deps to targets 👎.

Is there something else I need to do? We already have some type/stub packages as part of our requirements.in (and thus requirements.txt and gazelle_python.yaml). For example, we have sqlmypy: sqlalchemy_stubs and annotated_types: annotated_types in gazelle_python.yaml

Test methodology

  1. bazel run //:buildozer 'remove deps' '//src/pyle_xc/...:*'
  2. bazel run //:gazelle src/pyle_xc
  3. git diff and see what has changed.

gazelle/modules_mapping/generator.py Outdated Show resolved Hide resolved
self.stderr = stderr
self.output_file = output_file
self.excluded_patterns = [re.compile(pattern) for pattern in excluded_patterns]
self.include_stub_packages = include_stub_packages
self.mapping = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a more descriptive variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved the variable from a class attribute to an instance attribute to address a bug caused by shared state across instances of the Generator class during test.

gazelle/modules_mapping/generator.py Outdated Show resolved Hide resolved
gazelle/python/resolve.go Outdated Show resolved Hide resolved
@ewianda
Copy link
Contributor Author

ewianda commented Nov 23, 2024

Please add documentation to gazelle/README.md

Running with the default include_stub_packages = False didn't cause any problems with our large project running gazelle, so 👍 there.

Running with include_stub_packages = True didn't add any *_types or _stubs deps to targets 👎.

Is there something else I need to do? We already have some type/stub packages as part of our requirements.in (and thus requirements.txt and gazelle_python.yaml). For example, we have sqlmypy: sqlalchemy_stubs and annotated_types: annotated_types in gazelle_python.yaml

Test methodology

  1. bazel run //:buildozer 'remove deps' '//src/pyle_xc/...:*'
  2. bazel run //:gazelle src/pyle_xc
  3. git diff and see what has changed.

Thanks, @dougthor42, for testing. I think you missed bazel run //:gazelle_python_manifest.update to add the types to the manifest.

Did you fix 631ab19
locally before running the test?

Also, the logic is that sqlalchemy has to be a direct dependency before sqlalchemy-stubs will be added to the BUILD file. annotated-types doesn't fall under this category as well.

@dougthor42
Copy link
Contributor

Did you fix 631ab19 locally before running the test?

Ah, no I did not. I applied that change and ran gazelle_python_manifest.update. Here's the full diff of gazelle_python.yaml:

diff --git a/gazelle_python.yaml b/gazelle_python.yaml
index 5752a8274a..66e8ab5c4f 100644
--- a/gazelle_python.yaml
+++ b/gazelle_python.yaml
@@ -395,6 +395,7 @@ manifest:
     pymatching: PyMatching
     pymodbus: pymodbus
     pyparsing: pyparsing
+    pyqt5_stubs: pyqt5_stubs
     pytest: pytest
     pytest_asyncio: pytest_asyncio
     pytest_benchmark: pytest_benchmark
@@ -484,9 +485,8 @@ manifest:
     sphinxcontrib.svgbob: sphinxcontrib_svgbob
     sqlalchemy: SQLAlchemy
     sqlalchemy_bigquery: sqlalchemy_bigquery
-    sqlmypy: sqlalchemy_stubs
+    sqlalchemy_stubs: sqlalchemy_stubs
     sqlparse: sqlparse
-    sqltyping: sqlalchemy_stubs
     stack_data: stack_data
     stim: stim
     stimcirq: stimcirq

So it's is correctly pulling in new stubs (like pyqt5_stubs) 👍 but also changing names a bit 👎. This 2nd part is an issue if user code ends up trying to import the original name (eg sqlmypy) directly: gazelle will fail saying "sqlmypy" is an invalid dependency.

Luckily our code doesn't import sqlmypy or sqltyping directly, so I didn't run into that error and was able to move on and run gazelle. Here's a partial diff:

diff --git a/src/labrad/servers/GUIs/ADR/BUILD.bazel b/src/labrad/servers/GUIs/ADR/BUILD.bazel
index 0667587998..b8e716cc30 100644
--- a/src/labrad/servers/GUIs/ADR/BUILD.bazel
+++ b/src/labrad/servers/GUIs/ADR/BUILD.bazel
@@ -11,5 +11,6 @@ py_library(
         "//src/pyle/datavault:util",
         "@pypi//duet",
         "@pypi//pyqt5",
+        "@pypi//pyqt5_stubs",
     ],
 )
diff --git a/src/pyle/cloud/bigquery/scripts/BUILD.bazel b/src/pyle/cloud/bigquery/scripts/BUILD.bazel
index 0d10c6b62d..f86c846852 100644
--- a/src/pyle/cloud/bigquery/scripts/BUILD.bazel
+++ b/src/pyle/cloud/bigquery/scripts/BUILD.bazel
@@ -86,6 +86,7 @@ pyle_py_binary(
         "@pypi//google_cloud_bigquery",
         "@pypi//pandas",
         "@pypi//sqlalchemy",
+        "@pypi//sqlalchemy_stubs",
     ],
 )

So overall it looks like it's WAI 👍 🎉 🎊

@ewianda
Copy link
Contributor Author

ewianda commented Nov 25, 2024

This 2nd part is an issue if user code ends up trying to import the original name (eg sqlmypy) directly: gazelle will fail saying "sqlmypy" is an invalid dependency.

These are stub files, I will be surprised if folks are importing this directly since they are meant for type-checking tools, how ever we can let dig_wheel proceed to add package names.

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.

4 participants