fix(py_venv): merge colliding package directories#1205
Conversation
py_binary startup benchmark
sys.path quality
✅ No regression detected (PR is -1.2% vs HEAD main) |
tamird
force-pushed
the
fix-regular-package-overlay-v1
branch
from
9cad97c to
25718b2
Compare
|
The first matrix exposed an existing Airflow e2e assertion that required airflow.file to stay under the selected wheel repository. That is no longer the right contract once the directory is physically merged. This reroll replaces that path assertion with core-owned plus provider-only module checks; the exact two Airflow targets pass locally. |
tamird
force-pushed
the
fix-regular-package-overlay-v1
branch
from
25718b2 to
292b635
Compare
|
The rerolled matrix was 16/17 green; only the Bazel 8 e2e shard failed the two Airflow tests, while Bazel 9 passed. GitHub did not preserve the inner test logs. Both exact targets pass locally under Bazel 8.7.0 and the e2e 8.5.1 pin. I found the fixture was still using dotted find_spec calls, which import parent Airflow packages and test unrelated import side effects. This reroll checks the core and provider files in the resolved merged airflow directory instead. |
tamird
force-pushed
the
fix-regular-package-overlay-v1
branch
from
292b635 to
16694eb
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16694ebac0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
What new test cases failed without this fix? If none then add a new e2e/cases/ scenario to reproduce the issue. |
|
The new //py/tests/py_venv_conflict:native_nested_regular_first_collision_test fails against 16694eb before the reroll: the concrete venv projection resolves to native_graft.install while Python imports the earlier native_regular.install package. The reroll adds that reverse-order fixture alongside the existing native_nested_collision_test and asserts that the concrete projection and runtime import resolve to the same regular owner while pure sibling roots still union. I will post the reroll SHA when the public-repository push guard is cleared. |
tamird
force-pushed
the
fix-regular-package-overlay-v1
branch
from
16694eb to
2ec6556
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec6556a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Create a proper reproduction of this bug using packages in an e2e test case, not only in unit tests. |
Permissive top-level collisions currently select one whole-tree symlink and leave the other wheel on the pth fallback. When every claimant is directory-backed and one contributes a regular package, Python binds that package to the selected directory and hides unique children from the other wheel. Carry RECORD-derived top_level_dirs and native_roots through wheel metadata. Merge only relocatable directory roots. Keep roots with native-library RECORD entries on direct wheel projections plus losing-wheel fallback because copying them into a tree artifact changes their physical origin and can break origin-relative sibling lookup. When a regular package remains reachable through fallback, select its later claimant for both top-level and nested native conflicts; a namespace-only graft cannot override it and remains hidden. Cover regular and mixed directory unions, selected-wheel native metadata, ordinary native winner behavior, both nested claimant orders, and the top-level regular-first namespace-graft case while pure sibling roots still merge. Reverse-order tests assert that concrete venv projection and runtime import resolution agree and that losing native wheels stay on fallback. A package-backed google 1.9.3 plus protobuf 6.33.5 e2e reproduces the top-level mismatch through generated wheel metadata: with the old winner selection, concrete venv google points at protobuf while runtime binds google. Regenerate wheel metadata snapshots canonically.
|
Added e2e/cases/google-native-overlay with real google 1.9.3 and protobuf 6.33.5 wheels. google owns google/init.py; protobuf contributes native google/_upb/_message without a top-level init. With the 2ec6556 winner selection restored, the new target fails because the concrete venv projects protobuf/google and site-packages/google/init.py is absent while runtime still binds fallback google. The reroll selects the regular claimant, so concrete projection and runtime owner agree; protobuf stays on fallback and google.protobuf remains hidden. The focused e2e target passes at local 718f03a; I’ll post the public SHA after the manual push. |
tamird
force-pushed
the
fix-regular-package-overlay-v1
branch
from
2ec6556 to
718f03a
Compare
|
Downstream exact-head CI found an interaction with merged #1191. Two same-version pydantic_core wheels from different lock universes collide on a native root; one claimant stays on whole-wheel fallback, then the metadata pass rejects its duplicate dist-info even though it is the only fallback copy. I have a focused follow-up ready: apply collision policy first; with no fallback project the normal winner, with one fallback project nothing and let that sole visible copy own metadata discovery, and with multiple fallback copies keep the hard failure. The regression covers those three cases plus package_collisions = error. I will send the PR as soon as I can publish the branch. |
2 participants
Permissive top-level collisions currently select one whole-tree symlink and leave the other wheel on the pth fallback. When every claimant is directory-backed and one contributes a regular package, Python binds that package to the selected directory and hides unique children from the other wheel.
Carry RECORD-derived top_level_dirs and native_roots through generated whl_install metadata into PyWheelsInfo. Merge relocatable directory roots in provider order and mark their contributors covered so fallback cannot shadow the merged tree. Keep roots with native-library RECORD entries on direct wheel projections plus losing-wheel fallback, because copying those roots into a tree artifact changes their physical origin and can break origin-relative sibling lookup. When a regular package remains reachable through fallback, project the later regular claimant for both top-level and nested native conflicts; a namespace-only graft cannot override it and remains hidden. Pure sibling roots still merge.
The regression coverage keeps ordinary module winner behavior, covers regular and mixed directory unions, checks selected-wheel native metadata, and proves ordinary, top-level, and nested native collisions preserve runtime-consistent winner-only behavior while pure nested roots still union. The package-backed google 1.9.3 plus protobuf 6.33.5 e2e reproduces the top-level native mismatch through generated wheel metadata and asserts the concrete venv projection agrees with runtime import ownership while protobuf remains on fallback. The Airflow e2e asserts the resolved merged tree keeps core-owned and provider-only files. The focused py_venv, site_merge, whl_install, generated snapshot, Azure overlap, legacy pth fallback, google-native overlay, and Airflow suites pass.