Skip to content

feat(sweeper): preserve engine and request controls on canonical API [AIC-1771] - #19

Closed
jasonqinzhou wants to merge 2 commits into
ai-dynamo:mainfrom
jasonqinzhou:jasonzho/aic-1771-sweeper-engine-request-controls
Closed

jasonqinzhou wants to merge 2 commits into
ai-dynamo:mainfrom
jasonqinzhou:jasonzho/aic-1771-sweeper-engine-request-controls

Conversation

@jasonqinzhou

@jasonqinzhou jasonqinzhou commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Replacement published: PR #269 targets #242's branch (hzhou-codex/pr13-estimator-api) from codex/pr19-engine-controls-on-pr242, at tested commit a01b1ae21c5335b5f3387104b054bf10948f6246. Use #269 for the current diff and further review. This PR retains the original discussion and stale GitHub head.

Preserve pinned engine and request controls through the canonical estimator API introduced by #242. For example, nextn: 2 with nextn_accepted: 1.25 retains the caller's acceptance assumption, and two sequential synthetic requests sharing four prefix tokens report reuse of [0, 4].

Dependency and publication status

The updated implementation is pushed at a01b1ae21c5335b5f3387104b054bf10948f6246, with #242's current head, 1291c77f160c1e48bf425a1e6555a7d92c16f0f6, as a direct parent. The intended base is hzhou-codex/pr13-estimator-api while #242 is open. Review the 36-file incremental diff against #242.

This PR's displayed diff is still stale. GitHub reports that the original head repository, jasonqinzhou/aisimulate, is no longer a fork. Although the branch push succeeded, this PR and refs/pull/19/head still point to f15b838f1361c566f23191f489f1c617e3316351, and both REST and GraphQL reject the requested base change. The implementation and validation below describe the pushed integration commit, not the old diff shown by this PR. The current implementation is now published in replacement PR #269; this original PR remains unchanged at the GitHub head/base level.

Changes beyond #242

  • Extend ForwardPassPerfModelConfig with EPLB, expert slots, and MoE backend selection. Carry these and existing attention/quantization controls through unified prediction/recommendation, resolved candidate identity, native construction, and memory sizing/cache keys.
  • Preserve explicit speculative acceptance, optional chunked prefill, and exact synthetic shared prefixes through saved predictions and replay. Unsupported provider/model combinations fail explicitly.
  • Reuse the existing context_length and per-role KV memory fractions. Omit the deprecated enable_wideep switch; topology selects the execution regime.
  • Integrate refactor(perfmodel): unify estimator selection and configuration #242's latest resource supervision, estimator readiness, data-root precedence, and migration documentation. Put supported controls in migration section 4.6.3, retain the remaining gaps in section 5.6, and document both public Rust struct-literal updates.

Validation on the pushed integration

macOS arm64, Python 3.13, freshly rebuilt native extension:

  • Core Rust library: 1,427 passed, 1 ignored.
  • CLI, Sweeper, native replay, AFD/EPD, offload, and resource regressions: 1,181 passed, 5 documented cross-repository Dynamo skips.
  • Engine-step/compile-engine parity plus SDK, memory, MTP, and Python public API: 555 passed, including all 386 parity cases; no golden changes.
  • External Rust public API: 7 passed.
  • Changed-file Ruff, Rust formatting, whitespace, strict CODEOWNERS coverage and generated artifacts, packaged legal files, and local documentation destinations pass. Documentation-link checker tests: 7 passed.

Exact commands for native and parity validation:

cargo test -p aisimulate-core --features python --lib
cargo test --manifest-path crates/tests/public-api/Cargo.toml
python -m pytest -p no:timeout -c pytest.ini \
  crates/core/parity_tests/perfmodel/test_engine_step_parity.py \
  crates/core/parity_tests/perfmodel/test_compile_engine_parity.py \
  python/aisimulate/tests/unit/sdk/test_rust_engine_step.py \
  python/aisimulate/tests/unit/sdk/test_memory_estimation.py \
  python/aisimulate/tests/unit/sdk/test_compile_engine_mtp.py \
  python/aisimulate/tests/cross_package/test_core_public_api.py -q

The Rust embedding harness used the local Python runtime and project site-packages explicitly. No golden fixtures changed. Hosted CI/review on the updated PR head cannot be established while GitHub retains the old head; this PR remains draft and is not ready for human review or merge.

Boundaries

EPLB/slot/MoE-backend overrides are unsupported by collected FPM interpolation. Explicit incompatible requests fail; automatic selection skips that estimator. Cold regression remains unavailable in offline replay. MTP acceptance is caller-supplied, and shared-prefix input does not prewarm caches or reproduce AIC's fixed cached-token assumption. These tests establish plumbing and parity, not hardware accuracy qualification.

Role-specific model-control overrides remain separate (AIC-1772). Multimodal/analytical encoder and AFD model-control support remain outside this change.

Linear: https://linear.app/nvidia/issue/AIC-1771

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
@linear-code

linear-code Bot commented Aug 19, 2026 •

Copy link
Copy Markdown

@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Exact-head audit and fixes are now pushed at f15b838.

