feat(experiments)!: modernize Python experiment workflows - #145
feat(experiments)!: modernize Python experiment workflows#145acgetchell wants to merge 2 commits into
Conversation
- migrate the retained MNIST example from TensorFlow to a deterministic CPU PyTorch baseline on Python 3.14 - make MNIST and initializer runs local-first and failure-atomic, with hashed artifacts, source provenance, and optional Comet mirroring - lock cross-platform CPU dependencies while preserving the lightweight default development environment - clarify CDT++'s continued role as a maintained scientific reference BREAKING CHANGE: Optional Python tooling now requires CPython 3.14 and uses PyTorch instead of TensorFlow for MNIST. Closes #143
WalkthroughThe project moves to Python 3.14, replaces TensorFlow experiments with PyTorch, adds deterministic local artifact workflows with optional Comet integration, expands package and experiment validation, and updates repository lifecycle documentation. ChangesPython experiment modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@Justfile`:
- Around line 260-293: Update the venv creation command in the
python-package-check recipe to use the pinned {{ python_version }} variable
instead of the hard-coded 3.14 value, preserving the existing consumer
environment and smoke-test flow.
In `@scripts/experiment_tests/test_comet_pytorch.py`:
- Around line 59-66: Update the checkpoint filename comparison in the upload
assertion to normalize platform-specific separators before comparing, using the
recorded fileName value emitted by Comet on Windows (backslash) and POSIX
systems (slash). Preserve the existing groupingName and upload_type checks, and
ensure the normalized path still matches the model-data parent segment and
comet-torch-model.pth basename.
- Line 58: Update the histogram assertion in the test to verify that at least
one upload has upload_type equal to "histogram3d", matching the neighbouring
any-based assertions, instead of requiring exactly four uploads.
In `@scripts/experiment_tests/test_mnist_training.py`:
- Around line 31-41: Update test_training_is_replayable_on_synthetic_cpu_data
and the _train_once return type to use a NamedTuple with named metric and
weights fields. Replace positional tuple slicing and indexing with the
corresponding named fields while preserving the existing equality checks and
per-parameter weight comparisons.
In `@scripts/mnist_experiment.py`:
- Around line 328-331: Remove the initial _write_json call for
configuration.json inside the _staged_run_directory block, and define a single
configuration_path variable for that artifact. Reuse configuration_path at the
later configuration payload write near the Comet/finalization flow, preserving
the payload that includes torch and torchvision versions.
- Around line 222-232: Update _dataset_manifest to sort retained files by their
recorded POSIX path string rather than by Path object ordering, ensuring
deterministic cross-platform manifest and run.json output while preserving the
existing manifest fields.
- Around line 281-323: Replace the ANN401-triggering Any annotations in
_build_model, _train_epoch, and _evaluate with concrete torch-related types
declared through the existing TYPE_CHECKING pattern, using ModuleType for
torch_module and appropriate module, data-loader, loss-function, and optimizer
types. Preserve runtime importability without importing torch; if Any must
remain, add scoped ANN401 suppressions to each affected parameter.
- Around line 409-423: Handle the MNIST dataset-construction failure within the
experiment flow used by main, including RuntimeError raised by datasets.MNIST
when raw files are missing or invalid, and convert it to the existing
user-facing ValueError path. Preserve the current exit code 2 behavior and avoid
exposing a traceback for unavailable or incomplete data.
In `@scripts/optimize_initialize.py`:
- Around line 470-472: Introduce a dedicated OutputDirectoryExistsError subclass
of ValueError and have _staged_run_directory raise it for the existing
output-directory condition. Update the handler around the sweep to catch only
OutputDirectoryExistsError, preserving its current stderr message and exit code
while allowing unrelated ValueError failures from _write_json or
_experiment_provenance to propagate with diagnostics.
In `@scripts/tests/test_mnist_experiment.py`:
- Around line 62-73: Update test_failed_run_does_not_publish_partial_artifacts
to assert that the temporary root directory contains no entries after the failed
staged run, rather than globbing for the _staged_run_directory prefix. Preserve
the existing assertions for the final output directory and exception.
In `@scripts/tests/test_optimize_initialize.py`:
- Around line 188-189: Update the test around _experiment_provenance to also
patch scripts.optimize_initialize.shutil.which, returning a valid git path so
the test does not depend on git being installed or available on PATH. Keep the
existing qx mock and provenance assertions unchanged.
- Around line 107-128: Extend the test around _run_parameter_sweep to assert
that plotter.clf() is called once after each parameter pair and that
experiment.log_figure is invoked for the Comet mirror. Keep the existing local
artifact and provenance assertions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02d36201-716a-4b5a-913a-106f011341be
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/feature_request.md.github/workflows/python-experiments.yml.python-versionJustfileREADME.mddocs/RELEASING.mddocs/multithreading.mdpyproject.tomlscripts/bootstrap_vcpkg.pyscripts/experiment_tests/__init__.pyscripts/experiment_tests/test_comet_pytorch.pyscripts/experiment_tests/test_mnist_training.pyscripts/generate_changelog.pyscripts/generate_reference_fixtures.pyscripts/mnist_experiment.pyscripts/optimize_initialize.pyscripts/pkgx-build.shscripts/release_check.pyscripts/semgrep_fixture_config.pyscripts/subprocess_utils.pyscripts/sync_vcpkg_tool_pins.pyscripts/tag_release.pyscripts/tests/test_bootstrap_vcpkg.pyscripts/tests/test_experiment_imports.pyscripts/tests/test_generate_changelog.pyscripts/tests/test_generate_reference_fixtures.pyscripts/tests/test_justfile_discoverability.pyscripts/tests/test_mnist_experiment.pyscripts/tests/test_optimize_initialize.pyscripts/tests/test_release_check.pyscripts/tests/test_sync_vcpkg_tool_pins.pyscripts/tests/test_tag_release.pyscripts/tests/test_validate_reference_fixtures.pyscripts/validate_reference_fixtures.pyty.toml
💤 Files with no reviewable changes (15)
- scripts/validate_reference_fixtures.py
- scripts/tests/test_validate_reference_fixtures.py
- scripts/subprocess_utils.py
- scripts/bootstrap_vcpkg.py
- scripts/tests/test_sync_vcpkg_tool_pins.py
- scripts/tag_release.py
- scripts/tests/test_tag_release.py
- scripts/tests/test_generate_reference_fixtures.py
- scripts/release_check.py
- scripts/tests/test_generate_changelog.py
- scripts/tests/test_release_check.py
- scripts/generate_reference_fixtures.py
- scripts/tests/test_bootstrap_vcpkg.py
- scripts/generate_changelog.py
- scripts/sync_vcpkg_tool_pins.py
| def main(argv: Sequence[str] | None = None) -> int: | ||
| """Run the MNIST experiment from an installed uv entry point.""" | ||
| _parse_args(sys.argv[1:] if argv is None else argv) | ||
| config = _config_from_args(_parse_args(sys.argv[1:] if argv is None else argv)) | ||
| try: | ||
| _run_experiment() | ||
| _run_experiment(config) | ||
| except ModuleNotFoundError as error: | ||
| print( | ||
| f"Missing experiment dependency {error.name!r}; run `just python-sync-experiments`, then retry with `uv run --no-sync cdt-mnist-experiment`.", | ||
| file=sys.stderr, | ||
| ) | ||
| return 2 | ||
| except ValueError as error: | ||
| print(str(error), file=sys.stderr) | ||
| return 2 | ||
| return 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the torchvision MNIST download/verify path for the failure type raised when the dataset is absent.
set -euo pipefail
# Locate the installed torchvision MNIST module, if vendored or available in the repo tree.
fd -i 'mnist.py' --type f | head -20
# Confirm how the repository documents the --no-download failure mode.
rg -n -C3 -- '--no-download|no_download|Dataset not found' --glob '!**/.git/**'Repository: acgetchell/CDT-plusplus
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate MNIST files ---'
fd -i 'mnist.py' --type f | head -20 || true
printf '%s\n' '--- experiment and dependency references ---'
rg -n -C3 --glob '!**/.git/**' -- '--no-download|no_download|torchvision|MNIST|Dataset not found' . | head -240
printf '%s\n' '--- dependency declarations ---'
rg -n -C3 --glob 'pyproject.toml' --glob '*lock*' --glob '*.txt' -- 'torchvision|torch==' . | head -160Repository: acgetchell/CDT-plusplus
Length of output: 21348
🌐 Web query:
torchvision 0.28.0 MNIST source download verify RuntimeError "Dataset not found. You can use download=True to download it"
💡 Result:
The RuntimeError: Dataset not found. You can use download=True to download it typically occurs because the MNIST dataset files are missing, corrupted, or unreachable due to network or mirror issues [1][2][3]. If you are encountering this error despite setting download=True, consider the following troubleshooting steps: 1. Clear Corrupted Data: If files exist but are invalid, torchvision may fail to load them [4]. Delete the existing MNIST directory (usually located in your specified root/MNIST) and attempt to run the script again to trigger a fresh download [4]. 2. Network or Mirror Issues: The MNIST dataset download relies on pre-defined mirrors [1][5]. Historically, these mirrors (including the official LeCun repository) have faced intermittent outages, SSL certificate issues, or 403 Forbidden errors [6][7]. Recent updates to torchvision (such as PR #9411, merged in early 2026) have improved error handling for these downloads [8][9]. Ensure you are using the latest version of torchvision to benefit from these fixes. 3. Verify Directory Structure: torchvision expects a specific directory structure [1][10]. It looks for raw files (e.g., train-images-idx3-ubyte) in the root/MNIST/raw folder [10]. If you are manually providing the dataset, ensure your directory paths and file names strictly follow this expected structure [3]. 4. Manual Download: If automated downloads continue to fail, you can manually download the standard MNIST files and place them into the correct directory (root/MNIST/raw) [4]. Once the files are present, you can set download=False in your dataset initialization [3]. If you are still unable to download the dataset, verify that your environment has stable internet access and is not behind a proxy or firewall that might be blocking the download mirrors [6][7].
Citations:
- 1: https://docs.pytorch.org/vision/stable/%5Fmodules/torchvision/datasets/mnist.html
- 2: https://github.com/pytorch/vision/blob/main/torchvision/datasets/mnist.py
- 3: https://discuss.pytorch.org/t/runtimeerror-dataset-not-found-you-can-use-download-true-to-download-it/7804
- 4: https://stackoverflow.com/questions/75007536/torchvision-datasets-mnist-runtimeerror-on-jupyterlab
- 5: https://github.com/pytorch/vision/blob/12bb88738a5b6b96767c4165282f644495780a80/torchvision/datasets/mnist.py
- 6: Downloading MNIST always fails on the first mirror due to an SSL certificate error pytorch/vision#8717
- 7: torchvision.datasets.mnist has a broken url in mirrors to download the dataset pytorch/vision#8568
- 8: fix(mnist): handle download runtime errors pytorch/vision#9411
- 9: pytorch/vision@4e0198c
- 10: https://docs.pytorch.org/vision/0.21/_modules/torchvision/datasets/mnist.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/pytorch/vision/v0.28.0/torchvision/datasets/mnist.py'
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl --fail --silent --show-error "$url" -o "$tmp"
printf '%s\n' '--- torchvision v0.28.0 MNIST failure path ---'
rg -n -C8 'def _check_exists|Dataset not found|def __init__|raw_folder|resources' "$tmp"
printf '%s\n' '--- repository implementation context ---'
sed -n '326,347p' scripts/mnist_experiment.py
sed -n '116,134p' scripts/mnist_experiment.pyRepository: acgetchell/CDT-plusplus
Length of output: 14418
Handle missing or incomplete MNIST data with a user-facing error.
With torchvision 0.28.0, datasets.MNIST(..., download=False) raises RuntimeError("Dataset not found. You can use download=True to download it") when the required raw files are absent or fail integrity checks. Checking only data_directory / "MNIST" is insufficient for incomplete or corrupt data. Convert this constructor failure to ValueError, or handle it explicitly in main, so the command returns exit code 2 without a traceback.
🤖 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 `@scripts/mnist_experiment.py` around lines 409 - 423, Handle the MNIST
dataset-construction failure within the experiment flow used by main, including
RuntimeError raised by datasets.MNIST when raw files are missing or invalid, and
convert it to the existing user-facing ValueError path. Preserve the current
exit code 2 behavior and avoid exposing a traceback for unavailable or
incomplete data.
- reject overlapping MNIST data and output paths before filesystem effects - make experiment manifests, Comet integration, and failure handling portable - align Just with 1.58.0 and add pinned spelling checks - restrict CI to its required actions and separate security dependency updates
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/tests/test_justfile_discoverability.py (1)
109-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover every UV-backed recipe in the guard test.
The test checks only the hard-coded recipe lists. The Justfile also contains UV-backed recipes such as
release-check,changelog-unreleased,tag-check,tag,semgrep, andsemgrep-test. A future omission of_sync-python-devor_ensure-uvin one of those recipes can pass this test.Derive candidates from the parsed recipe bodies, or keep the list exhaustive.
🤖 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 `@scripts/tests/test_justfile_discoverability.py` around lines 109 - 131, Expand test_uv_backed_recipes_reuse_pinned_guards to cover every recipe that invokes UV, preferably by deriving candidates from parsed recipe bodies and asserting each reaches _ensure-uv through the appropriate sync dependency. If retaining explicit lists, add all UV-backed recipes such as release-check, changelog-unreleased, tag-check, tag, semgrep, and semgrep-test, preserving the existing guard assertions.Justfile (1)
285-298: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
.exeto Windows entry-point paths.When the Windows branch is selected, invoke the four commands as
.venv/Scripts/*.exe.uvinstalls[project.scripts]console entry points as Windows executables, and Bash does not resolve the extensionless absolute paths throughPATHEXT. The smoke test can therefore fail on Windows.🤖 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 `@Justfile` around lines 285 - 298, Update the Windows branch of the Python entry-point setup so scripts_directory points to the executable paths under .venv/Scripts, including the .exe suffix. Ensure the invocations of cdt-bootstrap-vcpkg, cdt-optimize-initialize, cdt-mnist-experiment, and cdt-tag-release use those Windows executable paths while preserving the Unix branch unchanged.
🤖 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.
Outside diff comments:
In `@Justfile`:
- Around line 285-298: Update the Windows branch of the Python entry-point setup
so scripts_directory points to the executable paths under .venv/Scripts,
including the .exe suffix. Ensure the invocations of cdt-bootstrap-vcpkg,
cdt-optimize-initialize, cdt-mnist-experiment, and cdt-tag-release use those
Windows executable paths while preserving the Unix branch unchanged.
In `@scripts/tests/test_justfile_discoverability.py`:
- Around line 109-131: Expand test_uv_backed_recipes_reuse_pinned_guards to
cover every recipe that invokes UV, preferably by deriving candidates from
parsed recipe bodies and asserting each reaches _ensure-uv through the
appropriate sync dependency. If retaining explicit lists, add all UV-backed
recipes such as release-check, changelog-unreleased, tag-check, tag, semgrep,
and semgrep-test, preserving the existing guard assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bfc8c8a-af62-4a0b-8d49-879ad80cc23b
📒 Files selected for processing (18)
.github/dependabot.yml.github/workflows/ci.yml.github/workflows/python-experiments.ymlJustfileREADME.mddocs/Doxyfileinclude/Utilities.hppscripts/experiment_tests/test_comet_pytorch.pyscripts/experiment_tests/test_mnist_training.pyscripts/mnist_experiment.pyscripts/optimize_initialize.pyscripts/tests/test_justfile_discoverability.pyscripts/tests/test_mnist_experiment.pyscripts/tests/test_optimize_initialize.pysemgrep.yamltests/Foliated_triangulation_test.cpptests/Manifold_test.cpptypos.toml
1 participant
BREAKING CHANGE: Optional Python tooling now requires CPython 3.14 and uses PyTorch instead of TensorFlow for MNIST.
Closes #143
Summary by CodeRabbit