Make Binder build the native Rust core - #383
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBinder now provisions a pinned Python and scientific environment, installs the project from source with native-build checks, removes build artifacts, validates the setup in tests and CI, and adds Binder access documentation and links. ChangesBinder source build
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Repo2Docker
participant Environment
participant PostBuild
participant Pip
Repo2Docker->>Environment: Provision Python, Rust, Node.js, and notebook dependencies
Environment->>PostBuild: Run strict post-build script
PostBuild->>Pip: Install project from source with native core required
PostBuild->>PostBuild: Remove node_modules and target
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_binder_config.py (1)
6-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate parsed configuration instead of substrings.
These assertions can pass when required tokens appear in comments or unrelated commands, and they do not verify dependency-list membership, environment-variable ordering, or cleanup behavior. Parse the YAML or use anchored entry checks, then validate the relevant script structure or execute it against a temporary fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_binder_config.py` around lines 6 - 16, The test_binder_source_build_requires_native_core currently relies on unanchored substring assertions that can match comments or unrelated content. Parse environment.yml or use anchored dependency-entry checks, and validate postBuild’s required environment assignments, command ordering, and cleanup behavior using structured script checks or execution against a temporary fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.binder/environment.yml:
- Around line 6-9: Update the Binder environment specification to pin Python,
Node.js, Rust, and pip to exact reproducible builds rather than broad
major/minor versions. Use exact build-level package pins or replace the
dependency list with a committed conda lock file, preserving the existing
toolchain components.
In @.binder/postBuild:
- Around line 11-13: Align the cleanup contract with the postBuild behavior:
either extend .binder/postBuild to remove the specified Cargo registry/git and
npm caches after installation, or update the relevant spec/ documentation to
claim only node_modules and target are removed. Ensure the spec/ directory
reflects the chosen behavior and remains current.
---
Nitpick comments:
In `@tests/test_binder_config.py`:
- Around line 6-16: The test_binder_source_build_requires_native_core currently
relies on unanchored substring assertions that can match comments or unrelated
content. Parse environment.yml or use anchored dependency-entry checks, and
validate postBuild’s required environment assignments, command ordering, and
cleanup behavior using structured script checks or execution against a temporary
fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf8cc38e-baf6-4ab3-bf3c-fa44abbbaba0
📒 Files selected for processing (4)
.binder/environment.yml.binder/postBuildspec/design-dossier.mdtests/test_binder_config.py
Alek99
left a comment
Alek99
left a comment
There was a problem hiding this comment.
The diagnosis and the mechanism are right — .binder/ + conda rust/nodejs + a source install under XY_REQUIRE_CARGO=1 is exactly how you make repo2docker produce a native core. But as written the Binder image cannot build: three of the four conda pins do not exist on conda-forge, so the environment solve fails before postBuild ever runs. And with the solve fixed, most examples/ notebooks still won't run.
Green CI is not evidence here — nothing in CI builds a Binder image.
1. Blocking: 3 of 4 conda pins don't exist
Checked each pinned dist against anaconda.org (method validated with a known-good control dist that returns 200):
pin in .binder/environment.yml |
exists? | reality on conda-forge |
|---|---|---|
python=3.13.5=h2b335a9_102_cp313 |
404 | the linux-64 build is hec9711d_102_cp313 (h2b335a9 is the 3.13.8 hash) |
nodejs=22.16.0=hb8e1007_0 |
404 | 22.16.0 was never packaged — linux-64 goes 22.13.0 → 22.17.0 |
rust=1.88.0=h3a20983_0 |
404 | the linux-64 build is h1a8d7c4_0 |
pip=25.1.1=pyh8b19718_0 |
✅ noarch | — |
Net effect: mamba env update fails with "nothing provides …", the image build dies, and #379 gets worse — a visitor sees an opaque build log instead of today's actionable ImportError.
2. Blocking for the issue's goal: notebook dependencies are missing
pip install . brings only numpy + anywidget. repo2docker's frozen 3.13 base env has none of the scientific stack (its lock carries only matplotlib-inline, plus requests). Imports across examples/**/*.ipynb:
38 numpy 20 matplotlib 10 sklearn 8 requests 6 pandas
3 seaborn 2 scipy 2 pysam 2 urllib3 1 h5py 1 gwosc
The specific notebook in #379 (04_dukascopy_fx_ticks) survives because requests/urllib3 happen to be in the base — but the pdsh series, matplotlib_shim.ipynb, and everything touching sklearn/pandas/seaborn/scipy still fail at import. #379 is "examples cannot be run on online jupyter notebook"; fixing the xy import alone doesn't close it. Please add the notebook deps to environment.yml (matplotlib, pandas, scipy, scikit-learn, seaborn, h5py at minimum) and either add or explicitly scope out the exotic ones (pysam, gwosc).
3. Pinning strategy, once the strings are corrected
- Exact build strings are the wrong granularity here. They are arch-specific (mybinder also runs aarch64 nodes) and conda-forge repackages/removes builds, so this file will silently rot back into an unbuildable image.
- Pinning Python below repo2docker's base is actively harmful. Its 3.13 lock ships
python 3.13.8andpip 25.2; asking for 3.13.5 / pip 25.1.1 forces mamba to downgrade the interpreter and churn the whole preinstalled Jupyter stack. - The wheel is ABI-agnostic (
py3-none-<plat>, ctypes C ABI), so nothing here needs a Python pin at all — the 3.14 default that reported the bug would work fine. Suggest droppingpython/pipand usingnodejs=22.*,rust=1.88.*. - The inline comment says 3.13 is "covered by XY's test matrix", but CI's only explicit pin is
3.11(the floor job) and there is no.python-version. The stated justification doesn't hold.
4. The test locks in the wrong values, and can't detect a structure error
Running the test body on this branch passes — with unbuildable pins. That's the problem: it asserts transcription, not solvability.
- The
dependenciesset is built from every-line, soconda-forge(a channel) is asserted as a dependency (tests/test_binder_config.py:18). Swap thechannels:anddependencies:keys and the test still passes. - Exact
commands ==matching means any added comment or reorder inpostBuildbreaks the test, while a semantically broken script that keeps the shape passes it. not line.startswith("#!")is dead code — already covered bystartswith("#").stat().st_mode & 0o111fails on a Windows checkout (CI's pytest is ubuntu-only, so this one is only a local-dev nit).
Better: parse the YAML properly and assert properties — rust and nodejs present, XY_REQUIRE_CARGO=1 exported before the pip install, the Playwright download disabled, the script executable. The only check with real signal would be a CI job running jupyter-repo2docker --no-run ., which would have caught finding 1 outright. Probably worth the runner minutes, since nothing else in the repo exercises this config.
5. Worth stating in the PR description
Binder is linux-64, where published wheels already exist — pip install xy needs no toolchain and carries no build risk. This PR instead takes on a full cargo build --release + npm ci + vite build inside repo2docker (build-timeout and image-size exposure) purely to keep notebooks on the browsed commit. That's a defensible trade-off, but it should be stated explicitly, ideally with a documented fallback for when the source build fails.
What's correct
XY_REQUIRE_CARGO=1is the right lever —hatch_build.py:171. Without it the hook silently ships a pure-Python wheel, which is precisely the #379 failure mode.nodejsis genuinely required, not incidental: the JS client is a hard requirement of every distribution and is built withnpm ci && node js/build.mjsfrom a clone.PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1is the correct variable and it matters —playwrightis indevDependencies, sonpm ciwould otherwise pull several hundred MB of browsers into the image.rm -rf node_modules targetis right for image size; the installed wheel force-includes the lib into site-packages, so nothing at runtime depends on those directories..binder/postBuildis committed executable (100755), and there is no competing root-level repo2docker config to conflict with.binder/.
One aside: the repo has no Binder references at all (no launch badge, no docs link), so once this works nothing points users at it.
The exact conda build strings pinned three dists that do not exist on conda-forge (python 3.13.5's hash belongs to 3.13.8, nodejs 22.16.0 was never packaged, rust 1.88.0's linux-64 build hash was wrong), so the mamba solve failed before postBuild ever ran. Build strings are also arch-specific (mybinder schedules aarch64 nodes) and get retired by conda-forge rebuilds, so replace them with loose pins: rust=1.88.*, nodejs=22.*. Drop the python/pip pins entirely — the wheel is ABI-agnostic (py3-none, ctypes C ABI), and pinning below repo2docker's frozen base environment only forces an interpreter downgrade that churns the preinstalled Jupyter stack. Provision the scientific stack the example notebooks import (matplotlib, pandas, scipy, scikit-learn, seaborn, h5py, requests): the checkout install brings only numpy + anywidget, so most of examples/ failed at import even with a working xy. pysam (bioconda-only) and gwosc are scoped out explicitly for in-notebook install. Rewrite tests/test_binder_config.py to assert properties of the parsed config — toolchain and notebook stack present, no arch-specific build strings, no interpreter pin, XY_REQUIRE_CARGO=1 and the Playwright download guard exported before the pip install, caches removed after — instead of transcribing the file contents, which passed verbatim with the unbuildable pins. PyYAML joins the dev group for the parse. Add .github/workflows/binder.yml running repo2docker --no-run on .binder/ changes: nothing else in CI builds a Binder image, and it is the only check that would have caught the unsolvable pins. Point users at the deployment with a README launch badge and a docs section, and record the source-build trade-off (linux-64 wheels exist; the source build buys browsed-commit fidelity) and its fallback in the dossier.
repo2docker's default kernel env resolves to Python 3.10 (observed 3.10.19 on both mybinder.org and the binder.yml CI run), below the package's requires-python >=3.11, so the postBuild pip install died at metadata: "Package 'xy' requires a different Python: 3.10.19 not in '>=3.11'". Pin python=3.13.* — version only, never an arch-specific build string — and assert in the test that the pinned version satisfies the package floor instead of asserting no pin exists.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/integrations/notebooks.md`:
- Around line 110-116: Update the Binder description near the mybinder.org link
to avoid claiming it builds the browsed or current repository commit; state that
Binder builds the main revision resolved by the pinned URL, or replace the URL
with a commit-specific link and retain the matching description.
In `@tests/test_binder_config.py`:
- Around line 32-34: Update the binder configuration test around the
package-name assertions to also validate the required toolchain versions: Rust
must be pinned to 1.88 and Node.js to 22, while preserving the existing package
presence checks for rust, nodejs, and NOTEBOOK_STACK.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4de8b3a4-7c25-4ead-abdd-15bcf99c56f5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.binder/environment.yml.github/workflows/binder.ymlREADME.mddocs/integrations/notebooks.mdpyproject.tomlspec/design-dossier.mdtests/test_binder_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/design-dossier.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
mybinder's bids builder node failed the mamba env update twice at the same spot — "Cannot find a valid extracted directory cache for 'zipp-...conda' / Package cache error" — while CI built the identical commit green. The scientific stack does not need conda at all: every package has manylinux wheels, so install it with pip in postBuild and leave conda carrying only the toolchain (python, rust, nodejs). That keeps ~200 packages out of the mamba link phase, which both speeds the image build and removes the flaky extraction path.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The python-floor CI job installs the bare package + pytest (that is its point), so tests/test_binder_config.py now importorskips yaml the same way the suite handles pyarrow and pillow, instead of erroring at collection. Review fixes: pysam and gwosc join the postBuild pip line — both ship wheels, so provisioning them beats a dead markdown %pip suggestion and lets the three real_world genomics/LIGO notebooks run. The rm line also drops cargo's crate-download cache and npm's package cache, matching what the dossier promises about build-only caches. The config test now asserts the contract versions (rust=1.88.*, nodejs=22.*), not just package names. binder.yml pins jupyter-repo2docker==2026.4.0 for determinism and also triggers on pyproject.toml/hatch_build.py, whose contracts (requires-python, the XY_REQUIRE_CARGO hook) the image build consumes directly. Docs and dossier stop claiming the badge builds the browsed commit — Binder builds the launched ref, and the badge launches main. Declined from review: exact conda build pins / a conda lock (that exact advice produced the unsolvable image the human review flagged) and widening the workflow trigger to all source changes (the wheel jobs already gate src/ and js/; this build is too expensive per PR).
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
2 participants
Motivation
cargowas absent, leaving notebooks that fail at import time because the native Rust core is missing (examples cannot be run on online jupyter notebook (mybinder) due to Rust core missing #379).Trade-off, stated explicitly
Binder is linux-64, where published wheels already exist —
pip install xywould need no toolchain and carry no build risk. This PR instead takes on a fullcargo build --release+npm ci+ vite build inside repo2docker (build-timeout and image-size exposure) deliberately, so the notebooks always run against the launched ref rather than the last release. Fallback if the source build breaks: swap thepostBuildinstall topip install xy(published wheel), losing only launched-ref fidelity. This is recorded inspec/design-dossier.md.Description
.binder/environment.ymlprovisions only the build toolchain with loose, arch-agnostic pins (rust=1.88.*,nodejs=22.*). Exact conda build strings are deliberately avoided: they are arch-specific (mybinder also schedules aarch64 nodes) and conda-forge rebuilds retire them, which silently rots the file into an unsolvable image.python=3.13.*): repo2docker's default kernel env resolves to Python 3.10 (observed 3.10.19 on both mybinder.org and CI), below the package'srequires-python >=3.11, so without the pin the pip install fails at metadata. pip stays unpinned.examples/**/*.ipynb(matplotlib, pandas, scipy, scikit-learn, seaborn, h5py, requests, plus pysam and gwosc for thereal_worldseries) installs from pip wheels on thepostBuildinstall line rather than through conda — keeping ~200 packages out of the mamba solve makes the build faster and sidesteps a reproducible mamba extraction failure observed on a mybinder builder node ("Cannot find a valid extracted directory cache")..binder/postBuild(executable) exportsXY_REQUIRE_CARGO=1so a missing toolchain fails the image build instead of shipping a coreless wheel, disables Playwright's browser download (PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1), installs the checkout + notebook stack withpip --no-cache-dir, and removes the build-only caches (node_modules,target, cargo's crate downloads, npm's cache).tests/test_binder_config.pyasserts properties of the parsed config rather than transcribing its contents: contract toolchain versions present, no arch-specific build strings, interpreter pinned at or above therequires-pythonfloor, both env exports ordered before the pip install, notebook stack on the install line, cache removal after it, script executable (skipped on Windows checkouts). Itimportorskips PyYAML so the python-floor CI job (bare package + pytest by design) skips it cleanly..github/workflows/binder.ymlruns a pinnedjupyter-repo2docker==2026.4.0 --no-run .on changes to.binder/**,pyproject.toml, orhatch_build.py— nothing else in CI builds a Binder image, so this is the only check that exercises the config end to end (it caught the Python-floor conflict on this very PR).docs/integrations/notebooks.mda "Run the Examples on Binder" section, so the deployment is discoverable. Both are explicit that the badge launchesmain.spec/design-dossier.md.Testing
04_dukascopy_fx_ticks.ipynb) opens with the native core present..github/workflows/binder.yml(fullrepo2dockerimage build) green on this PR.uv run pytest -q tests/test_binder_config.py, pre-commit, ruff check/format — all pass; the missing-PyYAML floor environment was simulated locally and skips cleanly.Fixes #379