test: run example scripts in subprocesses so joblib workers can pickle - #90
Conversation
PR Summary
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe example tests now run each script in an isolated subprocess. They serialize and reload the computed ChangesExample test execution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #90 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 17 17
Lines 496 496
=======================================
Hits 478 478
Misses 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_examples.py (2)
132-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the child traceback in test failures.
capture_output=Truestores child output, but this helper does not reportCalledProcessError.stdoutorCalledProcessError.stderr. A failing example therefore exposes only the exit status. Preserve the captured output in the failure message or let the child inherit its output streams.Suggested failure handling
- subprocess.run([sys.executable, str(variant)], env=env, - capture_output=True, timeout=1200, check=True) + try: + subprocess.run([sys.executable, str(variant)], env=env, + capture_output=True, text=True, + timeout=1200, check=True) + except subprocess.CalledProcessError as exc: + pytest.fail(f'{name} failed:\n{exc.stderr}\n{exc.stdout}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_examples.py` around lines 132 - 133, Update the subprocess invocation in the example-test helper to retain child stdout and stderr in failures: either report CalledProcessError.stdout and CalledProcessError.stderr when check=True raises, or stop capturing output so the child traceback is visible directly. Preserve the existing timeout and successful-execution behavior.
131-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the inherited
PYTHONPATH.Line 131 replaces the caller's
PYTHONPATHwith onlyEXAMPLES. A source checkout or CI job that usesPYTHONPATHfor project packages can fail to import those packages in the child process. PrependEXAMPLESand retain existing entries, unless repository setup guarantees an installed package.Suggested environment construction
- env = {**os.environ, 'MPLBACKEND': 'Agg', 'PYTHONPATH': str(EXAMPLES)} + inherited_pythonpath = os.environ.get('PYTHONPATH') + pythonpath = os.pathsep.join( + path for path in (str(EXAMPLES), inherited_pythonpath) if path) + env = {**os.environ, 'MPLBACKEND': 'Agg', 'PYTHONPATH': pythonpath}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_examples.py` at line 131, Update the environment construction around the env assignment to prepend EXAMPLES to any inherited PYTHONPATH instead of replacing it. Preserve all existing PYTHONPATH entries, using the platform’s path separator when joining them, while retaining the MPLBACKEND value and other inherited environment variables.
🤖 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 `@tests/test_examples.py`:
- Around line 8-9: Update the documentation in tests/test_examples.py to
reference TOLERANCES separately from REFERENCE_VALUES, directing maintainers to
the correct definition for tolerance values while preserving the
reference-values guidance.
---
Nitpick comments:
In `@tests/test_examples.py`:
- Around line 132-133: Update the subprocess invocation in the example-test
helper to retain child stdout and stderr in failures: either report
CalledProcessError.stdout and CalledProcessError.stderr when check=True raises,
or stop capturing output so the child traceback is visible directly. Preserve
the existing timeout and successful-execution behavior.
- Line 131: Update the environment construction around the env assignment to
prepend EXAMPLES to any inherited PYTHONPATH instead of replacing it. Preserve
all existing PYTHONPATH entries, using the platform’s path separator when
joining them, while retaining the MPLBACKEND value and other inherited
environment variables.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5e2bf1-c518-46c1-993d-5bbd05d73309
📒 Files selected for processing (1)
tests/test_examples.py
Running the scripts with runpy.run_path gives them the module name '<run_path>', which loky workers cannot import, breaking the parallel path in CI (ModuleNotFoundError: No module named '<run_path>'). Run each script in a subprocess as a real __main__ instead, and have it dump its table to a pickle the test reads back, so values are still checked directly without parsing printed output. Also assert each table is non-empty and every reference row has the expected number of values, instead of silently truncating.
be58012 to
c42770e
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: 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 `@tests/test_examples.py`:
- Around line 147-155: Add a concise docstring to the test_example function
documenting that it verifies non-empty output and complete reference rows, and
that these checks prevent silent truncation; do not alter the test logic.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eccfc325-418b-4edd-bbeb-4bc2cd9d85d2
📒 Files selected for processing (1)
tests/test_examples.py
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Add a docstring explaining that the example scripts are run in a subprocess and their computed table checked against REFERENCE_VALUES within TOLERANCES, and that the non-empty/equal-length assertions make silent output truncation fail with a clear message. Co-authored-by: opencode <opencode@anomalyco.ai>
5e22493 to
e088bcd
Compare
1 participant
Problem
The example tests run the scripts with
runpy.run_path, which gives them the module name'<run_path>'. When the script callsParallel(backend='loky'), loky workers can't unpickle the batch function and CI fails withModuleNotFoundError: No module named '<run_path>'. This only shows up with real parallelism, so it wasn't caught locally.What this PR changes
__main__(the way joblib/loky expects), appending apickle.dump(table, ...)so the test still reads the computedtabledirectly instead of parsing printed output.[:len(expected)]silently truncated extra columns).Also supersedes #89 (its length-assertion change is folded in here).
Tests
The example tests themselves (
pytest -m slow); verified the two Weber scripts pass with the new mechanism, and CI now exercises the real loky parallel path.Summary by CodeRabbit