Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds first-party Python bytecode modes, compilation providers, runtime packaging, OCI image support, rules_python interoperability, and broad integration coverage for ChangesFirst-party bytecode support
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BazelTarget
participant PycAspect
participant PycCompiler
participant RuntimeLauncher
participant OCIImageLayer
BazelTarget->>PycAspect: collect dependency sources
PycAspect->>PycCompiler: compile compatible Python sources
PycCompiler->>RuntimeLauncher: provide PycInfo artifacts
RuntimeLauncher->>OCIImageLayer: expose selected source or bytecode files
OCIImageLayer->>OCIImageLayer: validate mode and rewrite layers
Merge Risk: 🔵 Low · up to The bytecode feature is broadly mergeable, but the Starlark dependency metadata and protobuf pyc-only regression assertion should be corrected to keep release tooling and CI coverage accurate. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 40.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 50 files. (14 skipped: 12 unsupported, 2 over the file limit.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| layer: 0 | ||
| files: | ||
| - -rwxr-xr-x 0 0 0 42 Jan 1 2023 ./app.runfiles/_main/oci/py_image_layer/branding/__init__.py | ||
| - -rwxr-xr-x 0 0 0 276 Jan 1 2023 ./app.runfiles/_main/oci/py_image_layer/branding/__pycache__/__init__.cpython-311.pyc |
There was a problem hiding this comment.
pyc = "pyc": source .py files having .pyc in __pycache__
| --- | ||
| layer: 0 | ||
| files: | ||
| - -rwxr-xr-x 0 0 0 276 Jan 1 2023 ./app.runfiles/_main/oci/py_image_layer/branding/__init__.pyc |
There was a problem hiding this comment.
pyc = "pyc_only": no source .py files and only .pyc in its place
8552c7c to
b5dc5a4
Compare
Aspect Workflows Tasks📅 Mon Sep 21 21:27:27 UTC 2026 Task Results
⏱ Last updated Mon Sep 21 22:27:01 UTC 2026 · 📊 GitHub API quota 806/7,700 (10% used, resets in 20m) |
py_binary startup benchmark
sys.path quality
Bazel analysis benchmark
py_image_layer benchmark
|
b5dc5a4 to
c572f70
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
72b09f0 to
addd93c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
a939918 to
acba6e7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
87748f3 to
4a31404
Compare
4a31404 to
dbc5a45
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed the rewritten ed8f20b. Comparing both immutable archive trees directly, only README.md, docs/interpreter.md, and the Ruff target in pyproject.toml changed; the bytecode implementation and tests are byte-identical.
The new ruleset-wide Python 3.10 minimum and interpreter-policy clarification address the ambiguity behind my earlier Python 3.8/3.9 finding. I no longer classify inability to run the compiler on those versions as a supported-version correctness blocker. An older PBS interpreter can still be downloaded/configured and the unchanged compiler still fails with a generic annotation TypeError rather than a clear unsupported-version error; I consider that a nonblocking diagnostics follow-up under the clarified policy.
The previously reported cross-ruleset source-sharing action conflict remains: its entire implementation is unchanged. The previous supported Bazel 8.6 controls passed with either owner individually and on the merge base, but failed even in default source mode when the two owners were combined. I’m not repeating that or the other unchanged findings inline. I found no new issue in this three-file rewrite.
-zbarskybot
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed the five-file e312809 update. I found one new issue in the OCI coverage regression: it can pass on query/archive errors and can miss a forbidden source even in a valid archive. I reproduced those cases with the exact shell block and GNU tar; a short forbidden-source archive failed as expected, and a .pyc-only control passed. The existing image test can replace the new shell inspection; details are inline.
The image transition now explicitly disables coverage instrumentation for the packaged image, and the compiler now postpones annotation evaluation. The previously reported cross-ruleset source-sharing action conflict is not changed by moving compiler inputs to tools: this increment leaves the output declarations and foreign-output reuse unchanged. I’m not duplicating that finding. I did not run the actual OCI coverage target locally.
-zbarskybot
| name = "my_app_layers_fp_pyc", | ||
| binary = ":my_app_bin", | ||
| layer_tier = ":my_app_tier", | ||
| pyc = "pyc", |
There was a problem hiding this comment.
To me this attribute doesn't really tell me what happens when pyc = "pyc". perhaps precompile = "yes|no|auto|pyc_only"?
There was a problem hiding this comment.
WDYT about:
py_binary(bytecode = "none") # sources only
py_binary(bytecode = "cache") # sources plus __pycache__
py_binary(bytecode = "only") # bytecode, no sources
?
PEP 3147 uses "sourceless distribution" / "legacy pyc files" - so maybe the "sourceless" or "legacy" is better for that one?
Also blank still means "use default", normally meaning fallback which is whatever is configured in your .bazelrc. I prefer blank/None (depending on macro vs internal where it must have a value) instead of "default"
| ) | ||
| embedded_args, transformed_args = launcher.append_runfile( | ||
| file = main, | ||
| file = pyc.entrypoint, |
There was a problem hiding this comment.
shouldn't we fall back to main if pyc is disabled?
There was a problem hiding this comment.
I believe it does that already
| default = "", | ||
| values = [""] + PYC_MODES, | ||
| doc = """First-party bytecode packaging: `source` ships only `.py` | ||
| sources; `pyc` additionally ships PEP 3147 `__pycache__` bytecode; |
There was a problem hiding this comment.
did we consider always using the bytecode specific extension described in https://peps.python.org/pep-3147?
This may allow better cache invalidation and action result retention.
There was a problem hiding this comment.
The https://peps.python.org/pep-3147/#multiple-file-extensions section? That is under the "Alternatives" section of ideas that was rejected IIUC.
Is there another section in there I'm missing?
| def _platform_cfg_impl(settings, attr): | ||
| result = { | ||
| "//command_line_option:platforms": [attr.platform] if attr.platform else settings["//command_line_option:platforms"], | ||
| "//command_line_option:collect_code_coverage": False, |
There was a problem hiding this comment.
This is strange to me. usually bazel coverage does not lead to any artifact delivery except for cov files. why do we need to disable this?
There was a problem hiding this comment.
If we are running coverage then we need the original .py files to be shipped and not only .pyc files
| pyc_only_sources = [] | ||
| retained_source_paths = {} | ||
| retained_source_short_paths = {} | ||
| if effective_pyc == "pyc_only": |
There was a problem hiding this comment.
shouldn't these be utility functions exported from pyc .bzl file?
| def _py_python_pex_impl(ctx): | ||
| binary = ctx.attr.binary | ||
| binary = _single_target(ctx.attr.binary) | ||
| if PycModeInfo in binary and binary[PycModeInfo].mode != "source": |
There was a problem hiding this comment.
i don't why would every py_binary downstream rule needs to get a pyc attribute. if the a downstream does not want the pyc files, it can simply omit them when collecting .py files, that's what the providers are for.
pyc files are simply perf optimizations, so we shouldn't treat them as must be present or must not be, python interpreter will do the right thing when they are absent.
There was a problem hiding this comment.
I think this mess is because py_pex_binary is passed the py_binary which has converted all the PyInfo into plain DefaultInfo(executable, runfiles) and we've lost all our info. If that py_bianry did pyc_only then we lost all the sources.
That seems like either a bad py_pex_binary API, or maybe py_binary simply needs to export all its data in a PyInfo in case another rule wants to consume it instead of directly executing the the DefaultInfo(executable, runfiles)?
There was a problem hiding this comment.
py_binary exporting the full PyInfo would also allow py_image_layer to consume that data easier maybe?
e312809 to
62475ea
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed 62475ea, including the compiler/toolchain changes, runtime interop, and removal of the image-level pyc override. Three new findings below; earlier findings are not repeated.
Validation: the exact compiler test file on CPython 3.12 and 3.13, isolated UTF-8 transport controls, the exact archive checker with a missing-source counterexample, and current-head CI logs. I did not run the complete Bazel/container suite locally.
-zbarskybot
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Rechecked ba705e524b361725b77ce4adba59822cac482c89. The previous locale-fixture, retained-source assertion, and interpreter-snapshot findings are addressed; the 15 compiler tests pass on Python 3.12 and 3.13, and the archive controls distinguish missing source from bytecode. The remaining Bazel 8 container failure exposes one additional fixture mismatch below. I did not run a complete local Bazel/container suite.
-zbarskybot
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Rechecked fb79d328ab1968119c7661def67c2f3cfb788f8b as part of the requested review. The sourceless OCI command fixture is fixed, including an executed Bazel 8 CI pass. The updated pytest retention and unittest path handling introduce two collection/launch regressions, and the native bytecode reuse loop adds repeated transitive work; details below.
Validation includes paired real-pytest collection controls and the exact old/new unittest drivers through the native launcher. The analysis-cost finding follows from the provider traversal, not a measured timing regression. I did not run the complete local Bazel suite.
-zbarskybot
| entrypoint = main_entry.pyc, | ||
| info = info, | ||
| mode = mode, | ||
| venv_files = depset(ctx.files.srcs + [main], transitive = [info.sourceless_files]) if ctx.attr.retain_srcs else info.sourceless_files, |
There was a problem hiding this comment.
[P2] Preserve the package markers needed to collect retained tests. A valid target can put alpha/test_behavior.py and beta/test_behavior.py in srcs, with the two packages' __init__.py files owned by libraries in deps. This now retains the tests but leaves only __init__.pyc for their packages. Pytest's default package discovery checks for __init__.py, so it imports both tests as test_behavior and aborts with an import-file-mismatch error. A paired pytest 9.0.3 control passes both cases with the old source-retaining layout and fails collection with this layout. Retain the ancestor package markers required by collected tests without restoring all production dependency sources.
-zbarskybot
| here = os.path.abspath(__file__) | ||
| for candidate in (driver_path, driver_path[:-len(".py")] + ".pyc"): | ||
| if candidate and here.endswith(candidate): | ||
| return here[:-len(candidate)] |
There was a problem hiding this comment.
[P2] Resolve the test runfile instead of inferring a root from the driver's physical path. With manifest-only runfiles, the launcher resolves this generated driver under bazel-out, while a source test's manifest entry points into the source tree. Removing driver_path from __file__ therefore produces the generated-output prefix, and _import_test_modules searches there rather than at the mapped test file. A native-launcher control using the exact old/new drivers passes in source mode before this change and raises FileNotFoundError afterward; directory-runfiles controls pass. Manifest-only operation is already exercised by e2e/cases/venv-manifest-runfiles-1378/wrapper.sh. Resolve each source/bytecode candidate through the runfiles directory or manifest instead of this suffix heuristic.
-zbarskybot
fb79d32 to
7eda5d8
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Rechecked caedf11 against its current merge base as part of the requested review. One new cache-check failure is below. The native bytecode lookup no longer flattens the transitive bytecode set, but its replacement again trusts unadvertised .pyc action outputs (existing finding); that concern remains open. I have not duplicated the existing inline findings.
The compiler implementation is unchanged from the previous pass. Its 16 tests and focused one-shot/worker byte-identity and error-recovery controls pass on CPython 3.12. The CI failure below is reproduced in both Bazel 8 and 9 jobs for this revision; their synthetic merge contains the exact same test-script blob. No full local Bazel suite was run.
-zbarskybot
caedf11 to
f930148
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Rechecked f930148 as part of the requested review. The batching change has one generated-source ownership issue and one optional test-layout simplification below. Existing findings are not duplicated; the execution-log-format conflict still fails the Bazel 8 cases job on this revision.
Validation: the new batching fixture executes successfully in Bazel 8 and 9 CI on the verified synthetic merge. Focused controls exercise the exact action-construction functions with the real compiler, including mixed reused outputs, batch sizes, worker/one-shot transports, and failure/retry behavior. The protobuf finding is supported by the upstream provider implementation and an exact-function action-registration trace; it is not a fresh native Bazel/protoc reproduction. No full local Bazel suite was run.
-zbarskybot
| - { path: "e2e/rules-python-interop", slug: "e2e-rules-python-interop", runner: "ubuntu-latest" } | ||
| - { path: "e2e/rules-python-provider-compat", slug: "e2e-rules-python-provider-compat", runner: "ubuntu-latest" } | ||
| - { path: "e2e/rules-python-protobuf", slug: "e2e-rules-python-protobuf", runner: "ubuntu-22.04-32core" } | ||
| - { path: "e2e/pyc-batch", slug: "e2e-pyc-batch", runner: "ubuntu-latest" } |
There was a problem hiding this comment.
[P3] Fold the batching fixture into the existing cases workspace
Optional simplification: the new workspace adds 12 files, including a 728-line lockfile, plus two CI matrix jobs. The existing e2e/cases/pyc/test.sh already selects a unique-source closure with per-invocation batch-size overrides. Put the three-source batching library and 4/5/6 action-count checks there too, and pin its runtime tests to pyc_only. The deliberately conflicting batch_conflict_bin is outside that selected closure, so it need not force a separate module. A short .bazelrc example can preserve the global opt-in documentation without maintaining another module and CI pair.
-zbarskybot
f930148 to
df7898c
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed df7898c9a92bd9a38d7613f95f890f95283179b9, including the two changed blobs against the previously reviewed cumulative change.
The execution-log correction now passes the cache-check section in both Bazel 8 and Bazel 9 CI. One new compatibility issue is below: custom native providers with partially precompiled sources lose the per-job output-layout distinction. The before/after proof uses exact aspect/action function bodies and the real compiler, not native Bazel analysis. Existing findings are not repeated.
-zbarskybot
df7898c to
6683c4e
Compare
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
One non-blocking correction to the automatic-sharding contract below. The existing aspect-output findings are unchanged and are not repeated here.
-zbarskybot
Never shard sources the aspect infers for srcs-less rules: two forwarders of the same generated file must register identical per-source actions. Adds a srcs-less PyInfo forwarder fixture to rules-python-interop.
Decide --sourceless per compile group from each job's output count instead of once per target, so a native PyInfo producer that precompiled only some of its sources keeps both layouts correct.
Let srcs-less forwarders reuse the bytecode their forwarded rules_python library advertises and see sources it omitted from runfiles, instead of re-declaring the library's outputs. Extends the forwarder fixture to the keep_source and omit_source precompile modes.
zbarsky-openai
left a comment
zbarsky-openai
left a comment
There was a problem hiding this comment.
Re-reviewed 8de28a914f35af7aff03ca69e9e8cb6eae13df5e as requested. The per-job output grouping and inferred-source fallback fix the earlier partial-precompilation and forwarder-only overlap reproductions. Two additional forwarding cases are below; unchanged findings are not repeated.
Validation: complete 108-file inventory against the unchanged merge base, reusing the previous review for 97 identical blobs. Paired exact-function action traces and real compiler/CPython loader controls confirm these cases. These are not native Bazel analysis or a full local Bazel test run.
-zbarskybot
| for f in depset(transitive = [ | ||
| getattr(info, "transitive_pyc_files", depset()), | ||
| getattr(info, "transitive_implicit_pyc_files", depset()), | ||
| ]).to_list(): | ||
| if _same_package(f, ctx.label): | ||
| existing[f.short_path] = f |
There was a problem hiding this comment.
[P2] Preserve the producer’s optimization check across forwarding
Wrap a same-package rules_python_library(precompile = "enabled", precompile_optimize_level = 1) with the new forward_py_info(actual = ...). This fallback adopts its bytecode, but the check below reads the wrapper’s missing precompile_optimize_level as zero. The aspect follows only deps, so the producer’s conflict is not inherited through actual. Exact-function controls return complete = True with no conflicts; real SourceFileLoader and SourcelessFileLoader controls at runtime optimization zero then load the inherited level-1 code with __debug__ == False and assertions removed. Matching level-zero controls preserve assertions, and the direct producer is correctly rejected. Carry the producer’s compatibility/conflict information with reused bytecode, or reject reuse whose optimization cannot be established; an absent wrapper attribute is not proof of level zero. This is a new route around the previously fixed direct-producer check, not a native Bazel reproduction.
-zbarskybot
| if not owned: | ||
| shards = 0 |
There was a problem hiding this comment.
[P2] Keep direct and forwarded plans identical for shared sources
A native rules_python_library(name = "lib", srcs = ["a.py", "b.py"], precompile = "disabled") can be reached both directly and through forward_py_info(actual = ":lib"). With consumer deps = [":lib", ":forward"] and pyc_shards = 1 or -1, the direct path batches both sources while this inferred path forces two per-file actions. Both claim the same natural .pyc and __pycache__ outputs with different input/output sets, so they cannot share generating actions. Paired traces of the exact old/current helpers show identical actions at 6683c4ec but conflicting actions here; the current default 0 remains identical. Forward the producer’s compiled mapping, or use a consistent per-source plan on every path that can rediscover these artifacts. Having a srcs attribute does not establish exclusive ownership. This is an action-registration proof, not a fresh native Bazel run.
-zbarskybot
3 participants
PYC compilation as an action in
py_libraryfor first-partypy_library(srcs)as well as an aspect applied topy_library(deps)for legacyrules_pythonproviders.Changes are visible to end-users: yes
Add
pyc = "source|pyc|pyc_only"flag alongsidepython_version,freethreadedflags in public APIs as well as a default via--config=@aspect_rules_py//py:pyc=source|pyc|pyc_only.Test plan