fix: name the missing price CSV instead of raising a polars traceback - #525
Conversation
…#524) `optimize.py` is a documented user-facing CLI, but its one environmental failure — an absent `public/Prices_hashed.csv` — reached the user as a raw polars read error naming neither the file the loader looked for nor the fact that it ships with the repository. `load_prices` now checks for the file before handing it to `pl.read_csv` and raises `FileNotFoundError` with the resolved path and the remedy, so the notebooks that call it directly get the clear error too. `main` returns an exit code, catches that error around the study loop, and prints a one-line `error:` diagnostic to stderr; the entry guard becomes `sys.exit(main())`. Both new branches are covered, keeping the 100% line-and-branch gate: the loader test uses `tmp_path` as a caller outside the notebook directory, and the CLI test patches the module-global `optimize` via `__globals__` — the technique the singular-matrix and zero-baseline tests already use — so the failure is raised without touching the CSV the rest of the suite reads. The new doctest is written as `try`/`except` rather than a `Traceback` block so it carries no platform-specific path separators. Closes #524 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves CLI/user-facing error handling for the marimo experiment notebooks by turning a missing public/Prices_hashed.csv into a clear FileNotFoundError message (naming the expected path) and ensuring the optimize.py entry point reports it cleanly with a non-zero exit status.
Changes:
- Add a preflight existence check in
preamble.load_prices(withPRICES_CSVas a module constant) to raise an actionableFileNotFoundErrorbeforepolars.read_csvruns. - Update
optimize.mainto return an exit code, catch missing-priceFileNotFoundError, print a one-lineerror:message to stderr, andsys.exit(main()). - Extend tests to cover the new missing-CSV branches in both
load_pricesandmain.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| book/marimo/notebooks/preamble.py | Introduces PRICES_CSV and raises a clearer FileNotFoundError when the price CSV is absent (plus a doctest for the branch). |
| book/marimo/notebooks/optimize.py | Returns 0/1, catches FileNotFoundError to emit a concise stderr diagnostic, and exits via sys.exit(main()). |
| tests/test_preamble.py | Adds a test for the missing-price-CSV branch of load_prices. |
| tests/test_optimize.py | Updates main test to assert exit code and adds a missing-price-data test to ensure no traceback leaks to stderr. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| with pytest.raises(FileNotFoundError, match=PRICES_CSV): | ||
| load_prices(str(tmp_path / "Experiment1.py")) |
| captured = capsys.readouterr() | ||
| assert "Prices_hashed.csv" in captured.err | ||
| assert "Traceback" not in captured.err |
Closes #524.
The problem
book/marimo/notebooks/optimize.pyis a documented user-facing CLI (README§ Running the Experiments) and its argparse surface is good. But its one
environmental failure was unhandled: an absent
book/marimo/notebooks/public/Prices_hashed.csvreached the user as a rawpolars read error out of
preamble.load_prices, naming neither the file theloader looked for nor the fact that it ships with the repository.
That was the only subcategory scoring below 10 in the last
/rhiza:qualityrun (error handling & CLI UX, 9 → 10).
The fix
preamble.py—PRICES_CSVextracted as a module constant, andload_priceschecks for the file before handing it topl.read_csv:The guard lives in the loader rather than only in the CLI so the notebooks,
which call
load_prices(__file__)directly, get the same clear error.optimize.py—mainnow returns an exit code (0/1), catchesFileNotFoundErroraround the study loop, prints a one-lineerror:diagnostic to stderr, and the entry guard becomes
sys.exit(main()).This matches the idiom the repo-local gate scripts already use
(
scripts/check_test_layout.py:144).Tests
One test per new branch, keeping the 100% line-and-branch gate intact:
test_load_prices_missing_csv_names_the_expected_pathusestmp_pathas acaller sitting outside the notebook directory, so the
public/sibling doesnot exist.
test_main_reports_missing_price_data_without_a_tracebackpatches themodule-global
optimizevia__globals__— the technique thesingular-matrix and zero-baseline tests already use — so the failure is
raised from inside the loop without deleting the CSV the rest of the suite
reads. It asserts exit
1, the filename in stderr, and noTraceback.The new doctest is written as
try/exceptrather than aTracebackblockso it carries no platform-specific path separators (cf. 41a32e4).
Verification
End-to-end against a copy of the notebook directory with no
public/:Gates re-run locally, all green:
make fmtmake typecheckty+mypy --strictcleanmake testmake rhiza-test🤖 Generated with Claude Code