chore(generator): use absolute paths in __lazy_modules__#17614
Conversation
0fea5b8 to
46cf01b
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces support for PEP 0810 explicit lazy imports in generated packages, adds shared mTLS utility functions for the KMS service, and implements a Python SDK import profiling tool with documentation. Feedback on the changes highlights a redundant import of tracemalloc and a platform compatibility issue with the default CPU pinning value in profiler.py, as well as an unused import and several unused variables in rewrite_init.py.
| import tracemalloc | ||
| tracemalloc.start() |
There was a problem hiding this comment.
The tracemalloc module is already imported at the module level (line 6). Importing it again inside _mprofile_worker is redundant and violates the rule against importing modules inside functions when they are already imported at the module level.
| import tracemalloc | |
| tracemalloc.start() | |
| tracemalloc.start() |
References
- Do not import modules or classes inside functions if they are already imported at the module level.
| {_format_stats("Physical RSS RAM (MB)", rss_memories, p50_rss, p90_rss, p99_rss, ".4f")}""" | ||
| print(final_output.strip()) | ||
|
|
||
| def run_master(iterations, target_module, cpu=0, csv_path=None, clear_cache=True): |
There was a problem hiding this comment.
Hardcoding cpu=0 as the default value in run_master will cause failures on non-Linux platforms (like macOS or Windows) if the function is called programmatically without overriding cpu, because it will attempt to use taskset. Using NO_CPU_PINNING as the default value ensures platform-independent behavior by default.
| def run_master(iterations, target_module, cpu=0, csv_path=None, clear_cache=True): | |
| def run_master(iterations, target_module, cpu=NO_CPU_PINNING, csv_path=None, clear_cache=True): |
| @@ -0,0 +1,126 @@ | |||
| import re | |||
| import os | |||
There was a problem hiding this comment.
The os module is imported but never used in this script. It should be removed to keep the imports clean.
| import_pattern = re.compile(r'^from\s+(\.[a-zA-Z0-9_.]+)\s+import\s+(.*?)$', re.MULTILINE | re.DOTALL) | ||
|
|
||
| imports = [] |
There was a problem hiding this comment.
The variables import_pattern and imports are defined but never used anywhere in the script. Removing them keeps the code clean and maintainable.
| new_lines = [] | ||
|
|
||
| in_import = False | ||
| import_module = "" |
There was a problem hiding this comment.
The variables new_lines, in_import, and import_module are defined but never used in the script. Removing them improves code readability and maintainability.
46cf01b to
7b7a8a1
Compare
| # For more information, see: | ||
| # https://docs.python.org/3.15/library/sys.html#sys.set_lazy_imports_filter | ||
| # Older Python versions safely ignore this variable. | ||
| # We use absolute paths (via package_path) rather than __name__ to avoid runtime evaluation overhead. |
_lazy_modules
_lazy_modules__lazy_modules__
d217d25 to
13c40f3
Compare
| "google.cloud.asset_v1.types.asset_enrichment_resourceowners", | ||
| "google.cloud.asset_v1.types.asset_service", | ||
| "google.cloud.asset_v1.types.assets", | ||
| } |
There was a problem hiding this comment.
small nit, but it seems like these strings should probably be indented
9bde644 to
1888d4b
Compare
The GAPIC generator template now uses the {{package_path}} variable instead of evaluating name at runtime. The generator will now output the literal absolute strings (e.g., "google.cloud.compute_v1.services.accelerator_types") directly into the lazy_modules set, keeping the execution as lightweight as possible.