feat(experiments)!: modernize Python experiment workflows - #145
feat(experiments)!: modernize Python experiment workflows#145acgetchell wants to merge 1 commit 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
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
| # Build both Python artifacts and exercise every installed entry point outside the checkout. | ||
| [group('workflows')] | ||
| python-package-check: _sync-python-dev | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| artifact_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-artifacts.XXXXXX")" | ||
| consumer_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-consumer.XXXXXX")" | ||
| cleanup() { | ||
| rm -rf "$artifact_directory" "$consumer_directory" | ||
| } | ||
| trap cleanup EXIT | ||
|
|
||
| uv build --out-dir "$artifact_directory" | ||
| wheel="$(find "$artifact_directory" -maxdepth 1 -name '*.whl' -print -quit)" | ||
| [[ -n "$wheel" ]] || { echo "uv build did not produce a wheel." >&2; exit 1; } | ||
| uv venv --python 3.14 "$consumer_directory/.venv" | ||
| uv pip install --python "$consumer_directory/.venv" --no-build "$wheel" | ||
|
|
||
| if [[ -x "$consumer_directory/.venv/bin/python" ]]; then | ||
| python="$consumer_directory/.venv/bin/python" | ||
| scripts_directory="$consumer_directory/.venv/bin" | ||
| else | ||
| python="$consumer_directory/.venv/Scripts/python.exe" | ||
| scripts_directory="$consumer_directory/.venv/Scripts" | ||
| fi | ||
| ( | ||
| cd "$consumer_directory" | ||
| "$python" -c "import scripts" | ||
| "$scripts_directory/cdt-bootstrap-vcpkg" --help >/dev/null | ||
| "$scripts_directory/cdt-optimize-initialize" --help >/dev/null | ||
| "$scripts_directory/cdt-mnist-experiment" --help >/dev/null | ||
| "$scripts_directory/cdt-tag-release" --help >/dev/null | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse the pinned python_version variable instead of a bare 3.14 literal.
Line 275 creates the smoke-test venv with uv venv --python 3.14. The rest of the Justfile (for example the clang-tidy recipe at line 165) resolves the exact pinned patch through {{ python_version }} ("3.14.6"). uv venv --python 3.14 accepts any locally available 3.14.x interpreter, so this recipe can smoke-test the package against a different patch release than the one the project pins elsewhere.
Use {{ python_version }} here to keep one source of truth for the exact patch version under test.
♻️ Proposed fix
- uv venv --python 3.14 "$consumer_directory/.venv"
+ uv venv --python {{ python_version }} "$consumer_directory/.venv"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build both Python artifacts and exercise every installed entry point outside the checkout. | |
| [group('workflows')] | |
| python-package-check: _sync-python-dev | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| artifact_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-artifacts.XXXXXX")" | |
| consumer_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-consumer.XXXXXX")" | |
| cleanup() { | |
| rm -rf "$artifact_directory" "$consumer_directory" | |
| } | |
| trap cleanup EXIT | |
| uv build --out-dir "$artifact_directory" | |
| wheel="$(find "$artifact_directory" -maxdepth 1 -name '*.whl' -print -quit)" | |
| [[ -n "$wheel" ]] || { echo "uv build did not produce a wheel." >&2; exit 1; } | |
| uv venv --python 3.14 "$consumer_directory/.venv" | |
| uv pip install --python "$consumer_directory/.venv" --no-build "$wheel" | |
| if [[ -x "$consumer_directory/.venv/bin/python" ]]; then | |
| python="$consumer_directory/.venv/bin/python" | |
| scripts_directory="$consumer_directory/.venv/bin" | |
| else | |
| python="$consumer_directory/.venv/Scripts/python.exe" | |
| scripts_directory="$consumer_directory/.venv/Scripts" | |
| fi | |
| ( | |
| cd "$consumer_directory" | |
| "$python" -c "import scripts" | |
| "$scripts_directory/cdt-bootstrap-vcpkg" --help >/dev/null | |
| "$scripts_directory/cdt-optimize-initialize" --help >/dev/null | |
| "$scripts_directory/cdt-mnist-experiment" --help >/dev/null | |
| "$scripts_directory/cdt-tag-release" --help >/dev/null | |
| ) | |
| # Build both Python artifacts and exercise every installed entry point outside the checkout. | |
| [group('workflows')] | |
| python-package-check: _sync-python-dev | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| artifact_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-artifacts.XXXXXX")" | |
| consumer_directory="$(mktemp -d "${TMPDIR:-/tmp}/cdt-python-consumer.XXXXXX")" | |
| cleanup() { | |
| rm -rf "$artifact_directory" "$consumer_directory" | |
| } | |
| trap cleanup EXIT | |
| uv build --out-dir "$artifact_directory" | |
| wheel="$(find "$artifact_directory" -maxdepth 1 -name '*.whl' -print -quit)" | |
| [[ -n "$wheel" ]] || { echo "uv build did not produce a wheel." >&2; exit 1; } | |
| uv venv --python {{ python_version }} "$consumer_directory/.venv" | |
| uv pip install --python "$consumer_directory/.venv" --no-build "$wheel" | |
| if [[ -x "$consumer_directory/.venv/bin/python" ]]; then | |
| python="$consumer_directory/.venv/bin/python" | |
| scripts_directory="$consumer_directory/.venv/bin" | |
| else | |
| python="$consumer_directory/.venv/Scripts/python.exe" | |
| scripts_directory="$consumer_directory/.venv/Scripts" | |
| fi | |
| ( | |
| cd "$consumer_directory" | |
| "$python" -c "import scripts" | |
| "$scripts_directory/cdt-bootstrap-vcpkg" --help >/dev/null | |
| "$scripts_directory/cdt-optimize-initialize" --help >/dev/null | |
| "$scripts_directory/cdt-mnist-experiment" --help >/dev/null | |
| "$scripts_directory/cdt-tag-release" --help >/dev/null | |
| ) |
🤖 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 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.
| self.assertTrue(any(metric.get("metricName") == "loss" and metric.get("epoch") == 1 for metric in metrics)) | ||
| self.assertTrue(any(parameter.get("paramName") == "seed" and parameter.get("paramValue") == 0 for parameter in parameters)) | ||
| self.assertTrue(any(record["type"] == "graph" for record in records)) | ||
| self.assertEqual(sum(upload["upload_type"] == "histogram3d" for upload in uploads), 4) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Assert a lower bound on the histogram count.
The exact count 4 depends on how Comet batches weight and gradient histograms for this specific two-Linear model and this single backward pass. The dependency range in pyproject.toml is comet-ml>=3.58.3,<4, so a minor Comet upgrade can change the batching and break this test without any behavior regression. Every neighbouring assertion already uses assertTrue(any(...)). Assert that histograms were produced instead of the exact number.
♻️ Proposed refactor
- self.assertEqual(sum(upload["upload_type"] == "histogram3d" for upload in uploads), 4)
+ self.assertGreaterEqual(sum(upload["upload_type"] == "histogram3d" for upload in uploads), 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.assertEqual(sum(upload["upload_type"] == "histogram3d" for upload in uploads), 4) | |
| self.assertGreaterEqual(sum(upload["upload_type"] == "histogram3d" for upload in uploads), 1) |
🤖 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/experiment_tests/test_comet_pytorch.py` at 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.
| self.assertTrue( | ||
| any( | ||
| upload["upload_type"] == "model-element" | ||
| and upload["additional_params"].get("groupingName") == "cdt-mnist-smoke" | ||
| and upload["additional_params"].get("fileName") == "model-data/comet-torch-model.pth" | ||
| for upload in uploads | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the Windows failure: the asserted checkpoint path hard-codes a POSIX separator.
The Windows CI job fails here. The assertion compares additional_params.get("fileName") against the literal "model-data/comet-torch-model.pth". That value is an internal path that Comet builds when it stores the checkpoint. On Windows the recorded value uses a backslash separator, so the equality check never matches and the test reports that no checkpoint was found. The Linux and macOS jobs pass because their separator matches the literal.
Normalize the separator before comparison, or assert on the basename and the parent segment separately.
🐛 Proposed fix
self.assertTrue(
any(
upload["upload_type"] == "model-element"
and upload["additional_params"].get("groupingName") == "cdt-mnist-smoke"
- and upload["additional_params"].get("fileName") == "model-data/comet-torch-model.pth"
+ and str(upload["additional_params"].get("fileName", "")).replace("\\", "/")
+ == "model-data/comet-torch-model.pth"
for upload in uploads
)
)Please confirm the recorded fileName value on the Windows runner, so the normalization matches what Comet actually emits.
#!/bin/bash
# Description: Retrieve the failing Windows job log for the Comet PyTorch offline test to confirm the recorded fileName value.
set -euo pipefail
# List recent runs of the Python experiments workflow.
gh run list --workflow python-experiments.yml --limit 10 || true
# Show the failing Windows job output around the assertion.
gh run list --workflow python-experiments.yml --limit 1 --json databaseId --jq '.[0].databaseId' \
| xargs -I{} gh run view {} --log-failed 2>/dev/null \
| rg -n -C 10 'test_offline_comet_run_records_a_pytorch_checkpoint|model-element|comet-torch-model' || true🧰 Tools
🪛 GitHub Actions: Python experiments / Windows x86-64
[error] 59-59: Unit test test_offline_comet_run_records_a_pytorch_checkpoint failed: expected a recorded PyTorch checkpoint, but no matching checkpoint was found. Command: python -m unittest scripts.experiment_tests.test_comet_pytorch
🤖 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/experiment_tests/test_comet_pytorch.py` around lines 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.
Source: Pipeline failures
| def test_training_is_replayable_on_synthetic_cpu_data(self) -> None: | ||
| """The same seed and local data produce identical metrics and weights.""" | ||
| import torch # noqa: PLC0415 | ||
|
|
||
| first = self._train_once() | ||
| second = self._train_once() | ||
| self.assertEqual(first[:3], second[:3]) | ||
| self.assertEqual(first[3].keys(), second[3].keys()) | ||
| for name in first[3]: | ||
| with self.subTest(parameter=name): | ||
| self.assertTrue(torch.equal(first[3][name], second[3][name])) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Name the returned metrics to improve failure output.
The assertions index the result tuple positionally in four places. If the test fails, assertEqual(first[:3], second[:3]) reports two anonymous tuples and does not name the metric that diverged. A NamedTuple return type removes the positional indices and makes the failure message identify the field.
♻️ Proposed refactor
import unittest
-from typing import Any
+from typing import Any, NamedTuple
from scripts.mnist_experiment import _build_model, _evaluate, _train_epoch
+
+
+class _Run(NamedTuple):
+ """One replayed training run."""
+
+ training_loss: float
+ test_loss: float
+ accuracy: float
+ state_dict: dict[str, Any] `@staticmethod`
- def _train_once() -> tuple[float, float, float, dict[str, Any]]:
+ def _train_once() -> _Run:- return training_loss, test_loss, accuracy, model.state_dict()
+ return _Run(training_loss, test_loss, accuracy, model.state_dict())- self.assertEqual(first[:3], second[:3])
- self.assertEqual(first[3].keys(), second[3].keys())
- for name in first[3]:
+ self.assertEqual(first[:3], second[:3])
+ self.assertEqual(first.state_dict.keys(), second.state_dict.keys())
+ for name in first.state_dict:
with self.subTest(parameter=name):
- self.assertTrue(torch.equal(first[3][name], second[3][name]))
+ self.assertTrue(torch.equal(first.state_dict[name], second.state_dict[name]))🤖 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/experiment_tests/test_mnist_training.py` around lines 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.
| def _dataset_manifest(dataset_root: Path) -> list[dict[str, object]]: | ||
| """Describe every retained local MNIST input file.""" | ||
| return [ | ||
| { | ||
| "bytes": path.stat().st_size, | ||
| "path": path.relative_to(dataset_root).as_posix(), | ||
| "sha256": _sha256(path), | ||
| } | ||
| for path in sorted(dataset_root.rglob("*")) | ||
| if path.is_file() | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Sort the dataset manifest by the recorded POSIX path.
The comprehension sorts Path objects. PurePath ordering is case-insensitive on Windows and case-sensitive on POSIX, so the manifest order can differ across platforms for the same dataset. The recorded strings already use as_posix(). Sort by that same key to keep run.json byte-identical across platforms.
♻️ Proposed refactor
def _dataset_manifest(dataset_root: Path) -> list[dict[str, object]]:
"""Describe every retained local MNIST input file."""
- return [
- {
- "bytes": path.stat().st_size,
- "path": path.relative_to(dataset_root).as_posix(),
- "sha256": _sha256(path),
- }
- for path in sorted(dataset_root.rglob("*"))
- if path.is_file()
- ]
+ records = [
+ {
+ "bytes": path.stat().st_size,
+ "path": path.relative_to(dataset_root).as_posix(),
+ "sha256": _sha256(path),
+ }
+ for path in dataset_root.rglob("*")
+ if path.is_file()
+ ]
+ return sorted(records, key=lambda record: record["path"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _dataset_manifest(dataset_root: Path) -> list[dict[str, object]]: | |
| """Describe every retained local MNIST input file.""" | |
| return [ | |
| { | |
| "bytes": path.stat().st_size, | |
| "path": path.relative_to(dataset_root).as_posix(), | |
| "sha256": _sha256(path), | |
| } | |
| for path in sorted(dataset_root.rglob("*")) | |
| if path.is_file() | |
| ] | |
| def _dataset_manifest(dataset_root: Path) -> list[dict[str, object]]: | |
| """Describe every retained local MNIST input file.""" | |
| records = [ | |
| { | |
| "bytes": path.stat().st_size, | |
| "path": path.relative_to(dataset_root).as_posix(), | |
| "sha256": _sha256(path), | |
| } | |
| for path in dataset_root.rglob("*") | |
| if path.is_file() | |
| ] | |
| return sorted(records, key=lambda record: record["path"]) |
🤖 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 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.
| 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.
| except ValueError as error: | ||
| print(str(error), file=sys.stderr) | ||
| return 2 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Narrow the ValueError handler so unrelated failures stay diagnosable.
This handler is intended for the "Output directory already exists" error from _staged_run_directory. It also catches every other ValueError raised during the sweep. Two reachable examples: _write_json raises ValueError for a non-finite metric because of allow_nan=False, and _experiment_provenance raises ValueError from relative_to when the resolved binary is outside the resolved repository root. Both then print a bare message with no traceback and exit 2, which hides a real defect behind a usage-style error.
Raise a dedicated exception type from _staged_run_directory and catch only that type.
♻️ Proposed change
+class OutputDirectoryExistsError(ValueError):
+ """The requested canonical output directory is already populated."""
+
+
`@contextmanager`
def _staged_run_directory(output_directory: Path) -> Iterator[Path]:
"""Publish one complete sweep without mixing it with an older generation."""
if os.path.lexists(output_directory):
message = f"Output directory already exists: {output_directory}; choose a new --output-directory."
- raise ValueError(message)
+ raise OutputDirectoryExistsError(message)- except ValueError as error:
+ except OutputDirectoryExistsError as error:
print(str(error), file=sys.stderr)
return 2OutputDirectoryExistsError subclasses ValueError, so the existing test that asserts a ValueError with "already exists" still passes.
🤖 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/optimize_initialize.py` around lines 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.
| def test_failed_run_does_not_publish_partial_artifacts(self) -> None: | ||
| """A failed generation leaves neither a final nor an incomplete run.""" | ||
| with TemporaryDirectory() as temporary_directory: | ||
| root = Path(temporary_directory) | ||
| output_directory = root / "run" | ||
| message = "training failed" | ||
| with self.assertRaisesRegex(RuntimeError, message), _staged_run_directory(output_directory) as staging_directory: | ||
| (staging_directory / "configuration.json").write_text("partial\n", encoding="utf-8") | ||
| raise RuntimeError(message) | ||
|
|
||
| self.assertFalse(output_directory.exists()) | ||
| self.assertEqual(list(root.glob(".run.incomplete-*")), []) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the cleanup assertion independent of the staging prefix.
Line 73 matches the literal glob .run.incomplete-*. That pattern duplicates the prefix format built in _staged_run_directory at scripts/mnist_experiment.py line 211. If the prefix ever changes, this glob matches nothing and the assertion passes even when a staging directory leaks. The assertion that guards cleanup would then stop guarding it.
Assert that the parent directory holds no entries at all. That holds for any prefix.
♻️ Proposed refactor
self.assertFalse(output_directory.exists())
- self.assertEqual(list(root.glob(".run.incomplete-*")), [])
+ self.assertEqual(list(root.iterdir()), [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_failed_run_does_not_publish_partial_artifacts(self) -> None: | |
| """A failed generation leaves neither a final nor an incomplete run.""" | |
| with TemporaryDirectory() as temporary_directory: | |
| root = Path(temporary_directory) | |
| output_directory = root / "run" | |
| message = "training failed" | |
| with self.assertRaisesRegex(RuntimeError, message), _staged_run_directory(output_directory) as staging_directory: | |
| (staging_directory / "configuration.json").write_text("partial\n", encoding="utf-8") | |
| raise RuntimeError(message) | |
| self.assertFalse(output_directory.exists()) | |
| self.assertEqual(list(root.glob(".run.incomplete-*")), []) | |
| def test_failed_run_does_not_publish_partial_artifacts(self) -> None: | |
| """A failed generation leaves neither a final nor an incomplete run.""" | |
| with TemporaryDirectory() as temporary_directory: | |
| root = Path(temporary_directory) | |
| output_directory = root / "run" | |
| message = "training failed" | |
| with self.assertRaisesRegex(RuntimeError, message), _staged_run_directory(output_directory) as staging_directory: | |
| (staging_directory / "configuration.json").write_text("partial\n", encoding="utf-8") | |
| raise RuntimeError(message) | |
| self.assertFalse(output_directory.exists()) | |
| self.assertEqual(list(root.iterdir()), []) |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 68-68: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
🤖 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_mnist_experiment.py` around lines 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.
| plotter.savefig.side_effect = lambda path: path.write_bytes(b"figure") | ||
| provenance = {"initializer": {"sha256": "initializer-digest"}} | ||
|
|
||
| with TemporaryDirectory() as temporary_directory, patch("builtins.print"): | ||
| output_directory = Path(temporary_directory) / "run" | ||
| services = _SweepServices(experiment_factory=experiment_factory, initializer_runner=initializer_runner, plotter=plotter) | ||
| _run_parameter_sweep(Path("initialize"), 92, output_directory, provenance, services) | ||
|
|
||
| run_directory = output_directory / "radius-1-spacing-1" | ||
| configuration_path = run_directory / "configuration.json" | ||
| figure_path = run_directory / "volume-profile.png" | ||
| run_path = run_directory / "run.json" | ||
| stdout_path = run_directory / "stdout.txt" | ||
| self.assertEqual(stdout_path.read_text(encoding="utf-8"), initializer_runner.return_value) | ||
| run_record = json.loads(run_path.read_text(encoding="utf-8")) | ||
| self.assertEqual(run_record["provenance"], provenance) | ||
| for name, path in (("configuration", configuration_path), ("figure", figure_path), ("stdout", stdout_path)): | ||
| with self.subTest(artifact=name): | ||
| record = run_record["artifacts"][name] | ||
| self.assertEqual(record["path"], path.name) | ||
| self.assertEqual(record["bytes"], path.stat().st_size) | ||
| self.assertEqual(record["sha256"], hashlib.sha256(path.read_bytes()).hexdigest()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider asserting the new figure-mirroring behavior.
The test covers the local figure artifact well. It does not assert the two new plotter interactions: plotter.clf() after each pair, and experiment.log_figure for the Comet mirror. Both are new in this PR and are currently untested.
🧪 Suggested additional assertions
for experiment, (initial_radius, foliation_spacing) in zip(experiments, PARAMETER_PAIRS, strict=True):
+ experiment.log_figure.assert_called_once_with(figure_name="Volume per Timeslice", figure=plotter)
experiment.log_parameters.assert_called_once_with(+ self.assertEqual(plotter.clf.call_count, len(PARAMETER_PAIRS))🤖 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_optimize_initialize.py` around lines 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.
| with patch("scripts.optimize_initialize.qx", side_effect=["abc123\n", " M README.md\n", b"tracked diff"]): | ||
| provenance = _experiment_provenance(repository_root, initialize_binary) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Patch shutil.which so the test does not depend on a git executable.
_experiment_provenance calls shutil.which("git") and raises RuntimeError when it returns None. This test patches only qx. In a container or sandbox without git on PATH, the test fails with "Git is required to record initializer source provenance." instead of exercising the provenance record. Patch the lookup to keep the test hermetic.
🧪 Proposed fix
- with patch("scripts.optimize_initialize.qx", side_effect=["abc123\n", " M README.md\n", b"tracked diff"]):
+ with (
+ patch("scripts.optimize_initialize.shutil.which", return_value="/usr/bin/git"),
+ patch("scripts.optimize_initialize.qx", side_effect=["abc123\n", " M README.md\n", b"tracked diff"]),
+ ):
provenance = _experiment_provenance(repository_root, initialize_binary)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with patch("scripts.optimize_initialize.qx", side_effect=["abc123\n", " M README.md\n", b"tracked diff"]): | |
| provenance = _experiment_provenance(repository_root, initialize_binary) | |
| with ( | |
| patch("scripts.optimize_initialize.shutil.which", return_value="/usr/bin/git"), | |
| patch("scripts.optimize_initialize.qx", side_effect=["abc123\n", " M README.md\n", b"tracked diff"]), | |
| ): | |
| provenance = _experiment_provenance(repository_root, initialize_binary) |
🤖 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_optimize_initialize.py` around lines 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.
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