Skip to content

Pin 202609 pipeline toolchain (duckdb/pandas/pyarrow/rdflib) + isolated venv - #358

Open
rdhyee wants to merge 3 commits into
isamplesorg:mainfrom
rdhyee:chore/pin-202609-toolchain
Open

Pin 202609 pipeline toolchain (duckdb/pandas/pyarrow/rdflib) + isolated venv#358
rdhyee wants to merge 3 commits into
isamplesorg:mainfrom
rdhyee:chore/pin-202609-toolchain

Conversation

@rdhyee

@rdhyee rdhyee commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • 🤖 rbotyee (Claude, operated by @rdhyee) — Raymond's intent: close the toolchain-pin gap a clean-checkout reproducibility test found on 2026-08-31 (decision 0 of the isamples_202609 publish plan), before assembling a publish set. What I did: exact-pinned pandas/pyarrow/rdflib in scripts/requirements.txt alongside the existing duckdb==1.4.4 pin, and added scripts/setup_pipeline_venv.sh so those pins install into an isolated scripts/.venv (via uv) instead of a shared interpreter.
  • Why this was needed: only duckdb was pinned, and nothing ever installed from scripts/requirements.txt into an isolated env — scripts ran against whatever was in the shared pyenv myenv (used by many other projects). Between 2026-08-28 and 2026-08-31, an unrelated myenv upgrade moved duckdb 1.4.4 → 1.5.5, which changed one step's output bytes and hard-crashed another. Full writeup: provenance/isamples_202609/step9_clean_rerun_2026-08-31.md (isamples-suite provenance repo, not in this PR).
  • Mirrors the hermetic pattern the pqg repo's step 1 already uses (its own uv.lock) — applied here as a pinned requirements.txt + uv-managed venv since these are one-shot scripts, not an installable package.

Test plan

  • bash scripts/setup_pipeline_venv.sh creates scripts/.venv and reports duckdb 1.4.4 pandas 2.3.3 pyarrow 21.0.0 rdflib 6.3.2 — matches the 2026-08-28 reference build's own manifest.json environment fields exactly.
  • Full clean-checkout chain rerun (steps 2–7) against this pinned venv, byte-compared to the 2026-08-28 reference — in progress, will report results in the provenance repo (not part of this PR).
  • Codex review

🤖 Generated with Claude Code

https://claude.ai/code/session_01NdwkgvUJhJ3G9cJNj9oUuZ

rdhyee and others added 2 commits September 2, 2026 15:21
…olated venv

The 202609 rebuild's reproducibility was verified twice (8/28, 8/28→8/31)
but only by luck: only duckdb was pinned in scripts/requirements.txt, and
nothing actually installed FROM that file into an isolated environment —
scripts ran against whatever was in the shared pyenv `myenv`, used by many
other projects. Between 8/28 and 8/31, an unrelated `myenv` upgrade moved
duckdb 1.4.4 -> 1.5.5, which changed step 2's output bytes and hard-crashed
step 4 (provenance/isamples_202609/step9_clean_rerun_2026-08-31.md in the
isamples-suite provenance repo). pandas/pyarrow/rdflib were only ever
floor-pinned (>=), so even duckdb's pin wasn't protecting the full chain.

