Skip to content

fix(rlix-scheduler): warn on GENERATION request without demand signal - #34

Open
JunzheJoe wants to merge 1 commit into
zhenyu/miles-mvp-e2efrom
joe/gen-request-demand-warn
Open

fix(rlix-scheduler): warn on GENERATION request without demand signal#34
JunzheJoe wants to merge 1 commit into
zhenyu/miles-mvp-e2efrom
joe/gen-request-demand-warn

Conversation

@JunzheJoe

Copy link
Copy Markdown
Member

What

Review finding S12-EST-NONE-HANG (F1-F12 code review, scheduler deep-dive).

request_gpus(GENERATION, step_target_estimate=None) is accepted and writes None into the durable rollout_open_pipelines registry. On the planner side (planner.plan_generation_gap_ratio, bootstrap-estimate branch), a pipeline with no positive estimate and no stored progress is skipped entirely — its pending request is never signaled, and the caller blocks forever inside request_gpus (which waits without timeout by design). No log line points at the cause; today the hazard is documented only in MilesPipeline._request_cluster_gpus's docstring.

This adds a logger.warning at both enqueue sites (request_gpus and notify_release_then_request_gpus) via a shared _warn_gen_request_without_demand_locked helper, naming the exact skip condition and the likely outcome.

Warn-only, no behavior change: some backends may legitimately enqueue first and publish progress later, so rejecting outright could break request-then-report flows. If we later decide to hard-reject, this helper is the single place to do it.

Tests

tests/test_scheduler_gen_request_warning.py — warn / no-warn matrix:

  • no estimate + no progress → warns
  • estimate=0 → warns
  • positive estimate → silent
  • stored progress (no estimate) → silent

Full suite: 69 passed, 3 skipped.

The gap-ratio planner skips a pipeline whose rollout_open_pipelines entry
has no positive step_target_estimate AND no stored progress (the
bootstrap-estimate branch in planner.plan_generation_gap_ratio). Such a
request is never signaled and its caller blocks forever in request_gpus
— silently. The hazard was documented only in MilesPipeline's docstring.

Warn at enqueue time (request_gpus + notify_release_then_request_gpus)
via a shared helper. Warn-only, no behavior change: some backends
legitimately request first and report progress later, so rejecting
outright could break them.

@zhenyulincs zhenyulincs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the finding end-to-end before approving:

  • The planner skip condition is real (planner.py:199-209): with no stored progress (step_target <= 0) and no positive estimate, the pipeline is continue-skipped — its pending request is never signaled, and request_gpus blocks without timeout by design.
  • Semantic alignment is exact: the warn helper keys off the second element of _pipeline_progress_totals_locked (total_required), which is the same quantity the planner's skip branch tests. No drift risk between the warning and the actual skip behavior.
  • Coverage is complete: the only two write sites of rollout_open_pipelines[...] in the repo are the two enqueue points this PR instruments, and both helper calls sit inside self._lock.
  • Why nothing hits this today: every in-repo caller already defends itself — MilesPipeline._init_phase_b_infer computes max(rollout_batch_size * n_samples_per_prompt, 1) (its comment cites exactly this hazard), the steady-state re-request threads the demand signal's step_target, and the ROLL backend computes its own estimate. So the trap is currently unreachable, guarded only by tribal knowledge copied into each caller. This PR moves that guard to the scheduler side where it belongs.
  • Ran the new test file (4/4) and the full suite on the PR head — green.

Warn-only is the right call given the request-then-report flows some backends legitimately use, and the shared helper gives a single flip point if we later decide to hard-reject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants