Skip to content

feat(roles): support per-role engines and images - #33

Closed
cquil11 wants to merge 1 commit into
mainfrom
feat/role-engines
Closed

cquil11 wants to merge 1 commit into
mainfrom
feat/role-engines

Conversation

@cquil11

@cquil11 cquil11 commented Sep 28, 2026 •

Copy link
Copy Markdown

Allow independent engines and worker images per role, using existing backends with shared node/GPU placement and port allocation.

Recipes use either a shared top-level engine or an explicit engine on every role. Mixing the forms fails preflight, even when the engine types match. Role engines do not inherit options from each other.

Validation runs during configuration loading, before Slurm submission:

  • Per-role engines with Dynamo or Dynamo sidecars are rejected.
  • Each frontend checks every active role against its required engine, then runs its own pairing/topology validation. vLLM prefill with SGLang decode is rejected by both existing native routers.
  • Additional unsupported job-wide features are rejected, as documented in the role configuration reference.

Configuration acceptance does not verify the connector installed inside an image or prove cross-engine KV compatibility.

Stack 1/2. TileRT support builds on this in #29.

Validation: 423 focused tests passed independently, without TileRT installed or registered. The latest update documents the existing validation boundary.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread src/srtctl/core/roles.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6c7b0da. Configure here.

result: NamedProcesses = {}
for role, backend in self.config.active_role_backends():
processes = [p for p in self.backend_processes if p.endpoint_mode == role]
result.update(self._start_workers(backend, processes))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Colocated workers skip sequential start

Medium Severity

start_all_workers now launches each role through a separate _start_workers call. sequential_node_start therefore groups endpoints by node within one role only, so colocated prefill and decode on the same node are no longer batched and health-gated together. Concurrent model loads on shared GPUs can contend or OOM when a role engine uses per-endpoint launch.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6c7b0da. Configure here.


configured_backend = getattr(getattr(config, "backend", None), "type", self.required_backend)
if configured_backend != self.required_backend:
if self.required_backend is not None and configured_backend != self.required_backend:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Router uses serving engine only

Medium Severity

The static router now allows required_backend to be unset so mixed engines can start, but it still reads config.backend for worker scheme, bootstrap ports, and the type check. That object is a copy of the serving role only, so prefill workers can be advertised with decode-engine URL rules.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6c7b0da. Configure here.

@cquil11

cquil11 commented Oct 5, 2026

Copy link
Copy Markdown
Author

Closing because per-role engines and images were merged upstream in NVIDIA#529.

@cquil11 cquil11 closed this Oct 5, 2026
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