feat(week-3): implement Non-IID Dirichlet simulation, client drift memo, and interactive dashboard#2
Open
koleajeolayinka wants to merge 14 commits into
Open
feat(week-3): implement Non-IID Dirichlet simulation, client drift memo, and interactive dashboard#2koleajeolayinka wants to merge 14 commits into
koleajeolayinka wants to merge 14 commits into
Conversation
…g into feature/week-3-non-iid
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (12)
📝 WalkthroughWalkthroughChangesFederated Learning Week 3
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant main.py
participant load_data
participant LocalClient
participant FedAvg
participant ResultsCSV
main.py->>load_data: load IID or Non-IID client data
main.py->>LocalClient: send global model parameters
LocalClient->>FedAvg: return locally trained parameters and sample counts
FedAvg->>main.py: compute weighted global parameters
main.py->>ResultsCSV: record round accuracy and loss
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
week-3/walkthrough/index.html-149-153 (1)
149-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRepresent Client 3 with a distinct legend color.
The simulator renders three client minima, but this legend lists only Clients 1 and 2. Client 3 also uses the same green as the global optimum, making the visualization ambiguous.
🤖 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 `@week-3/walkthrough/index.html` around lines 149 - 153, Update the landscape-legend markup to add a Client 3 Min legend item with a distinct color class matching Client 3’s rendered minimum, and ensure it does not reuse the global-min color. Preserve the existing Global Optimum and Client 1/2 entries.week-3/walkthrough/app.js-622-630 (1)
622-630: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle charts containing only round 1.
When both uploads contain one row at round 1,
maxR - 1is zero and all x-coordinates becomeNaN.const maxR = Math.max(/* ... */); +const roundSpan = Math.max(maxR - 1, 1); -const x = paddingLeft + (chartW * (r - 1) / (maxR - 1)); +const x = paddingLeft + (chartW * (r - 1) / roundSpan);Apply the same denominator in
mapCoords.Also applies to: 650-653
🤖 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 `@week-3/walkthrough/app.js` around lines 622 - 630, Update the round-label coordinate calculation in the max-round drawing loop and the corresponding mapCoords logic to handle maxR equal to 1 without dividing by zero. Use the same safe denominator in both locations, while preserving normal spacing for charts with multiple rounds.week-3/walkthrough/app.js-114-118 (1)
114-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the selected alpha preset state.
setAlphaPresetaddsactive, butupdateDirichletSplitimmediately removes it and the empty conditional never restores it. The default preset is also cleared during initialization.Proposed fix
document.querySelectorAll('.preset-btn').forEach(btn => { - btn.classList.remove('active'); + btn.classList.toggle('active', parseFloat(btn.innerText) === alpha); }); -if (alpha === 0.10) { - // match preset 0.1 -}Also applies to: 180-188
🤖 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 `@week-3/walkthrough/app.js` around lines 114 - 118, Update updateDirichletSplit and the related setAlphaPreset flow so updateDirichletSplit does not unconditionally remove the active class from preset buttons. Preserve the selected alpha preset, including the default alpha === 0.10 state during initialization, by restoring or maintaining the matching .preset-btn active state in the existing conditional.week-3/walkthrough/style.css-90-93 (1)
90-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the reported Stylelint errors.
Rename the keyframes to kebab-case and update their uses, change
currentColortocurrentcolor, and remove the redundantdisplay: inline-blockfrom.option-marker.Also applies to: 138-138, 155-162, 884-893, 926-929
🤖 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 `@week-3/walkthrough/style.css` around lines 90 - 93, Resolve the Stylelint issues in style.css: rename the rotate🍭 keyframes identifier to kebab-case and update its animation use, change currentColor declarations to currentcolor, and remove the redundant display: inline-block from .option-marker. Apply these changes consistently to all referenced locations.Source: Linters/SAST tools
week-3/walkthrough/app.js-457-461 (1)
457-461: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the simulation state when the interval finishes.
The button text resets, but
isSimulatingremainstrue. Consequently, the next click only resets the simulation instead of starting another round.clearInterval(driftSimInterval); +driftSimInterval = null; +driftState.isSimulating = false; document.getElementById('drift-play-text').innerText = 'Simulate Round';🤖 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 `@week-3/walkthrough/app.js` around lines 457 - 461, Update the simulation-finished branch that clears driftSimInterval and resets drift-play-text to also set isSimulating to false, allowing the next button click to start a new round.week-3/walkthrough/app.js-615-619 (1)
615-619: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the CSS custom property before assigning
fillStyle.chartCtx.fillStyle = 'var(--text-secondary)'won’t apply in canvas, so both label blocks should use a computed color string instead.🤖 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 `@week-3/walkthrough/app.js` around lines 615 - 619, Update both canvas label blocks around the Acc labels to resolve the --text-secondary CSS custom property to a computed color string before assigning chartCtx.fillStyle; replace the direct var(--text-secondary) assignment in each block while preserving the existing label rendering behavior.week-3/fed/client.py-39-52 (1)
39-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompute evaluation loss as a true per-example mean.
Both runners average batch means, which overweights partial batches.
week-3/fed/client.py#L39-L52: accumulateloss.item() * labels.size(0)and divide bytotal.week-3/main.py#L99-L119: apply the same calculation before weighting client losses.🤖 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 `@week-3/fed/client.py` around lines 39 - 52, Compute evaluation loss as a per-example mean in evaluate: accumulate each batch loss multiplied by labels.size(0), then divide loss_sum by total instead of len(self.trainloader). Apply the same loss accumulation and normalization in week-3/main.py lines 99-119 before client-loss weighting; no direct change is required elsewhere.week-3/main.py-158-172 (1)
158-172: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport single-mode plot failures.
Both
exceptblocks silently hide unexpected plotting failures. Print the exception as done in thebothbranch so local verification remains diagnosable.Proposed fix
- except Exception as e: - pass + except Exception as e: + print(f"Could not generate plot automatically: {e}")🤖 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 `@week-3/main.py` around lines 158 - 172, Update the exception handlers in the single-mode branches of the main argument dispatch to report plotting failures instead of silently passing. In both the args.mode == "iid" and args.mode == "noniid" blocks, print the caught exception, matching the existing behavior in the "both" branch.Source: Linters/SAST tools
week-3/MEMO.md-11-11 (1)
11-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the accuracy gap in percentage points.
77.75% - 57.63%is a 20.12 percentage-point gap. Calling it 20.12% incorrectly implies a relative difference.-—representing a performance gap of **20.12%**. +—representing a performance gap of **20.12 percentage points**.🤖 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 `@week-3/MEMO.md` at line 11, Update the “Initial Round Contrast” statement in the Round 1 comparison to label the 20.12 difference between 77.75% and 57.63% as percentage points, not percent; leave the reported accuracy and loss values unchanged.week-3/plot.py-82-99 (1)
82-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSupport one-round and partial simulation logs.
When the only available log contains Round 1,
max_round - 1is zero at Lines 85 and 98, causing plot generation to crash.max_round = max((iid_rounds or [10]) + (noniid_rounds or [10])) + round_span = max(max_round - 1, 1) for r in range(1, max_round + 1): - x = pad_left + int(chart_w * (r - 1) / (max_round - 1)) + x = pad_left + int(chart_w * (r - 1) / round_span) def map_coords(round_num, acc): - x = pad_left + int(chart_w * (round_num - 1) / (max_round - 1)) + x = pad_left + int(chart_w * (round_num - 1) / round_span)🤖 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 `@week-3/plot.py` around lines 82 - 99, Update the round-to-x-coordinate calculations in the plotting loop and map_coords so a max_round of 1 does not divide by zero. Use a zero horizontal offset for the single-round case while preserving the existing scaling for multi-round logs, including partial simulation data.week-3/main_flower.py-86-92 (1)
86-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-positive Dirichlet alpha values at the CLI boundary.
Dirichlet concentration parameters must be positive; values such as
--alpha 0currently propagate into partitioning and fail during execution.args = parser.parse_args() + if args.mode in {"noniid", "both"} and args.alpha <= 0: + parser.error("--alpha must be greater than zero")🤖 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 `@week-3/main_flower.py` around lines 86 - 92, Validate the --alpha argument in the argument parser before execution, rejecting zero and negative values while retaining the existing default and float parsing. Update the parser configuration around parser.add_argument and ensure invalid values produce a clear CLI error instead of reaching partitioning.week-3/README.md-87-108 (1)
87-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint the Flower/Ray instructions to
main_flower.py.The isolated subprocesses, Ray handling, CSV logging, and automatic plotting shown here are implemented by
main_flower.py, not the standalone PyTorch runner described formain.py.Proposed documentation correction
-The simulation runner in `main.py` is configured to run both IID and Non-IID setups sequentially inside isolated subprocesses. +The Flower runner in `main_flower.py` runs both setups sequentially inside isolated subprocesses. -python main.py +python main_flower.py -python main.py --mode iid +python main_flower.py --mode iid -python main.py --mode noniid --alpha 0.1 +python main_flower.py --mode noniid --alpha 0.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 `@week-3/README.md` around lines 87 - 108, Update the Step 2 experiment commands and descriptions to reference main_flower.py instead of main.py. Ensure the IID, Non-IID, custom-alpha, and automatic plotting instructions match the Flower/Ray runner implemented by main_flower.py, while keeping standalone main.py usage separate.
🤖 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 `@week-3/fed/_compat.py`:
- Around line 27-39: Update the TensorFlow import handling in the compatibility
initialization so only a missing top-level TensorFlow package returns normally;
treat a ModuleNotFoundError raised by TensorFlow’s internal dependencies like
other broken-installation exceptions. Preserve the existing cleanup and no-op
stub setup for that failure path, using the import exception context to
distinguish the cases.
In `@week-3/fed/data.py`:
- Around line 56-78: Update load_data to return both the client’s training
DataLoader and a distinct evaluation DataLoader backed by held-out MNIST test
data, rather than reusing the training partition. Preserve the existing
IID/non-IID partitioning for training, then update the runner call sites to
consume the separate loader for accuracy and loss evaluation.
In `@week-3/main_flower.py`:
- Around line 9-13: Replace the hard-coded /private/tmp Ray temporary directory
configuration in the startup setup with a platform-native temporary-directory
value from the standard library, and append a unique subdirectory for this run.
Apply the same change to the additional configuration block so Ray uses a
writable, unique path consistently across macOS, Linux, and Windows.
In `@week-3/plot.py`:
- Line 25: Update generate_plot to accept a noniid_alpha argument and use it in
the Non-IID plot legend instead of a hard-coded value. In main_flower.py, pass
args.alpha as noniid_alpha when calling plot.generate_plot, preserving the
existing file and output arguments.
In `@week-3/requirements.txt`:
- Around line 4-5: Update week-3/requirements.txt by removing the matplotlib
dependency and adding Pillow, matching the PIL import used by week-3/plot.py
while preserving the existing numpy constraint.
In `@week-3/walkthrough/app.js`:
- Around line 790-805: Update the quiz answer markup generated in the
answersHTML map to use button elements with type="button" and the existing
quiz-option class instead of clickable divs, preserving the current onclick
handler, IDs, option markers, and answer text.
- Around line 572-583: Update updateDiscrepancyMetric to compare IID and Non-IID
entries from the latest round present in both datasets, rather than blindly
using each array’s final entry. Use each selected row’s round property for the
displayed round labels, and calculate the penalty from those matched accuracies
while preserving the existing metric formatting.
- Around line 158-170: Update the barContainer created in the client
distribution rendering loop to expose an accessible text alternative: assign an
appropriate image role and build an aria-label summarizing every digit’s
percentage from clientProfiles[client], including values for segments omitted
visually because of the pct threshold.
In `@week-3/walkthrough/index.html`:
- Around line 185-190: Update the “Run Both Simulations” section to invoke the
Flower orchestration entry point main_flower.py, or revise the surrounding
description to accurately identify main.py as the direct PyTorch runner; ensure
the command and promised execution flow consistently refer to the same runner.
In `@week-3/walkthrough/style.css`:
- Around line 32-38: Update the responsive CSS for the body and the referenced
grid, header-navigation, and canvas rules so layouts adapt to narrow viewports
before horizontal overflow is clipped. Add appropriate small-screen breakpoints
and responsive sizing/wrapping for those elements, while preserving the existing
desktop layout and avoiding reliance on body overflow-x hiding as the primary
solution.
- Around line 751-771: Update the .upload-btn input[type="file"] styling to
visually hide the file input without using display: none, preserving keyboard
focusability. Add a :focus or :focus-visible style on the associated .upload-btn
label to expose the focused state while retaining the existing upload button
appearance.
---
Minor comments:
In `@week-3/fed/client.py`:
- Around line 39-52: Compute evaluation loss as a per-example mean in evaluate:
accumulate each batch loss multiplied by labels.size(0), then divide loss_sum by
total instead of len(self.trainloader). Apply the same loss accumulation and
normalization in week-3/main.py lines 99-119 before client-loss weighting; no
direct change is required elsewhere.
In `@week-3/main_flower.py`:
- Around line 86-92: Validate the --alpha argument in the argument parser before
execution, rejecting zero and negative values while retaining the existing
default and float parsing. Update the parser configuration around
parser.add_argument and ensure invalid values produce a clear CLI error instead
of reaching partitioning.
In `@week-3/main.py`:
- Around line 158-172: Update the exception handlers in the single-mode branches
of the main argument dispatch to report plotting failures instead of silently
passing. In both the args.mode == "iid" and args.mode == "noniid" blocks, print
the caught exception, matching the existing behavior in the "both" branch.
In `@week-3/MEMO.md`:
- Line 11: Update the “Initial Round Contrast” statement in the Round 1
comparison to label the 20.12 difference between 77.75% and 57.63% as percentage
points, not percent; leave the reported accuracy and loss values unchanged.
In `@week-3/plot.py`:
- Around line 82-99: Update the round-to-x-coordinate calculations in the
plotting loop and map_coords so a max_round of 1 does not divide by zero. Use a
zero horizontal offset for the single-round case while preserving the existing
scaling for multi-round logs, including partial simulation data.
In `@week-3/README.md`:
- Around line 87-108: Update the Step 2 experiment commands and descriptions to
reference main_flower.py instead of main.py. Ensure the IID, Non-IID,
custom-alpha, and automatic plotting instructions match the Flower/Ray runner
implemented by main_flower.py, while keeping standalone main.py usage separate.
In `@week-3/walkthrough/app.js`:
- Around line 622-630: Update the round-label coordinate calculation in the
max-round drawing loop and the corresponding mapCoords logic to handle maxR
equal to 1 without dividing by zero. Use the same safe denominator in both
locations, while preserving normal spacing for charts with multiple rounds.
- Around line 114-118: Update updateDirichletSplit and the related
setAlphaPreset flow so updateDirichletSplit does not unconditionally remove the
active class from preset buttons. Preserve the selected alpha preset, including
the default alpha === 0.10 state during initialization, by restoring or
maintaining the matching .preset-btn active state in the existing conditional.
- Around line 457-461: Update the simulation-finished branch that clears
driftSimInterval and resets drift-play-text to also set isSimulating to false,
allowing the next button click to start a new round.
- Around line 615-619: Update both canvas label blocks around the Acc labels to
resolve the --text-secondary CSS custom property to a computed color string
before assigning chartCtx.fillStyle; replace the direct var(--text-secondary)
assignment in each block while preserving the existing label rendering behavior.
In `@week-3/walkthrough/index.html`:
- Around line 149-153: Update the landscape-legend markup to add a Client 3 Min
legend item with a distinct color class matching Client 3’s rendered minimum,
and ensure it does not reuse the global-min color. Preserve the existing Global
Optimum and Client 1/2 entries.
In `@week-3/walkthrough/style.css`:
- Around line 90-93: Resolve the Stylelint issues in style.css: rename the
rotate🍭 keyframes identifier to kebab-case and update its animation use, change
currentColor declarations to currentcolor, and remove the redundant display:
inline-block from .option-marker. Apply these changes consistently to all
referenced locations.
🪄 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: c1baf577-4e75-49f6-89c6-1eb820e54e6e
📒 Files selected for processing (16)
week-3/.gitignoreweek-3/MEMO.mdweek-3/README.mdweek-3/fed/__init__.pyweek-3/fed/_compat.pyweek-3/fed/client.pyweek-3/fed/data.pyweek-3/fed/model.pyweek-3/fed/strategy.pyweek-3/main.pyweek-3/main_flower.pyweek-3/plot.pyweek-3/requirements.txtweek-3/walkthrough/app.jsweek-3/walkthrough/index.htmlweek-3/walkthrough/style.css
… model on test partitions
…path and validate alpha parameter
…in walkthrough page
…labels, and improve matched rounds calculation
…t test-set simulation
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
This PR implements the curriculum and coding deliverables for Fellowship Week 3: The Non-IID Problem. We simulate statistical heterogeneity using Dirichlet partitioning, analyse the numerical impact of label skew on convergence, explain the underlying mathematical mechanism of client drift, and build a premium, interactive walkthrough dashboard for students to explore these concepts.
Key Changes
1. Data Partitioning (
fed/data.py)dirichlet_splitto divide MNIST across 5 clients using the concentration parameter2. Simulation Runner & Plotter (
main.py/plot.py)main.py): Runs both IID and Non-IID simulations sequentially. Features a pure PyTorch optimization loop that matches FedAvg mathematically, bypassing Ray socket path limits and network sandboxing blocks.main_flower.py): Maintained the standard Flower/Ray simulation runner for direct curriculum compatibility.plot.py): Uses Python'sPILlibrary to generate the comparison chartconvergence_comparison.pngas a self-contained operation.3. Interactive Walkthrough (
walkthrough/)Experimental Results
Runs of 10 communication rounds on 5 simulated clients yielded a clear convergence penalty for Non-IID data:
How to Verify Locally
1. Run the Simulations & Plot
Verify that
iid_results.csv,noniid_results.csv, andconvergence_comparison.pngare correctly generated in theweek-3folder.2. Run the Interactive Dashboard
Open your browser and visit http://localhost:8000/walkthrough/ to interact with the Dirichlet slider, the vector field simulator, and the quiz.
Summary by CodeRabbit
New Features
Documentation
Chores