Drop non-local (GPU/CUDA and +local) pins from generated artifacts - #21
Conversation
*Why*
The artifacts are meant to install cleanly on a developer machine, but the ML
environments carried pins that cannot: packages with a PEP 440 local version
segment (torch==2.9.0+cu129, torch==2.7.0+cpu, flask==1.1.2+db1, horovod+db1)
that are published only on an out-of-band index or rebuilt inside the cluster
image, and GPU-only distributions (nvidia-* CUDA components, triton, flash-attn,
deepspeed) that need an NVIDIA GPU and have no wheel at all on macOS. As a local
constraint these never help and can only make uv sync unsatisfiable.
*What*
- envgen.py: _filtered now also drops any pin whose version carries a local
segment ("+" in the version), and DROP/DROP_PREFIX gain the GPU-only names
(triton, flash-attn, deepspeed) and the nvidia- prefix. Because _filtered feeds
both build_pyproject and build_constraints, this cleans pyproject.toml and
constraints.txt alike.
- Remove the now-dead "+" branch in req() (such versions no longer reach it) and
document the new rule in the module docstring and req().
- Add .github/scripts/test_envgen.py (stdlib unittest) covering the drops, the
keeps (plain torch, numpy, ray, databricks-sdk), and that both build_* omit the
dropped pins.
- Regenerate the committed artifacts under the new rule: a delta strip that
removes only the now-dropped lines (no re-fetch, so no unrelated version drift),
leaving every surviving line byte-for-byte unchanged. Touches the ML and GPU-ML
environments only; standard serverless/DBR envs had no such pins.
*Verification*
- python -m unittest test_envgen: 7 passing.
- After regeneration: no nvidia-/triton/flash-attn/deepspeed/+local pins remain in
python/; a plain torch pin and ordinary pins (numpy, pyarrow) are retained.
- All 33 pyproject.toml re-parse as valid TOML.
Co-authored-by: Isaac <no-reply@databricks.com>
*Why* Review feedback: the README's DBR-ML section still said local (+local) builds are "pinned with ==", which the envgen change made false — they are now dropped. The repo also had no single place documenting what gets dropped and why. *What* - README: fix the stale DBR-ML sentence and add a "What's dropped" section enumerating the four drop categories (system/OS, setuptools-vendored, +local builds, GPU-only distributions), noting torch/torchvision are left unpinned in ML envs, and cross-linking it from the Artifacts section. - test_envgen.py: add DbconnectPinTest covering that dbconnect_pin normalizes to ~=MAJOR.0 (so a +local release-notes version is stripped, never reaching an artifact) and returns None when databricks-connect is absent. *Verification* - python -m unittest test_envgen: 9 passing. - README renders the new section; both in-page links resolve to #whats-dropped. Co-authored-by: Isaac <no-reply@databricks.com>
*Why* Review nits: the README's PyTorch-index roadmap item read as if the +cpu/+cuXXX torch builds are pinned, contradicting the new drop policy (a maintainer could reintroduce the dropped pins). Minor terminology/comment accuracy points were raised alongside it. *What* - README: rewrite the PyTorch-index Status item to state that the +cpu/+cuXXX torch/torchvision builds are dropped today (cross-linking "What's dropped"), and that adding PyTorch's index is the future path to pin-and-resolve them instead. - envgen.py: relabel the module-docstring bullet to "Local-version and GPU builds dropped" (matches the README's clearer terminology) and narrow the nvidia- prefix comment to be accurate (it also catches the pure-Python nvidia-ml-py). *Verification* - python -m unittest test_envgen: 9 passing (no logic change). - README's three "What's dropped" links resolve to the section anchor. Co-authored-by: Isaac <no-reply@databricks.com>
*Why* Clearer, less blunt heading for the section that lists the packages the generator leaves out. *What* - README: rename "What's dropped" to "What is intentionally not included" and repoint the three in-page links to the new anchor. *Verification* - All three links resolve to #what-is-intentionally-not-included; no stale #whats-dropped anchors remain. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
left a comment
There was a problem hiding this comment.
Verified against a fresh clone of the PR branch.
What it does
envgen.py gains one rule — drop any pin whose version contains + — plus GPU-only names (triton, flash-attn, deepspeed) in DROP and an nvidia- entry in DROP_PREFIX. Because _filtered feeds both builders, this cleans pyproject.toml and constraints.txt together. 38 artifact files are regenerated as a pure deletion, a new test_envgen.py covers the rule, and the README gains a "what is intentionally not included" section.
The central premise is correct and worth affirming: constraint-dependencies / -c bind a version only if the package enters resolution — they never force an install. So removing these pins cannot break a resolution that previously worked. The blast radius is genuinely low.
The delta-strip execution is also clean. I confirmed:
- 0 additions / 376 deletions under
python/— no unrelated version drift - all 118 unique removed lines match the new rule, nothing unexplained
- all 33
pyproject.tomlstill parse as valid TOML - no residual
+localor GPU-only pins anywhere underpython/
Issues
1. The blanket + drop removes packages whose base version is on PyPI
The rationale is "resolves nowhere off the cluster." That holds for torch==2.9.0+cu129. It does not hold for flask==1.1.2+db1 or horovod==0.28.1+db1 — +db1 means Databricks rebuilt it, but flask 1.1.2 and horovod 0.28.1 are ordinary PyPI releases. The result ends up decided by incidental docs formatting:
| docs list | after this PR | |
|---|---|---|
| 13.3 ML | horovod 0.28.1 |
horovod~=0.28.1 ✅ |
| 15.4 ML | horovod 0.28.1+db1 |
dropped |
| 13.3 ML | flask 1.1.2+db1 |
dropped |
| 15.4 ML | flask 2.2.5 |
flask~=2.2.5 ✅ |
Same package, same version in the horovod case, opposite treatment. For a repo whose stated purpose is "reproduce the runtime's Python environment locally," 13.3 ML now carries no flask constraint while 15.4 does.
Stripping the segment (flask~=1.1.2) preserves fidelity and is valid PEP 440.
One caveat: a blanket strip is also wrong — it would resurrect python-apt~=2.7.7 and distro-info~=1.7, Ubuntu system packages that are currently dropped only as a side effect of the + rule. Those belong in DROP by name; then +local can be stripped rather than dropped. Worth adding them to DROP regardless of what you decide here, so the intent is explicit and survives a future change to this rule.
2. torch is now unpinned in 17 of 19 ML environments, and the two survivors are the CPU/GPU inverse of what the description implies
The body says a plain torch 2.7.0 pin is kept because it resolves to a macOS/CPU wheel. True, but it's kept only where the docs happened to list it plain — which is the GPU pages:
17.3.x-cpu-ml-scala2.13 torch UNPINNED (main: torch==2.7.0+cpu)
17.3.x-gpu-ml-scala2.13 torch~=2.7.0 <- survives
19.x-cpu-ml-scala2.13 torch UNPINNED (main: torch==2.12.0+cpu)
19.x-gpu-ml-scala2.13 torch~=2.12.0 <- survives
Every other ML env, CPU and GPU alike, loses torch entirely. So the GPU artifact pins torch and its CPU sibling doesn't — backwards from the reasoning given. Fixing item 1 resolves this along with it.
3. Test bug: substring assertions will misfire on real inputs
assertNotIn("torch", out) passes only because the fixture contains no other torch* package. Real ML envs ship torchmetrics, torcheval, torch-optimizer, torchvision. I confirmed the failure — adding torchmetrics to the fixture makes the assertion fail even though the torch pin was correctly dropped:
numpy~=2.1.3
torcheval~=0.0.7
torchmetrics~=1.6.0
--- assertNotIn("torch", out) -> 'torch' in out == True => test FAILS
Assert on the rendered line ("torch~=", or the exact list entry) instead of the bare name. Same pattern applies to triton — no collision today, but the same trap.
4. The tests don't run anywhere
.github/workflows/ contains only sync.yml, which runs sync.py and never the tests. Nine tests that execute only when someone types the command will rot. A ~10-line job on push/PR would close this, and it's the natural place to also assert the invariant in item 5.
5. req()'s new docstring states an invariant the code no longer enforces
The docstring says "Every version passed in is therefore a plain release" and the defensive + branch is gone. That's true for the two in-tree call sites, but req() is module-level and importable, and if a local version does reach it the output is silently invalid — confirmed with packaging:
torch~=2.9.0+cu129 INVALID -> Local version label can only be used with `==` or `!=`
That yields a corrupt-but-plausible artifact instead of an error. assert "+" not in version keeps it cheap and loud.
Related: dbconnect_pin is the one function reading raw pkgs rather than _filtered — good catch adding a test for it, and .split(".")[0] does make it safe. Worth a comment saying that's why.
6. The nvidia-ml-py rationale doesn't fit
The prefix comment calls it "GPU tooling, no local use" and the README says these "have no macOS wheel at all." nvidia-ml-py is a pure-Python NVML binding — it installs anywhere; it just can't do anything without a driver. Dropping it is defensible, but not for the stated reason, and it's grouped with genuinely platform-locked wheels like nvidia-cublas-cu12. I couldn't reach PyPI from my network to confirm the wheel tag, so please verify before adjusting the wording.
7. Nit
Body says "7 passing"; it's 9 (FilterNonLocal 3, BuildArtifacts 2, Req 2, DbconnectPin 2).
Verdict
The GPU-only name drops (nvidia-*, triton, flash-attn, deepspeed) are clearly right and I'd merge those as-is.
The blanket + rule is the part I'd push back on: it conflates "unpublishable build" with "Databricks-patched build of a real PyPI release," and the visible cost is torch disappearing from 17 ML environments plus the flask/horovod split. Fixing items 1 and 3 is a small change to the same lines this PR already touches. Item 4 is worth doing while the file is fresh.
Reviewed by Isaac on behalf of Anton.
*Why* Review feedback (Anton): the blanket "drop any pin with a +local segment" rule was too broad. It dropped Databricks rebuilds of ordinary PyPI releases (flask==1.1.2+db1, horovod==0.28.1+db1) and unpinned torch in 17 of 19 ML envs, with the CPU/GPU survivors inverted from the stated reasoning. For a repo whose purpose is local reproduction, stripping the segment preserves fidelity and stays resolvable. *What* - envgen.py: req() now strips a PEP 440 local segment and pins the base release (torch 2.9.0+cu129 -> torch~=2.9.0, flask 1.1.2+db1 -> flask~=1.1.2); _filtered no longer drops +local pins. Genuine Ubuntu system packages whose base is not on PyPI (python-apt, distro-info) are added to DROP by name so stripping can't resurrect them. GPU-only name/prefix drops (nvidia-*, triton, flash-attn, deepspeed) unchanged. - Regenerated all artifacts from origin/main under the new rules: +local pins are stripped to base (== -> ~=), GPU-only and system packages dropped. torch/torchvision are now consistently pinned across CPU and GPU ML envs; flask/horovod no longer differ by incidental docs formatting. - test_envgen.py: cover strip behavior and system-package drops; assert on rendered pins (torch~=2.9.0, and torchmetrics kept) rather than bare substrings, which previously could misfire on torch*/triton* neighbors. Renamed FilterNonLocalTest -> FilterTest for scope accuracy. - README: "What is intentionally not included" now documents strip-vs-drop, adds the system packages, corrects the nvidia-ml-py rationale, and the DBR-ML + PyTorch-index notes reflect base-pinning. *Verification* - python -m unittest test_envgen: 11 passing. - No +local segments remain under python/; no nvidia-*/triton/flash-attn/deepspeed/ python-apt/distro-info pins remain; torch/flask/horovod re-pinned as ~=base. - All 33 pyproject.toml re-parse as valid TOML. Co-authored-by: Isaac <no-reply@databricks.com>
*Why* Review feedback (Anton, item 4): the new tests only ran when someone typed the command; nothing exercised them in CI, so they would rot. The repo had no test workflow (only the weekly sync). *What* - Add .github/workflows/test.yml: on push / pull_request touching .github/scripts/**, run `python -m unittest` over the scripts. Mirrors sync.yml's conventions (linux-ubuntu-latest runner for the org IP allow list, SHA-pinned checkout/setup-python, Python 3.12). No third-party deps. *Verification* - Workflow is valid YAML; the same command passes locally (11 tests). Co-authored-by: Isaac <no-reply@databricks.com>
|
Thanks @anton-107 — all addressed (commits
Also checked the open issues (#18 pandas cp312, #19 grpcio-status conflict, #20 grpcio macOS wheel): this PR doesn't touch those packages, so they're unaffected — same spirit, separate follow-ups. |
*Why* Second-pass review: horovod was kept as horovod~=0.28.1 while its same-class GPU siblings (deepspeed, flash-attn) are dropped — a source-only distribution needing MPI/NCCL + a compiler, which fails a plain local install (Claude/Anton). And the test workflow's path filter excluded itself, so a workflow-only change would merge untested (Codex). *What* - envgen.py: add horovod to DROP (GPU-only group); test_envgen covers it. - Regenerated artifacts: horovod removed from the ML envs that carried it. - README: add horovod to the GPU-only "not included" list. - .github/workflows/test.yml: add the workflow itself to both path filters so a change to its runner/pins/command re-runs the tests. *Verification* - python -m unittest test_envgen: 11 passing. - No horovod / nvidia-* / triton / flash-attn / deepspeed / python-apt / distro-info / +local pins remain under python/; all 33 pyproject.toml valid TOML. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
left a comment
There was a problem hiding this comment.
Approving. Both substantive problems from the first pass are properly fixed, and the fix is better than what I suggested — putting the strip in req() makes the invalid output unreachable rather than merely asserted against.
Verified against a fresh clone at 73ec1a0 rather than taking the summary at face value:
- Tests pass exactly as CI invokes them.
python -m unittest discover -s .github/scripts -p "test_*.py"→Ran 11 tests … OK. Count matches the corrected 11. - Items 1 and 2 (the CPU/GPU inversion) — resolved. Siblings now agree:
17.3.xbothtorch~=2.7.0 torchvision~=0.22.0;19.xbothtorch~=2.12.0 torchvision~=0.27.0. flask is pinned in both 13.3 and 15.4 (1.1.2/2.2.5) — no split. - Item 3 (substring trap) — fixed properly.
_checkasserts on rendered pins (torch~=2.9.0,nvidia-cublas-cu12~=,triton~=,python-apt~=) withtorchmetricsin the fixture as a live neighbor guard, and both build paths share it so pyproject and constraints can't drift apart. - Item 5 — invariant now enforced by construction. I validated all 9,441 constraint lines across all 33 environments as PEP 508 with
packaging: 0 invalid, no residual+, no single-component~=pins. - Artifacts are reproducible, not hand-edited. I re-derived all 33
constraints.txtfrommain's data using only the new rules: 0 mismatches, byte-for-byte. This is what makes me comfortable approving 38 regenerated files. - Item 4 — CI job is correct.
linux-ubuntu-latestwith the runner-group rationale carried fromsync.yml, SHA-pinned actions,permissions: contents: read, and the follow-up commit rightly addstest.ymlto its ownpathsso the workflow gates changes to itself.
Blast radius of the strip is narrower than it sounds: only torch, torchvision, flask are actually affected. The other former +local packages (horovod, python-apt, distro-info) are all name-dropped now.
Two things to settle (non-blocking)
A. horovod lost its pin in 4 environments, and the PR body doesn't mention it. The comment justifies it as source-only, needing MPI/NCCL + a compiler — reasonable on the merits. But main carried horovod~=0.28.1 in 13.3.x and 14.3.x (cpu and gpu), where it was a plain pin the old + rule never touched. So this isn't a consequence of the strip fix; it's an independent policy change riding along. Worth a line in the description, since it's the one behavior change here that isn't traceable to a review comment.
B. pynvml and cuda-* escape the GPU-only rule, so the README now asserts a cleaner taxonomy than the code implements. Still pinned after this PR:
pynvml~=11.5.0 (14.3.x cpu + gpu ML)
cuda-bindings~=13.2.0 (19.x gpu ML)
cuda-pathfinder~=1.5.4
cuda-toolkit~=13.0.2
pynvml is the same NVML binding as nvidia-ml-py — same purpose, one dropped by prefix and one kept, purely because of how it's named. cuda-toolkit is squarely in the class the README describes. All four are pre-existing on main, so this is not a regression from this PR and I'm not blocking on it — but either extend DROP or soften the README wording so the two agree.
Follow-ups, not for this PR
norm,parse_requirements, and_labelare still untested.parse_requirementsis the entry point for every artifact and carries real logic (the*footnote-marker strip, and a regex that silently skips non-matching lines — a malformed docs table would drop packages quietly). Obvious next test now that the harness exists.- Nothing guards artifacts against script drift.
sync.py --checkexists for exactly this and isn't in CI; only the weeklysync.ymlregenerates, so a futureenvgen.pychange can land with stalepython/artifacts and CI stays green. Wiring--checkinto the test job would need thepathsfilter widened, so it's a separate change.
Reviewed by Isaac on behalf of Anton.
81167e5 to
73ec1a0
Compare
*Why* Review nit (Anton): pynvml and the cuda-* packages escaped the GPU-only rule, so the README described a cleaner taxonomy than the code implemented. pynvml is the same NVML binding as nvidia-ml-py (dropped by the nvidia- prefix); cuda-toolkit / cuda-bindings / cuda-pathfinder are CUDA tooling — all GPU-only, useless on a dev machine. *What* - envgen.py: add pynvml to DROP and a cuda- entry to DROP_PREFIX; update the module docstring GPU-only bullet. README GPU-only bullet now lists cuda-*/pynvml and states nvidia-ml-py/pynvml are pure-Python NVML bindings (installable anywhere, useless without a driver) — so README and code agree. - test_envgen: extend the GPU-only drop test with pynvml + cuda-*. - Regenerated: pynvml removed from 14.3 cpu/gpu ML; cuda-bindings/pathfinder/toolkit removed from 19.x gpu ML. *Verification* - python -m unittest test_envgen: 11 passing. - No pynvml / cuda-* pins remain under python/; pandas untouched on this branch; all 33 pyproject.toml valid TOML. Co-authored-by: Isaac <no-reply@databricks.com>
Why
The per-environment constraint artifacts mirror the cluster image faithfully — right for the image, but some ML pins can't install on a developer machine.
What
envgen.py, applied to bothpyproject.tomlandconstraints.txt:+localversion segments and pins the base release (torch 2.9.0+cu129→torch~=2.9.0,flask 1.1.2+db1→flask~=1.1.2).~=is invalid with a local segment and the build exists only off-index, while its base is an ordinary PyPI version.nvidia-*/cuda-*CUDA wheels & tooling, plustriton,flash-attn,deepspeed,horovod,pynvml: all need an NVIDIA GPU (and a CUDA/MPI toolchain) a dev machine lacks.python-apt,distro-info(base version not on PyPI)..github/workflows/test.ymlrunning the unit tests on push / PR touching.github/scripts/**, plus a newtest_envgen.pyand a README "What is intentionally not included" section.Note on
horovod: it was previously kept (a plain pin) in 13.3/14.3 ML, so dropping it is an independent policy change, not a consequence of the strip fix — it's the same source-only, GPU/MPI-only class asdeepspeed/flash-attn, which can'tpip installcleanly on a dev machine.torch/torchvision are now pinned consistently across CPU and GPU ML envs. All affected artifacts are regenerated as a mechanical transform of the release-notes pins (no unrelated version drift).
Verification
python -m unittest test_envgen— 11 passing; CItest-envgengreen.+local/ GPU / system pins remain underpython/; all 33pyproject.tomlre-parse as valid TOML.The pandas-on-Python-3.12 fix for DBR 16.4 / serverless-v3 is split into #22.
This pull request and its description were written by Isaac.