Add annotations to instances in launcher by MikeSpreitzer · Pull Request #399 · llm-d-incubation/llm-d-fast-model-actuation

MikeSpreitzer · 2026-04-01T22:17:19Z

.. to carry data meaningful to clients but not launcher.

This PR also updates some type names to say "State" instead of "Status", to correspond with their actual scope.

Also reshaped InstanceState to include VllmConfig inline.

Resolves: #397

Copilot

Pull request overview

This PR adds a generic annotations map to the launcher instance configuration/state so clients can attach and later retrieve metadata (e.g., ISC name and inference port) without the launcher needing to interpret it, addressing issue #397.

Changes:

Extend the launcher’s VllmConfig schema (Python + Go) with annotations: map[string]string.
Populate annotations from the controller when creating vLLM instances (e.g., isc-name, inference-port).
Refactor the Go launcher client types from “status” to “state” responses.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pkg/controller/dual-pods/launcherclient.go	Adds `Annotations` to `VllmConfig` and refactors response structs/method names for instance “state”.
pkg/controller/dual-pods/inference-server.go	Adds controller-provided annotations to the launcher instance config; updates sync method return type.
inference_server/launcher/tests/test_launcher.py	Updates test fixture to include `annotations` in `VllmConfig`.
inference_server/launcher/launcher.py	Adds `annotations` field to the Python `VllmConfig` model.

Copilot · 2026-04-01T22:20:57Z

+	InstanceID string     `json:"instance_id"`
+	Status     string     `json:"status"`
+	Spec       VllmConfig `json:",inline"`


InstanceState won't unmarshal the launcher’s JSON response as intended. The tag json:",inline" is ignored by Go’s encoding/json, and because Spec is a named field the config will end up nested under "Spec" (and the top-level options/gpu_uuids/env_vars/annotations fields will not populate). The Python launcher returns a flat object ({"instance_id", "status", ...config...} via **config.model_dump(...)), so this struct should embed VllmConfig anonymously (or implement custom (un)marshal) to match the API schema.

Suggested change

InstanceID string `json:"instance_id"`

Status string `json:"status"`

Spec VllmConfig `json:",inline"`

InstanceID string `json:"instance_id"`

Status string `json:"status"`

VllmConfig

Copilot · 2026-04-01T22:20:58Z

    options: str
    gpu_uuids: Optional[List[str]] = None
    env_vars: Optional[Dict[str, str]] = None
+    annotations: Optional[Dict[str,str]] = None


This type annotation will fail flake8 (E231: missing whitespace after ',') because it uses Dict[str,str]. Update it to Dict[str, str] (and keep spacing consistent with the existing env_vars: Optional[Dict[str, str]]).

Suggested change

annotations: Optional[Dict[str,str]] = None

annotations: Optional[Dict[str, str]] = None

MikeSpreitzer · 2026-04-01T22:32:55Z

/ok-to-test

github-actions · 2026-04-01T22:33:04Z

🚀 E2E tests triggered by /ok-to-test

View the OpenShift E2E workflow run

MikeSpreitzer · 2026-04-02T02:03:09Z

/ok-to-test

github-actions · 2026-04-02T02:03:17Z

🚀 E2E tests triggered by /ok-to-test

View the OpenShift E2E workflow run

rubambiza

Leaving a general comment without explicit approval in case the PR may depend on PR 363 being merged first.

rubambiza · 2026-04-02T13:47:19Z


 func (ctl *controller) configInferenceServer(isc *fmav1alpha1.InferenceServerConfig, gpuUUIDs []string) (*VllmConfig, string, error) {
-	options := isc.Spec.ModelServerConfig.Options + " --port " + strconv.Itoa(int(isc.Spec.ModelServerConfig.Port))
+	portS := strconv.Itoa(int(isc.Spec.ModelServerConfig.Port))


I'd like to flag a potential conflict here. PR 363 has overhauled/replaced this method definition to include a check on whether the user explicitly specifies the port number. Might be worth discussing what the dependency will be, but I am okay with merging this one first.

Btw, I haven't finished reviewing PR 363, but I had noted that the port number became optional in the ModelServerConfig as we discussed on the call yesterday.

I think that PR #363 should come after completion of milestone 3.

I think that the PR at hand makes sense with or without the changes in PR #363 .

github-actions · 2026-04-02T14:09:56Z

Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

MikeSpreitzer · 2026-04-02T14:09:58Z

The force-push to 4619b3a is a rebase onto main.

.. to carry data meaningful to clients but not launcher. Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

MikeSpreitzer · 2026-04-02T14:16:18Z

.. aand the force-push to 3af68cd is a rebase with GPG signatures.

MikeSpreitzer · 2026-04-02T14:34:39Z

/ok-to-test

github-actions · 2026-04-02T14:34:51Z

🚀 E2E tests triggered by /ok-to-test

View the OpenShift E2E workflow run

Copilot AI review requested due to automatic review settings April 1, 2026 22:17

Copilot started reviewing on behalf of MikeSpreitzer April 1, 2026 22:17 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

MikeSpreitzer force-pushed the add-instance-annotations branch 2 times, most recently from 490ba2d to 3ca0b5e Compare April 1, 2026 22:28

MikeSpreitzer force-pushed the add-instance-annotations branch 2 times, most recently from f3b74d6 to c7c373e Compare April 2, 2026 01:22

MikeSpreitzer requested review from diegocastanibm and waltforme April 2, 2026 05:28

rubambiza reviewed Apr 2, 2026

View reviewed changes

rubambiza mentioned this pull request Apr 2, 2026

Use EnvVars map instead of copying it #401

Merged

MikeSpreitzer force-pushed the add-instance-annotations branch from c7c373e to 4619b3a Compare April 2, 2026 14:09

Add annotations to instances in launcher

3af68cd

.. to carry data meaningful to clients but not launcher. Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

MikeSpreitzer force-pushed the add-instance-annotations branch from 4619b3a to 3af68cd Compare April 2, 2026 14:15

waltforme approved these changes Apr 2, 2026

View reviewed changes

MikeSpreitzer merged commit 77f97d2 into llm-d-incubation:main Apr 2, 2026
25 checks passed

MikeSpreitzer deleted the add-instance-annotations branch April 2, 2026 17:26

	annotations: Optional[Dict[str,str]] = None
	annotations: Optional[Dict[str, str]] = None

Conversation

MikeSpreitzer commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

MikeSpreitzer Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

MikeSpreitzer commented Apr 1, 2026

Uh oh!

github-actions Bot commented Apr 1, 2026

Uh oh!

MikeSpreitzer commented Apr 2, 2026

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

rubambiza left a comment

Choose a reason for hiding this comment

Uh oh!

rubambiza Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

MikeSpreitzer Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

MikeSpreitzer commented Apr 2, 2026

Uh oh!

MikeSpreitzer commented Apr 2, 2026

Uh oh!

MikeSpreitzer commented Apr 2, 2026

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MikeSpreitzer commented Apr 1, 2026 •

edited

Loading