This repository was archived by the owner on Aug 12, 2026. It is now read-only.
Repository navigation
feat(experiments)!: modernize Python experiment workflows - #145
Merged
Merged
Conversation
acgetchell
enabled auto-merge
August 2, 2026 05:40
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe repository adopts Python 3.14, replaces TensorFlow experiments with PyTorch, adds deterministic local artifacts with optional Comet integration, expands CI validation, and updates contribution and release documentation. ChangesPython experiment modernization
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Just
participant PythonExperiment
participant LocalArtifacts
participant Comet
CI->>Just: run python-experiment-check
Just->>PythonExperiment: execute offline tests and experiment checks
PythonExperiment->>LocalArtifacts: stage and publish canonical artifacts
PythonExperiment->>Comet: optionally record experiment data
LocalArtifacts-->>CI: return validated local outputs
Possibly related PRs
Suggested labels: 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
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/tests/test_justfile_discoverability.py`:
- Around line 48-54: Update the value parameter annotation in _body_fragments
from Any to object, retaining the existing isinstance narrowing and recursive
fragment extraction behavior.
🪄 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: cf70d6f9-e422-48c8-81dc-771b5079abdc
📒 Files selected for processing (5)
.github/CONTRIBUTING.mdJustfileREADME.mdscripts/tests/test_justfile_discoverability.pyscripts/tests/test_optimize_initialize.py
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 (1)
Justfile (1)
263-266: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEnsure
python-experiment-checkandpython-package-checkwork on all CI platforms.The workflow targets ubuntu-latest, macos-15, and windows-latest. These recipes use OS-specific syntax:
python-experiment-check(lines 263–266) lacks a bash shebang. On Windows, the default PowerShell shell cannot parse the shell globscripts/experiment_tests/*.pyor the POSIX environment assignment syntaxMPLCONFIGDIR="${TMPDIR:-...}".python-package-checkline 281 usesfind -maxdepth, a GNU extension not available in macOS's BSDfind. Although the recipe has#!/usr/bin/env bash, the GNU find binary is not guaranteed to be in the PATH.Add
#!/usr/bin/env bashto thepython-experiment-checkrecipe. For line 281, replacefindwith portable logic or usefindwith-type fand manual filtering, or call a helper that locates the wheel file using Python or Just.🤖 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 263 - 266, Update the Just recipes python-experiment-check and python-package-check for cross-platform CI: add the existing Bash shebang to python-experiment-check so its POSIX environment assignments and glob are interpreted by Bash, and replace python-package-check’s GNU-specific find -maxdepth usage with portable file-selection logic while preserving wheel discovery behavior.
🤖 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 263-266: Update the Just recipes python-experiment-check and
python-package-check for cross-platform CI: add the existing Bash shebang to
python-experiment-check so its POSIX environment assignments and glob are
interpreted by Bash, and replace python-package-check’s GNU-specific find
-maxdepth usage with portable file-selection logic while preserving wheel
discovery behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca3e4bd9-327c-4c2d-bcec-1d07a9095254
📒 Files selected for processing (2)
Justfilescripts/tests/test_justfile_discoverability.py
- 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
- 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
- use platform-native executable names for installed Python entry points - require every uv-backed recipe to reach the pinned version guard - keep failed-sweep cleanup checks effective and document current CTest totals
Run sanitizer builds with the repository’s Python 3.14 runtime so the vcpkg bootstrap annotations remain importable. Tighten the Justfile parser helper’s unknown-input boundary without changing its behavior.
acgetchell
force-pushed
the
feat/143-python-314-pytorch-migration
branch
from
August 5, 2026 21:17
c3f73d3 to
96baa21
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_optimize_initialize.py (1)
107-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the nine run directories are distinct.
The artifact assertions are precise, and recomputing the digest with
hashlib.sha256instead of reusing_sha256keeps the test independent of the implementation.The test inspects
radius-1-spacing-1only._run_parameter_sweepbuilds each directory name withf"radius-{initial_radius}-spacing-{foliation_spacing:g}", andrun_directory.mkdir(..., exist_ok=True)accepts a repeat. If the:gformat ever changes, two spacings could map to one name and the later pair would overwrite the earlierrun.jsonwith no test failure.Add one assertion on the directory count:
🧪 Suggested assertion
_run_parameter_sweep(Path("initialize"), 92, output_directory, provenance, services) + run_directories = sorted(path.name for path in output_directory.iterdir() if path.is_dir()) + self.assertEqual(len(run_directories), len(PARAMETER_PAIRS)) + run_directory = output_directory / "radius-1-spacing-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/tests/test_optimize_initialize.py` around lines 107 - 144, Extend the test around _run_parameter_sweep to assert that output_directory contains exactly nine distinct run directories, matching the number of PARAMETER_PAIRS. Keep the existing artifact checks unchanged and count only directories representing generated runs.
🤖 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 `@README.md`:
- Around line 238-243: Update the Testing section’s CTest count statements for
the just build and parallel configurations to match the supported
127-registration contract described above. Keep the workflow descriptions
unchanged and replace the outdated 24- and 25-entry counts consistently.
In `@scripts/experiment_tests/test_comet_pytorch.py`:
- Around line 15-67: Add a dependency-free test in the appropriate MNIST
experiment suite for the online Comet path: build an online configuration, clear
COMET_API_KEY from the environment, and assert that _start_comet raises
ValueError mentioning COMET_API_KEY before contacting the service. Reuse the
existing argument/configuration helpers and place the test in the suite
compatible with the _start_comet comet_ml import and CI dependency split.
In `@scripts/mnist_experiment.py`:
- Around line 442-456: Define an ExperimentConfigurationError subclass of
ValueError and use it for intentional configuration failures in
_config_from_args, _start_comet’s COMET_API_KEY validation, and the
MNIST-unavailable conversion around line 376. Update main to catch only
ExperimentConfigurationError, preserving exit code 2 and existing messages while
allowing unrelated ValueError failures such as _write_json serialization errors
to propagate.
In `@scripts/optimize_initialize.py`:
- Around line 202-217: The artifact and staging helpers are duplicated and
expose inconsistent exception behavior. Create a shared
scripts/experiment_artifacts.py containing _sha256, _artifact_record,
_write_json, _staged_run_directory, OutputDirectoryExistsError, and
PACKAGE_NAME; update scripts/optimize_initialize.py (202-217) and
scripts/mnist_experiment.py (442-456) to import and use those shared symbols,
remove their local definitions, and narrow mnist_experiment.py’s handler to the
intended exception types so unrelated serialization failures propagate.
- Around line 371-389: Contain optional Comet mirror failures without affecting
canonical local output: wrap experiment.log_figure in _write_volume_profile, and
the log_parameters, log_metric, log_other, and experiment.end() calls in
_run_parameter_sweep, so exceptions are handled locally while the sweep and
staged run continue. Preserve local artifact generation and cleanup behavior,
and apply the same containment consistently to every listed experiment
operation.
In `@scripts/tests/test_optimize_initialize.py`:
- Around line 163-164: Update the cleanup assertion in the relevant
optimize-initialize test to derive the staging-directory glob from the same
prefix symbol used by the implementation, matching the stronger pattern in the
MNIST test. Remove the hardcoded ".run.incomplete-" literal while preserving the
expectation that no matching staging directories remain.
- Around line 184-219: Add a clean-repository subtest to
test_experiment_provenance_hashes_the_binary_and_records_source_state by making
the mocked git status output empty while keeping the other provenance inputs
valid, then assert that _experiment_provenance sets repository["dirty"] to False
and preserves the expected record shape.
---
Outside diff comments:
In `@scripts/tests/test_optimize_initialize.py`:
- Around line 107-144: Extend the test around _run_parameter_sweep to assert
that output_directory contains exactly nine distinct run directories, matching
the number of PARAMETER_PAIRS. Keep the existing artifact checks unchanged and
count only directories representing generated runs.
🪄 Autofix
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: a3666c21-2eb6-48d9-8d5d-97d562f6554e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/feature_request.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/python-experiments.yml.python-versionJustfileREADME.mddocs/Doxyfiledocs/RELEASING.mddocs/multithreading.mdinclude/Utilities.hpppyproject.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.pysemgrep.yamltests/Foliated_triangulation_test.cpptests/Manifold_test.cppty.tomltypos.toml
💤 Files with no reviewable changes (15)
- scripts/tests/test_generate_changelog.py
- scripts/tests/test_bootstrap_vcpkg.py
- scripts/release_check.py
- scripts/tests/test_tag_release.py
- scripts/generate_changelog.py
- scripts/generate_reference_fixtures.py
- scripts/sync_vcpkg_tool_pins.py
- scripts/bootstrap_vcpkg.py
- scripts/tests/test_validate_reference_fixtures.py
- scripts/subprocess_utils.py
- scripts/tests/test_generate_reference_fixtures.py
- scripts/tests/test_sync_vcpkg_tool_pins.py
- scripts/tests/test_release_check.py
- scripts/validate_reference_fixtures.py
- scripts/tag_release.py
- share deterministic artifact staging and narrow configuration error handling - contain optional Comet startup, logging, and shutdown failures - report incomplete mirrors and align experiment CI and documentation
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@scripts/experiment_artifacts.py`:
- Around line 22-48: Rename the shared helpers _sha256, _artifact_record,
_write_json, _staging_directory_prefix, and _staged_run_directory to their
public names without leading underscores, then update all imports, re-exports,
call sites, and tests in mnist_experiment.py, optimize_initialize.py,
test_mnist_experiment.py, and test_optimize_initialize.py to use the new names
consistently.
In `@scripts/mnist_experiment.py`:
- Around line 102-110: The _start_optional_comet function has invalid
multiple-exception syntax. Update its first except clause to use a parenthesized
tuple containing ExperimentConfigurationError and ModuleNotFoundError, while
preserving the existing re-raise and fallback handling.
In `@scripts/semgrep_fixture_config.py`:
- Line 53: Update the imports used by parse_args and main so Sequence is
imported unconditionally at runtime rather than only under TYPE_CHECKING,
preventing annotation evaluation from raising NameError during module import.
In `@scripts/tests/test_mnist_experiment.py`:
- Around line 103-114: Add tests in the existing optional Comet test area for
both re-raise branches of `_start_optional_comet`: verify `ModuleNotFoundError`
and `ExperimentConfigurationError` from `_start_comet` propagate unchanged,
using offline and online configurations respectively. Import
`ExperimentConfigurationError` in the test module’s existing import block and
avoid requiring the optional dependency.
- Around line 11-23: Update the test imports so _staged_run_directory is
imported directly from its owning module, scripts.experiment_artifacts, rather
than through scripts.mnist_experiment; leave the other mnist_experiment imports
unchanged.
- Around line 149-160: Update test_existing_run_is_never_overwritten to import
and assert the dedicated OutputDirectoryExistsError raised by
_staged_run_directory, replacing the broad ValueError assertion while retaining
the existing message check and sentinel-preservation validation.
🪄 Autofix
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: 941c236a-3ad0-4a49-aa82-ce6b8cd4e2b3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/feature_request.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/python-experiments.yml.python-versionJustfileREADME.mddocs/Doxyfiledocs/RELEASING.mddocs/multithreading.mdinclude/Utilities.hpppyproject.tomlscripts/bootstrap_vcpkg.pyscripts/experiment_artifacts.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.pysemgrep.yamltests/Foliated_triangulation_test.cpptests/Manifold_test.cppty.tomltypos.toml
💤 Files with no reviewable changes (15)
- scripts/release_check.py
- scripts/bootstrap_vcpkg.py
- scripts/tests/test_generate_changelog.py
- scripts/validate_reference_fixtures.py
- scripts/generate_changelog.py
- scripts/generate_reference_fixtures.py
- scripts/subprocess_utils.py
- scripts/tests/test_generate_reference_fixtures.py
- scripts/tests/test_validate_reference_fixtures.py
- scripts/tests/test_tag_release.py
- scripts/tests/test_release_check.py
- scripts/tests/test_bootstrap_vcpkg.py
- scripts/sync_vcpkg_tool_pins.py
- scripts/tag_release.py
- scripts/tests/test_sync_vcpkg_tool_pins.py
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: Optional Python tooling now requires CPython 3.14 and uses PyTorch instead of TensorFlow for MNIST.
Closes #143
Summary by CodeRabbit