[bzlmod] Re-enable bzlmod for python tests.#41713
Conversation
725df4b to
13d2271
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request re-enables bzlmod for Python tests by making several adjustments to the build configuration. The changes include setting a hermetic Python 3.11 as the default, renaming the typing_extensions dependency to avoid naming conflicts, adding a Bazel flag to handle implicit namespace packages correctly, and upgrading Python protobuf dependencies. The modifications appear to be consistent and well-targeted to resolve the bzlmod integration issues. I have one suggestion to improve the clarity of a comment regarding the protobuf version pinning.
| # | ||
| # See also https://github.com/bazel-contrib/rules_python/issues/55 | ||
| # | ||
| # Error message: ModuleNotFoundError: No module named 'src.python' |
There was a problem hiding this comment.
Thank you. This is very useful.
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| suites.append(loader.loadTestsFromModule(module)) | ||
| # Look in the current working directory for the test file physically |
There was a problem hiding this comment.
@asheshvidyut we two other places with SingleLoader and similar-ish logic:
- bazel/_gevent_test_main.py
- bazel/_single_module_tester.py
Do we need to fix them too?
- If yes, let's explain why this bazel/_run_time_type_check_main.py is special in this comment.
- If no, can we try the package discovery logic from them and see if it works here?
I'd like keeping all three SingleLoader implementation as close as possible to each other, so at some point refactor them so we have a single common implementation. That's I want us to keep track of these slight differences between them.
There was a problem hiding this comment.
@sergiitk Great question.
No, we can't reuse the package discovery logic from _gevent_test_main.py here as-is.
Here is why
_gevent_test_main.py relies on file copying, The py_grpc_gevent_test macro (defined in bazel/gevent_test.bzl) uses a genrule to copy _gevent_test_main.py into the same package directory as the target test before actually running it.
https://github.com/grpc/grpc/blob/master/bazel/gevent_test.bzl#L63
Because the script ends up executing from within the target test's directory, os.path successfully resolves to that very folder and safely discovers the test modules.
_run_time_type_check_main.py is executed in-place: The py_grpc_run_time_type_check_test macro (defined in bazel/run_time_type_check_test.bzl) executes _run_time_type_check_main.py
directly from the //bazel: package instead of copying it. This is why we have to crawl the Bazel execution root explicitly using os.getcwd().
_single_module_tester.py uses command-line arguments. It avoids both of these workarounds by forcing the caller to explicitly pass the target test directory as a command-line argument
I agree with the goal of unifying all three SingleLoader implementations.
Moving all of them to the explicit command-line argument approach—like _single_module_tester.py would be the cleanest and better.
But can we take this as a followup refactoring?
There was a problem hiding this comment.
Yes, sounds good. Thanks for looking into this.
There was a problem hiding this comment.
Added code suggestion with a TODO item linking this thread: #41713 (comment)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request re-enables bzlmod for Python tests by making several adjustments to the build configuration. The changes include renaming the typing_extensions repository to grpc_typing_extensions to resolve naming conflicts, updating Python dependencies like protobuf, and adding a Bazel flag to handle implicit workspace packages under bzlmod. The logic for discovering test modules in _run_time_type_check_main.py has also been updated.
My review identifies a potential improvement in the test discovery logic to make it more robust against multiple test files with the same name. Overall, the changes are well-explained and seem to correctly address the issues with enabling bzlmod.
| filepath = next(cwd.rglob(target_file), None) | ||
| if not filepath: | ||
| raise ValueError(f"Could not find target module {target_module}") |
There was a problem hiding this comment.
Using next() with rglob will silently select the first file if multiple files with the same name exist across different directories. This could lead to running an unintended test and mask potential configuration issues. The previous implementation would have failed an assertion in such a case.
It would be safer to find all matching files and raise an error if the result is ambiguous (i.e., more than one file is found).
| filepath = next(cwd.rglob(target_file), None) | |
| if not filepath: | |
| raise ValueError(f"Could not find target module {target_module}") | |
| filepaths = list(cwd.rglob(target_file)) | |
| if not filepaths: | |
| raise ValueError(f"Could not find target module {target_module}") | |
| if len(filepaths) > 1: | |
| raise ValueError( | |
| f"Found multiple files for target module {target_module}: {filepaths}" | |
| ) | |
| filepath = filepaths[0] |
Internal bug: https://b.corp.google.com/issues/489982254 Culprit: grpc#41747 Example failure: https://btx.cloud.google.com/invocations/086c9dbf-61e8-403f-8814-bf96b6d7dcde Proposed solution for root cause: grpc#41713 Closes grpc#41810 COPYBARA_INTEGRATE_REVIEW=grpc#41810 from yuanweiz:fix-psm b917de6 PiperOrigin-RevId: 881477012
Some changes required: * Use hermetic python 3.11 in bzlmod setup (same as WORKSPACE). * Repo `typing_extensions` needs to be renamed and have its `__init__.py` deleted, in order for it to work consistently in both WORKSPACE and bzlmod. this is because bzlmod mangles the name while WORKSPACE doesn't. (`@@_main~_repo_rules~typing_extensions` vs `typing_extensions`), this change should make python unambiguously read `src/typing_extensions.py`. * Use --incompatible_default_to_explicit_init_py in bzlmod python tests. This is a long-standing known [issue](bazel-contrib/rules_python#55) in bazel. It's unclear though why it doesn't break WORKSPACE setup. * python protobuf dependencies upgraded to 6.31.x to fix "major version mismatch" issue. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41713 PiperOrigin-RevId: 883336745
Previous change to disable bzlmod for these tests #41810 (e1e1d0a) #41713 is now in master and we're running most python linux/mac bazel tests with bzlmod via inclusion of `tools/remote_build/include/enable_bzlmod.bazelrc`. However that file isn't used by PSM tests so the python specific flag isn't properly applied. Solution is to use `tools/bazel.rc` for global config and in the long run delete `{enable,disable}_bzlmod.bazelrc`. Adhoc run passed (see #41871 (comment)) <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes #41871 COPYBARA_INTEGRATE_REVIEW=#41871 from yuanweiz:fix-psm c073998 PiperOrigin-RevId: 888726822
Internal bug: https://b.corp.google.com/issues/489982254 Culprit: grpc#41747 Example failure: https://btx.cloud.google.com/invocations/086c9dbf-61e8-403f-8814-bf96b6d7dcde Proposed solution for root cause: grpc#41713 Closes grpc#41810 COPYBARA_INTEGRATE_REVIEW=grpc#41810 from yuanweiz:fix-psm b917de6 PiperOrigin-RevId: 881477012
Some changes required: * Use hermetic python 3.11 in bzlmod setup (same as WORKSPACE). * Repo `typing_extensions` needs to be renamed and have its `__init__.py` deleted, in order for it to work consistently in both WORKSPACE and bzlmod. this is because bzlmod mangles the name while WORKSPACE doesn't. (`@@_main~_repo_rules~typing_extensions` vs `typing_extensions`), this change should make python unambiguously read `src/typing_extensions.py`. * Use --incompatible_default_to_explicit_init_py in bzlmod python tests. This is a long-standing known [issue](bazel-contrib/rules_python#55) in bazel. It's unclear though why it doesn't break WORKSPACE setup. * python protobuf dependencies upgraded to 6.31.x to fix "major version mismatch" issue. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41713 PiperOrigin-RevId: 883336745
Previous change to disable bzlmod for these tests grpc#41810 (e1e1d0a) grpc#41713 is now in master and we're running most python linux/mac bazel tests with bzlmod via inclusion of `tools/remote_build/include/enable_bzlmod.bazelrc`. However that file isn't used by PSM tests so the python specific flag isn't properly applied. Solution is to use `tools/bazel.rc` for global config and in the long run delete `{enable,disable}_bzlmod.bazelrc`. Adhoc run passed (see grpc#41871 (comment)) <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41871 COPYBARA_INTEGRATE_REVIEW=grpc#41871 from yuanweiz:fix-psm c073998 PiperOrigin-RevId: 888726822
Previous change to disable bzlmod for these tests grpc#41810 (e1e1d0a) grpc#41713 is now in master and we're running most python linux/mac bazel tests with bzlmod via inclusion of `tools/remote_build/include/enable_bzlmod.bazelrc`. However that file isn't used by PSM tests so the python specific flag isn't properly applied. Solution is to use `tools/bazel.rc` for global config and in the long run delete `{enable,disable}_bzlmod.bazelrc`. Adhoc run passed (see grpc#41871 (comment)) <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41871 COPYBARA_INTEGRATE_REVIEW=grpc#41871 from yuanweiz:fix-psm c073998 PiperOrigin-RevId: 888726822
Master renamed the typing_extensions repo to grpc_typing_extensions in PR grpc#41713 to fix bzlmod naming consistency. Update the aio and tests_aio/unit deps to use the new label.
Master renamed the typing_extensions repo to grpc_typing_extensions in PR grpc#41713 to fix bzlmod naming consistency. Update the aio and tests_aio/unit deps to use the new label.
Some changes required: * Use hermetic python 3.11 in bzlmod setup (same as WORKSPACE). * Repo `typing_extensions` needs to be renamed and have its `__init__.py` deleted, in order for it to work consistently in both WORKSPACE and bzlmod. this is because bzlmod mangles the name while WORKSPACE doesn't. (`@@_main~_repo_rules~typing_extensions` vs `typing_extensions`), this change should make python unambiguously read `src/typing_extensions.py`. * Use --incompatible_default_to_explicit_init_py in bzlmod python tests. This is a long-standing known [issue](bazel-contrib/rules_python#55) in bazel. It's unclear though why it doesn't break WORKSPACE setup. * python protobuf dependencies upgraded to 6.31.x to fix "major version mismatch" issue. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41713 PiperOrigin-RevId: 883336745
Previous change to disable bzlmod for these tests grpc#41810 (e1e1d0a) grpc#41713 is now in master and we're running most python linux/mac bazel tests with bzlmod via inclusion of `tools/remote_build/include/enable_bzlmod.bazelrc`. However that file isn't used by PSM tests so the python specific flag isn't properly applied. Solution is to use `tools/bazel.rc` for global config and in the long run delete `{enable,disable}_bzlmod.bazelrc`. Adhoc run passed (see grpc#41871 (comment)) <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. --> Closes grpc#41871 COPYBARA_INTEGRATE_REVIEW=grpc#41871 from yuanweiz:fix-psm c073998 PiperOrigin-RevId: 888726822
Some changes required:
typing_extensionsneeds to be renamed and have its__init__.pydeleted, in order for it to work consistently in both WORKSPACE and bzlmod. this is because bzlmod mangles the name while WORKSPACE doesn't. (@@_main~_repo_rules~typing_extensionsvstyping_extensions), this change should make python unambiguously readsrc/typing_extensions.py.