Findings resolved:

  • The previous green checks were stale and the branch conflicted with current main. I merged current main without force-pushing and resolved the unified CLI, Sweeper, deployment, and test conflicts.
  • The new controls existed only in the internal flat SearchSpace, so users could not migrate them through aisimulate recommend. They are now exposed as pinned, flat engine fields and round-trip through validation, recommendation lowering, candidate output, prediction compilation, and replay.
  • cached_prefix_tokens was silently filtered out by the unified native traffic path. The native traffic contract now preserves token-exact shared prefixes, including values that are not scheduler-block aligned.
  • Current engine constraints require SGLang chunking defaults and restrict max_model_len to vLLM. The lowering now preserves those backend contracts while context_length still controls recommendation capacity.
  • docs/cli/migrate-from-aiconfigurator.md now maps the new AIC flags and Task fields and includes a tested recommendation YAML example, including the explicit nextn and nextn_accepted requirement.

Verification:

  • 261 focused Python unit/integration tests passed.
  • The 17-case unified engine CLI E2E matrix passed.
  • The native exact-prefix regression passed.
  • Ruff, cargo fmt check, and diff checks passed.

There were no existing review threads to resolve. Fresh GitHub workflows are currently waiting for copy-PR validation; DCO is green. The PR remains draft and still requires code-owner approval.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

The current head f15b838f1361c566f23191f489f1c617e3316351 conflicts with current main and predates the canonical estimator contract in #242 (521f528d20b48dfabdee74afe5e40b4e0cb883e3). Its old compiler/runner and memory-control wiring cannot be carried forward unchanged without duplicating or bypassing the newer configuration paths.

I am updating this PR against #242, preserving the missing request/engine controls through the canonical estimator identity, candidate serialization, KV feasibility, and native replay. Existing memory/capacity and speculative-decoding controls will be reused where they already provide the behavior. The refreshed diff will target #242 while it is open, with focused regression tests and updated migration documentation.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Integration self-review found two reachable boundary issues in the updated implementation: explicitly setting aic_nextn=0 was still forwarded to a runner field that accepts positive values only, and calling the legacy deployment helper without a resolved canonical estimator silently discarded the new model controls. I am omitting zero speculation from runtime arguments and making unresolved control-bearing deployments fail explicitly. I am also rejecting an explicit chunked-prefill control for AFD, where the deployment path cannot apply it. Regression coverage will accompany these corrections before publication.

@jasonqinzhou jasonqinzhou changed the title feat(sweeper): preserve engine and request controls [AIC-1771] feat(sweeper): preserve engine and request controls on canonical API [AIC-1771] Sep 17, 2026
@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Integration review finding: after merging the current #242 head (1291c77), the migration guide still lists quantization/kernel selectors and speculative decoding as unified-CLI gaps while the #19 appendix exposes them. I am moving the supported controls into section 4.6, updating the gap section, and retaining the distinction between synthetic shared prefixes and fixed prewarmed-cache assumptions. The underlying branch update is being validated; GitHub still displays the stale f15b838 PR head and rejects retargeting this detached-source PR.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

A second integration documentation finding: the new cached_prefix_tokens member also changes public Rust SyntheticTraceSpec struct literals. I am adding its zero-value migration alongside the ForwardPassPerfModelConfig field migration so downstream Rust callers have both required updates documented.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Updated the existing source branch without force-pushing: a01b1ae21c5335b5f3387104b054bf10948f6246 now has the current #242 head, 1291c77f160c1e48bf425a1e6555a7d92c16f0f6, as a direct parent. The incremental comparison contains 36 files.

Both integration documentation findings are fixed in that pushed commit: supported controls now live in migration section 4.6.3 with accurate remaining gaps in 5.6, and the public Rust migration documents both ForwardPassPerfModelConfig and SyntheticTraceSpec additions. Final self-review found no additional actionable finding in the incremental diff.

Fresh local validation passed: 1,427 Rust library tests (1 ignored), 1,181 application/replay/resource tests (5 documented Dynamo integration skips), 555 parity/SDK/Python API tests, and 7 external Rust API tests. No golden fixtures changed. Ruff, formatting, whitespace, CODEOWNERS, packaged notices, and documentation checks pass.

Publication remains blocked. The original source repository reports fork=false; the pushed source branch is at a01b1ae, but this PR and refs/pull/19/head remain at f15b838. REST and GraphQL both reject retargeting to hzhou-codex/pr13-estimator-api. The description now identifies this mismatch and links the current diff. This PR remains draft; no hosted-CI or review-readiness claim is made for the updated branch. A linked replacement PR or GitHub restoration of the fork relationship is required to publish the current diff.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

The updated implementation is now published as #269 from the new same-repository branch codex/pr19-engine-controls-on-pr242. GitHub confirms that #269 targets hzhou-codex/pr13-estimator-api (#242), with the tested head a01b1ae and the intended 36-file incremental diff. Please use #269 for further review; this PR retains the original discussion and stale GitHub head.

@jasonqinzhou

jasonqinzhou commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Superseded by #269, which publishes the tested engine/request-control integration from codex/pr19-engine-controls-on-pr242 and correctly targets #242's branch, hzhou-codex/pr13-estimator-api. Closing this stale PR because its detached source repository prevents GitHub from refreshing its head/base. Please continue review in #269; the original discussion remains here.

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.

1 participant