This exact-pins scripts/requirements.txt to the versions the 8/28 reference
build actually used (recorded in that build's own manifest.json files), and
adds scripts/setup_pipeline_venv.sh to install them into a project-local
`scripts/.venv` via `uv` rather than into any shared interpreter — mirroring
the hermetic pattern the pqg repo's step 1 already uses via its own
uv.lock, applied here as a pinned requirements.txt since these are one-shot
scripts rather than an installable package.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NdwkgvUJhJ3G9cJNj9oUuZ
…hten claims

Codex review (High): the Makefile's `PY ?= python` default meant `make
all`/`make test` still ran against the ambient interpreter even after
`scripts/setup_pipeline_venv.sh` created the pinned venv — the exact mistake
this PR set out to close, just reachable through a different door. `PY` now
defaults to `scripts/.venv/bin/python` (overridable, e.g. `PY=python` for
CI, which already installs scripts/requirements.txt into its own ephemeral
runner); a Makefile warning fires if that venv is missing. Verified `make
test` (62/62) against the pinned venv both before and after this change.
build_vocab_labels.py's docstring usage block updated to match.

Codex review (Medium): scripts/requirements.txt and setup_pipeline_venv.sh
overstated hermeticity — pandas/pyarrow/rdflib's transitive deps, the
Python interpreter version, and `uv` itself are still resolver-chosen, so
this is narrower than pqg's step-1 uv.lock. Reworded both to claim only
what's actually pinned (the 4 output-critical direct deps), and to say so
explicitly rather than "all versions ... exact pins."

Codex review (Low): fixed "major-version bump" -> correct term for 1.4->1.5;
"the pin drifted" -> "the ambient install drifted, the pin was never
applied" (the file's pin didn't move); setup_pipeline_venv.sh now prints
package versions before "ready" instead of after (so an import-time failure
can't look like a successful run); DATA_PROVENANCE.md's toolchain-pin bullet
now explicitly distinguishes itself from the pre-existing "still unresolved"
data-lineage reproducibility gap immediately below it, instead of reading
as a contradiction.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NdwkgvUJhJ3G9cJNj9oUuZ
@rdhyee

rdhyee commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 rbotyee (Claude, operated by @rdhyee) — Raymond's intent: have Codex review this PR before merge, per the repo's standard multi-AI review loop. What I did: ran a Codex review; findings and my response below.

Codex review (first pass)

HighMakefile's PY ?= python default meant make all/make test still ran against the ambient interpreter even after scripts/setup_pipeline_venv.sh created the pinned venv — the exact failure mode this PR set out to close, just reachable through a different door.

Medium — the PR's language overstated hermeticity: pandas/pyarrow/rdflib's transitive dependencies, the Python interpreter version, and uv itself are still resolver-chosen, so this is narrower than a full lockfile (unlike pqg's uv.lock-based step 1).

Low — a few wording issues: "major-version bump" for a 1.4→1.5 release-line change; "the pin drifted" when the file's pin didn't move (the ambient install drifted because nothing had ever installed from the pin); setup_pipeline_venv.sh printed "ready" before its own verification step, so an import-time failure could look like success; and DATA_PROVENANCE.md's new bullet sat next to a pre-existing "still unresolved" note about an unrelated (data-lineage) reproducibility gap, reading as a contradiction.

Response (pushed in 710adf7)

  • Makefile's PY now defaults to scripts/.venv/bin/python (override with PY=python etc. — CI already installs into its own ephemeral runner via plain pip, so it isn't exposed to the ambient-drift risk this PR is about); warns clearly if the venv is missing. Verified make test (62/62) against the pinned venv.
  • Narrowed the "exact pins"/"hermetic" language in scripts/requirements.txt and setup_pipeline_venv.sh to what's actually true: the 4 output-critical direct dependencies are pinned; transitive deps/Python/uv are not, and that's flagged as a known gap rather than implied to be closed.
  • Fixed the three wording issues and the print-order bug; added a sentence in DATA_PROVENANCE.md distinguishing this (now-closed) toolchain-pin gap from the separate, still-open data-lineage gap described immediately below it.

Full Codex transcript available on request (kept in session notes, not posted here to keep this comment scannable). Second pass in progress.

…g#358

- Makefile: only warn about a missing PY when PY looks like a path
  (contains "/") -- `wildcard` doesn't search $PATH, so PY=python was
  producing a false "not found" warning even though the shell resolves it
  fine.
- DATA_PROVENANCE.md: `make` doesn't install anything -- it just uses the
  venv scripts/setup_pipeline_venv.sh already installed into. Reworded so
  only the setup script is credited with the install step.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NdwkgvUJhJ3G9cJNj9oUuZ
@rdhyee

rdhyee commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 rbotyee (Claude, operated by @rdhyee) — Raymond's intent: same as above — Codex sign-off before merge. What I did: fixed the 2 remaining low-severity nits from Codex's second pass (f8c05af) and got its final verdict.

Codex second pass — verdict

Verdict: all four original findings are substantively resolved.

Two low-severity nits it caught, both fixed in f8c05af:

  • Makefile's missing-venv check used wildcard, which doesn't search $PATH — so make ... PY=python produced a false "not found" warning. Now only checks when PY looks like a path (contains /).
  • DATA_PROVENANCE.md credited make with "installing" packages; only scripts/setup_pipeline_venv.sh installs — make just uses the resulting venv. Reworded.

Codex's own verification: make test (62/62) passing, bash -n and git diff --check clean, PR CI green.

Claude + Codex: LGTM. Not merged — Raymond's call.

https://claude.ai/code/session_01NdwkgvUJhJ3G9cJNj9oUuZ

@rdhyee rdhyee added the CC+Codex+LGTM Claude and Codex both reached LGTM; ready for human review label Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CC+Codex+LGTM Claude and Codex both reached LGTM; ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant