Parallelize examples with joblib (closes #5) - #65
Conversation
PR Summary
This update delivers an improved, efficient, and parallel execution process that promises increased productivity and faster task execution. It streamlines the simulation process, allowing us to handle more tasks concurrently. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## reference-values-in-examples #65 +/- ##
=============================================================
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:
|
947aef5 to
b4436ae
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe example simulations now use Joblib to process 100-election batches in parallel. Workers return partial counters for aggregation. Example tests run scripts in subprocesses and compare complete serialized tables. ChangesParallel example simulations
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ExampleScript
participant JoblibParallel
participant simulate_batch
participant ElectionMethods
participant ResultAggregation
ExampleScript->>JoblibParallel: create delayed batch jobs
JoblibParallel->>simulate_batch: execute election batches
simulate_batch->>ElectionMethods: simulate elections and calculate winners
ElectionMethods-->>simulate_batch: return election results
simulate_batch-->>JoblibParallel: return partial counters
JoblibParallel-->>ResultAggregation: provide batch results
ResultAggregation->>ExampleScript: aggregate counters for tables and figures
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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 `@examples/README.md`:
- Line 10: Update the Parallel example description so n_jobs=-3 says it uses all
but two CPU cores, and state that n_jobs=-4 should be used when leaving three
cores unused.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f3d2c995-ab62-403f-a7ea-c65898d78aa3
📒 Files selected for processing (12)
.pre-commit-config.yamlexamples/README.mdexamples/merrill_1984_fig_2c_2d.pyexamples/merrill_1984_fig_2c_2d_updated.pyexamples/merrill_1984_fig_4a_4b.pyexamples/merrill_1984_fig_4a_4b_updated.pyexamples/merrill_1984_table_1_fig_1.pyexamples/merrill_1984_table_2.pyexamples/merrill_1984_table_3_fig_3.pyexamples/merrill_1984_table_4.pyexamples/weber_1977_effectiveness_table.pyexamples/weber_1977_table_4.py
|
Rebased onto current Rebase
Verification — serial vs parallelized, byte-identical output
"Identical" = normalized stdout (excluding Caveat: the CI sandbox where this was run is a single-CPU container, so actual process-level parallelism ( Also added |
|
Follow-up on |
|
@coderabbitai review |
✅ Action performedReview finished.
|
b4436ae to
b6c8f13
Compare
|
The head branch has been force-updated to the rebased history (4 commits on top of current |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/python-package.yml (1)
25-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSet
persist-credentials: falseonactions/checkoutsteps.None of the three
actions/checkout@v7steps in this file setpersist-credentials: false. By default, checkout persists theGITHUB_TOKENin the local git config. None of these jobs need to push commits or run authenticated git commands afterward. Persisting the token unnecessarily increases the impact of a compromised dependency step that can read local git config.🔒 Proposed fix (apply to each of the three checkout steps)
- uses: actions/checkout@v7 + with: + persist-credentials: falseAlso applies to: 55-55, 96-96
🤖 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 @.github/workflows/python-package.yml at line 25, Update all three actions/checkout@v7 steps in the workflow to set persist-credentials to false. Apply the setting to each checkout invocation, including the steps referenced near lines 25, 55, and 96, without changing other job behavior.Source: Linters/SAST tools
🧹 Nitpick comments (1)
tests/test_examples.py (1)
78-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve
os.environwhen building the subprocess environment.
envhere fully replaces the parent process environment instead of extending it. This drops variables such asHOME,TMPDIR, andLANGthat some libraries use for cache/config resolution, and hardcodesPATHto/usr/bin:/bin, which does not match typical macOS (Homebrew) or Windows layouts. This risks fragile or failing test runs outside the pinnedubuntu-latestCI job.♻️ Proposed fix
- env = {'MPLBACKEND': 'Agg', - 'PYTHONPATH': str(EXAMPLES), - 'PATH': '/usr/bin:/bin'} + env = {**os.environ, + 'MPLBACKEND': 'Agg', + 'PYTHONPATH': str(EXAMPLES)}🤖 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 78 - 83, Update the subprocess environment construction in the test invoking subprocess.run to start from os.environ.copy(), then override MPLBACKEND and PYTHONPATH while preserving the inherited PATH and other environment variables; remove the hardcoded PATH value.
🤖 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 40-46: Update the worker setup around SEED_BLOCK and the
Parallel(n_jobs=-3) batch execution so every Joblib worker receives an explicit
seed or RNG rather than relying on the parent-process assignment to
elsim.elections.elections_rng. Ensure _check_random_state(None) observes the
worker-specific initialized RNG, using per-batch seed propagation or a Joblib
worker initializer.
---
Outside diff comments:
In @.github/workflows/python-package.yml:
- Line 25: Update all three actions/checkout@v7 steps in the workflow to set
persist-credentials to false. Apply the setting to each checkout invocation,
including the steps referenced near lines 25, 55, and 96, without changing other
job behavior.
---
Nitpick comments:
In `@tests/test_examples.py`:
- Around line 78-83: Update the subprocess environment construction in the test
invoking subprocess.run to start from os.environ.copy(), then override
MPLBACKEND and PYTHONPATH while preserving the inherited PATH and other
environment variables; remove the hardcoded PATH value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 076731a9-75ff-493a-9853-6b65f059b0a1
📒 Files selected for processing (13)
.github/workflows/python-package.ymlexamples/README.mdexamples/merrill_1984_fig_2c_2d.pyexamples/merrill_1984_fig_2c_2d_updated.pyexamples/merrill_1984_fig_4a_4b.pyexamples/merrill_1984_fig_4a_4b_updated.pyexamples/merrill_1984_table_1_fig_1.pyexamples/merrill_1984_table_2.pyexamples/merrill_1984_table_3_fig_3.pyexamples/merrill_1984_table_4.pyexamples/weber_1977_effectiveness_table.pyexamples/weber_1977_table_4.pytests/test_examples.py
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/merrill_1984_fig_4a_4b_updated.py
- examples/merrill_1984_fig_2c_2d.py
- examples/merrill_1984_table_4.py
- examples/merrill_1984_table_3_fig_3.py
- examples/merrill_1984_fig_2c_2d_updated.py
- examples/merrill_1984_table_2.py
- examples/merrill_1984_fig_4a_4b.py
- examples/weber_1977_table_4.py
2123178 to
fc5dc63
Compare
Document two PR workflow rules that keep history clean: - Rework an existing PR in place: back up the head branch locally, then modify and force-push the PR's actual head branch instead of creating a new parallel branch. - Fold follow-up fixes into the commit that caused the problem (fixup) rather than stacking fix commits on top. These came out of rebasing PR #65: the CI fix should have been squashed into the commit that introduced the job, and the rebase should have been applied to the PR-attached branch from the start.
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 @.github/workflows/python-package.yml:
- Line 96: Update the actions/checkout@v7 step in the smoke-test job to set
persist-credentials to false, ensuring repository credentials are not retained
while pytest executes repository code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce7bfffa-a465-4228-b6c3-372b7705a58e
📒 Files selected for processing (2)
.github/workflows/python-package.ymltests/test_examples.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_examples.py
Document two PR workflow rules that keep history clean: - Rework an existing PR in place: back up the head branch locally, then modify and force-push the PR's actual head branch instead of creating a new parallel branch. - Fold follow-up fixes into the commit that caused the problem (fixup) rather than stacking fix commits on top. These came out of rebasing PR #65: the CI fix should have been squashed into the commit that introduced the job, and the rebase should have been applied to the PR-attached branch from the start.
71fe6a2 to
fa94950
Compare
Reviewers can flag pre-existing or out-of-scope issues; those belong in their own PRs rather than being folded into the PR under review. This came from CodeRabbit flagging unrelated CI hardening on #65, which was moved to its own PR.
fa94950 to
7966c66
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/merrill_1984_table_2.py (2)
50-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse explicit validation for the batch partition.
assertis removed underpython -O, so Line 53 is not a runtime guarantee. A non-divisible configuration would use the floor from Line 52 and silently simulate fewer elections. Validate thatbatch_sizeis positive and dividesn_electionsbefore derivingn_batches. Python documents that optimization removesassertstatements. (docs.python.org)Proposed fix
batch_size = 100 -n_batches = n_elections // batch_size -assert n_batches * batch_size == n_elections +if batch_size <= 0 or n_elections % batch_size != 0: + raise ValueError( + "n_elections must be divisible by a positive batch_size" + ) +n_batches = n_elections // batch_size🤖 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 `@examples/merrill_1984_table_2.py` around lines 50 - 53, Replace the assert-based validation near batch_size and n_batches with explicit runtime validation that batch_size is positive and n_elections is evenly divisible by it, raising an appropriate exception for invalid configuration before calculating n_batches.
117-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the backend or pass per-batch RNG state.
Paralleldefaults toloky, so fork-state duplication does not affect the normal path. However, callers can selectmultiprocessing, whilesimulate_batchuses module-globalelections_rngand Python’s globalrandomstate. Pinbackend='loky', or pass distinct RNG state to each batch. The current tests only check completion and non-empty output.🤖 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 `@examples/merrill_1984_table_2.py` around lines 117 - 119, Update the Parallel invocation in the batch execution flow to explicitly use the loky backend, preserving independent random-state behavior when simulate_batch accesses elections_rng and Python’s global random state. Keep the existing job construction and parallelism settings 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.
Nitpick comments:
In `@examples/merrill_1984_table_2.py`:
- Around line 50-53: Replace the assert-based validation near batch_size and
n_batches with explicit runtime validation that batch_size is positive and
n_elections is evenly divisible by it, raising an appropriate exception for
invalid configuration before calculating n_batches.
- Around line 117-119: Update the Parallel invocation in the batch execution
flow to explicitly use the loky backend, preserving independent random-state
behavior when simulate_batch accesses elections_rng and Python’s global random
state. Keep the existing job construction and parallelism settings unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a5fb59d-d676-42f9-8c9b-46ce17b5a440
📒 Files selected for processing (13)
.github/workflows/python-package.ymlexamples/README.mdexamples/merrill_1984_fig_2c_2d.pyexamples/merrill_1984_fig_2c_2d_updated.pyexamples/merrill_1984_fig_4a_4b.pyexamples/merrill_1984_fig_4a_4b_updated.pyexamples/merrill_1984_table_1_fig_1.pyexamples/merrill_1984_table_2.pyexamples/merrill_1984_table_3_fig_3.pyexamples/merrill_1984_table_4.pyexamples/weber_1977_effectiveness_table.pyexamples/weber_1977_table_4.pytests/test_examples.py
🚧 Files skipped from review as they are similar to previous changes (10)
- examples/weber_1977_table_4.py
- examples/merrill_1984_fig_2c_2d_updated.py
- examples/merrill_1984_fig_4a_4b.py
- examples/merrill_1984_table_4.py
- examples/merrill_1984_table_1_fig_1.py
- .github/workflows/python-package.yml
- examples/weber_1977_effectiveness_table.py
- examples/merrill_1984_fig_2c_2d.py
- examples/merrill_1984_table_3_fig_3.py
- examples/merrill_1984_fig_4a_4b_updated.py
7966c66 to
640bb08
Compare
Document two PR workflow rules that keep history clean: - Rework an existing PR in place: back up the head branch locally, then modify and force-push the PR's actual head branch instead of creating a new parallel branch. - Fold follow-up fixes into the commit that caused the problem (fixup) rather than stacking fix commits on top. These came out of rebasing PR #65: the CI fix should have been squashed into the commit that introduced the job, and the rebase should have been applied to the PR-attached branch from the start.
Reviewers can flag pre-existing or out-of-scope issues; those belong in their own PRs rather than being folded into the PR under review. This came from CodeRabbit flagging unrelated CI hardening on #65, which was moved to its own PR.
640bb08 to
31b2827
Compare
Two rules that PR #65's history violated: - Code changes, their tests, and the CI that runs them are separate commits; don't mix them. - Agent-generated summary files must not be committed; if one slips in, remove it from history rather than adding a delete commit. Also drop the n_jobs=-4 note from examples/README.md in #65 (unnecessary).
Two commit-hygiene rules that PR #65's history violated: - Code changes, their tests, and the CI that runs them are separate commits; don't mix them. - Agent-generated summary files must not be committed; if one slips in, remove it from history rather than adding a delete commit.
Agent-generated summary files must not be committed; if one slips in, remove it from history rather than adding a delete commit. PR #65's history violated this. Also drop the separate code/tests/CI commits rule that contradicted the existing 'one coherent idea' guideline: tests, documentation, and CI/workflow changes for an idea belong in the same commit as the code they describe.
Document two PR workflow rules that keep history clean: - Rework an existing PR in place: push a backup of the head branch to the remote and confirm it exists, then modify the PR's actual head branch and force-push with --force-with-lease instead of creating a new parallel branch. - Fold follow-up fixes into the commit that caused the problem (fixup) rather than stacking fix commits on top. These came out of rebasing PR #65: the CI fix should have been squashed into the commit that introduced the job, and the rebase should have been applied to the PR-attached branch from the start.
Reviewers can flag pre-existing or out-of-scope issues; those belong in their own PRs rather than being folded into the PR under review. This came from CodeRabbit flagging unrelated CI hardening on #65, which was moved to its own PR.
Agent-generated summary files must not be committed; if one slips in, remove it from history rather than adding a delete commit. PR #65's history violated this. Also drop the separate code/tests/CI commits rule that contradicted the existing 'one coherent idea' guideline: tests, documentation, and CI/workflow changes for an idea belong in the same commit as the code they describe.
31b2827 to
959577f
Compare
|
Addressed CodeRabbit's backend/RNG comment: pinned |
959577f to
244077a
Compare
fecf7e3 to
d58ec07
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_examples.py (1)
150-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftValidate all parallelized scripts.
Four scripts in
ALL_SCRIPTShave no entries inREFERENCE_VALUESorTOLERANCES. For those scripts, this test accepts any nonemptytable. Add expected rows and tolerances, or add explicit structural assertions for every row and column.🤖 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 150 - 155, Extend the validation loop around REFERENCE_VALUES and TOLERANCES so every script listed in ALL_SCRIPTS is validated, including the four currently missing entries. Add their expected rows and tolerances, or provide explicit assertions covering every row and column; do not allow scripts to pass based only on a nonempty table.
🤖 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.
Nitpick comments:
In `@tests/test_examples.py`:
- Around line 150-155: Extend the validation loop around REFERENCE_VALUES and
TOLERANCES so every script listed in ALL_SCRIPTS is validated, including the
four currently missing entries. Add their expected rows and tolerances, or
provide explicit assertions covering every row and column; do not allow scripts
to pass based only on a nonempty table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9b2553c-f493-4ec9-ba20-4d397ee48b46
📒 Files selected for processing (12)
examples/README.mdexamples/merrill_1984_fig_2c_2d.pyexamples/merrill_1984_fig_2c_2d_updated.pyexamples/merrill_1984_fig_4a_4b.pyexamples/merrill_1984_fig_4a_4b_updated.pyexamples/merrill_1984_table_1_fig_1.pyexamples/merrill_1984_table_2.pyexamples/merrill_1984_table_3_fig_3.pyexamples/merrill_1984_table_4.pyexamples/weber_1977_effectiveness_table.pyexamples/weber_1977_table_4.pytests/test_examples.py
🚧 Files skipped from review as they are similar to previous changes (8)
- examples/merrill_1984_table_4.py
- examples/merrill_1984_fig_4a_4b_updated.py
- examples/merrill_1984_table_3_fig_3.py
- examples/weber_1977_effectiveness_table.py
- examples/merrill_1984_fig_4a_4b.py
- examples/merrill_1984_table_1_fig_1.py
- examples/weber_1977_table_4.py
- examples/merrill_1984_fig_2c_2d.py
Each example script now defines its own ``reference_table`` (the values its computed ``table`` is checked against, in the script's column order) and a ``tolerance`` (absolute comparison tolerance), so the script doubles as a test. test_examples.py no longer hardcodes REFERENCE_VALUES/TOLERANCES; it just runs each script (in a subprocess) and verifies ``table`` against the script's own ``reference_table``. Fixes #91. Reference provenance (see issue #88): - merrill_1984_table_1/3 and the Weber scripts reproduce the published tables, so their references are the paper's values (for table_1 and table_3 this is the existing ``merrill_table_1`` dict, renamed ``reference_table``). - merrill_1984_table_2/4 and the four figure scripts do not reproduce the papers (up to ~7-9 pp off), so those references are the docstring "Typical result"/"Results with N elections" values, with a comment noting they are a regression guard until the discrepancy is fixed. The figure scripts were restructured to keep a table per sub-figure (keyed by fig label) instead of overwriting a single ``table`` each loop iteration, so both sub-figures are checked. The examples README documents the convention. Co-authored-by: opencode <opencode@anomalyco.ai>
Refactor the Monte Carlo example scripts to run elections in batches (simulate_batch) executed by joblib.Parallel with n_jobs=-3 and the loky backend, aggregating the per-worker Counter results in the main process. Pin backend='loky' so each worker is a fresh interpreter with its own RNG state; fork-based backends would inherit and duplicate the module-global elections_rng across workers. joblib already prints elapsed time, so the manual timing is removed. Document the pattern in examples/README.md.
d58ec07 to
0352683
Compare
1 participant
Summary
Closes #5. Adds joblib parallelization to the remaining Monte Carlo example scripts that still ran serial election loops, and documents the shared parallelism pattern in
examples/README.md.Changes
Newly parallelized (10 scripts): Merrill tables/figures and Weber effectiveness/table scripts now use the same
batch_size/Parallel(n_jobs=-3)pattern as the examples that already had joblib.Documentation:
examples/README.mdexplains batching, worker count, result aggregation, and seed behavior (Monte Carlo scripts intentionally omit fixed seeds; parallelism preserves the same statistical intent as a serial loop).Scope:
examples/only — no library or test changes.Reproducibility review
Counter/defaultdicttotals merged on the main process; completion order does not affect aggregates.random_utilities,impartial_culture,normal_electorate,np.random,tiebreaker='random').CI
ruff check examples/ --select=E9,F63,F7,F82— passpytest— passOverlap with PR #52
PR #52 (
elsim.studiesAPI) also refactors the same example files to useJoblibBackendinstead of directjoblib.Parallelcalls. Only one of these PRs should merge as-is — whichever lands second will need a rebase and either adoption ofelsim.studiesor manual conflict resolution. This PR is intentionally scoped toexamples/with direct joblib usage to close #5 without depending on the studies API.Summary by CodeRabbit
New Features
Documentation
Tests