Repository navigation
refactor(pipeline): deduplicate scheduler constants and shared functions - #150
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
+ Coverage 83.61% 83.86% +0.25%
==========================================
Files 126 127 +1
Lines 51510 51148 -362
==========================================
- Hits 43069 42895 -174
+ Misses 8441 8253 -188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis pull request refactors scheduler implementations to use dynamic step resolution instead of static constants. Static 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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)
src/lib/unified_pipeline/scheduler/balanced_chase.rs (1)
39-58:⚠️ Potential issue | 🟠 MajorAdd
num_threads > 0guard to constructor before delegating to helper.The helper
balanced_chase_determine_role()evaluatesnum_threads - 1andnum_threads - 2in conditions (lines 369, 373 in mod.rs). These expressions are evaluated before their corresponding guards (num_threads > 1,num_threads > 3), causing panics in debug builds ifnum_threads == 0. Adding a precondition in the constructor prevents this class of error at the call boundary.Proposed fix
pub fn new(thread_id: usize, num_threads: usize, active_steps: ActiveSteps) -> Self { + assert!(num_threads > 0, "num_threads must be > 0"); let (current_step, exclusive_role) = Self::determine_role(thread_id, num_threads);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/unified_pipeline/scheduler/balanced_chase.rs` around lines 39 - 58, In the constructor new(thread_id, num_threads, active_steps) add a precondition that num_threads > 0 before calling determine_role to avoid downstream underflow in balanced_chase_determine_role; e.g., validate (panic/assert) with a clear message or return an error when num_threads == 0, then call Self::determine_role(thread_id, num_threads) as before so determine_role and balanced_chase_determine_role never see num_threads == 0.
🧹 Nitpick comments (1)
src/lib/unified_pipeline/scheduler/backpressure_proportional.rs (1)
85-87: HoistPipelineStep::all()outside the loop.Small readability/perf improvement: avoid rebuilding the step array each iteration.
♻️ Proposed refactor
- for (priority, (_, step_idx)) in weighted.iter().enumerate() { - self.priority_buffer[priority] = PipelineStep::all()[*step_idx]; - } + let steps = PipelineStep::all(); + for (priority, (_, step_idx)) in weighted.iter().enumerate() { + self.priority_buffer[priority] = steps[*step_idx]; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/unified_pipeline/scheduler/backpressure_proportional.rs` around lines 85 - 87, The loop repeatedly calls PipelineStep::all(), rebuilding the same array each iteration; hoist that call out of the for-loop by assigning let all_steps = PipelineStep::all() before iterating and then use all_steps[*step_idx] when populating self.priority_buffer[priority] (the loop over weighted, the priority_buffer assignment and the weighted/step_idx variables identify the spot to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/unified_pipeline/scheduler/balanced_chase.rs`:
- Around line 39-58: In the constructor new(thread_id, num_threads,
active_steps) add a precondition that num_threads > 0 before calling
determine_role to avoid downstream underflow in balanced_chase_determine_role;
e.g., validate (panic/assert) with a clear message or return an error when
num_threads == 0, then call Self::determine_role(thread_id, num_threads) as
before so determine_role and balanced_chase_determine_role never see num_threads
== 0.
---
Nitpick comments:
In `@src/lib/unified_pipeline/scheduler/backpressure_proportional.rs`:
- Around line 85-87: The loop repeatedly calls PipelineStep::all(), rebuilding
the same array each iteration; hoist that call out of the for-loop by assigning
let all_steps = PipelineStep::all() before iterating and then use
all_steps[*step_idx] when populating self.priority_buffer[priority] (the loop
over weighted, the priority_buffer assignment and the weighted/step_idx
variables identify the spot to change).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/unified_pipeline/scheduler/backpressure_proportional.rssrc/lib/unified_pipeline/scheduler/balanced_chase.rssrc/lib/unified_pipeline/scheduler/balanced_chase_drain.rssrc/lib/unified_pipeline/scheduler/chase_bottleneck.rssrc/lib/unified_pipeline/scheduler/epsilon_greedy.rssrc/lib/unified_pipeline/scheduler/learned_affinity.rssrc/lib/unified_pipeline/scheduler/mod.rssrc/lib/unified_pipeline/scheduler/optimized_chase.rssrc/lib/unified_pipeline/scheduler/sticky_work_stealing.rssrc/lib/unified_pipeline/scheduler/thompson_sampling.rssrc/lib/unified_pipeline/scheduler/thompson_with_priors.rssrc/lib/unified_pipeline/scheduler/ucb.rs
Replace per-scheduler STEPS constant (11 copies) with PipelineStep::all() and per-scheduler step_index() method (10 copies) with PipelineStep::index(), both of which already exist on PipelineStep. Extract shared balanced_chase_determine_role() function to scheduler/mod.rs, replacing identical copies in balanced_chase.rs and balanced_chase_drain.rs.
51b236c to
5a7c603
Compare
Summary
STEPSconstant (11 copies) with existingPipelineStep::all()methodstep_index()method (10 copies) with existingPipelineStep::index()methodbalanced_chase_determine_role()toscheduler/mod.rs, replacing identical copies inbalanced_chase.rsandbalanced_chase_drain.rsNet: -204 lines across 12 files.
Test plan
cargo nextest run -p fgumi --filter-expr 'test(scheduler) | test(pipeline) | test(unified)'— 339 tests passcargo ci-fmt— cleancargo ci-lint— clean