fix(api): restore model_id field in /workers response - #774
Conversation
What changed: - crates/protocols/src/worker.rs: add model_id field to WorkerInfo, computed from the primary model in the models array via with_model_id(). Field is serialization-only (skip_deserializing) and omitted when null (wildcard workers). - model_gateway/src/core/worker.rs: call with_model_id() in worker_to_info() so all /workers responses include model_id. - e2e_test/router/test_worker_api.py: assert worker.model is present in test_list_workers. Why: the WorkerModels refactor removed the flat model_id field from worker API responses, breaking clients that depend on it for model registration. This restores backwards compatibility while keeping the full models array. Closes #773 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
tests
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request reintroduces the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restores the model_id field to the /workers API response for backward compatibility, which was unintentionally removed during a refactor. The changes correctly add the field to the WorkerInfo struct and populate it from the primary model. The e2e tests are also updated to verify the presence of this field. My review includes a suggestion to simplify the instantiation of WorkerInfo to make the code more direct and efficient, aligning with best practices for handling optional fields.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e95918c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/router/test_worker_api.py`:
- Around line 45-55: The test currently asserts every worker has a model via the
unconditional assert on worker.model, which conflicts with the API contract that
model_id is optional for wildcard workers; update the loop that iterates over
workers to only assert presence of model when appropriate (e.g., skip or allow
missing model_id for wildcard workers) by checking the worker's nature before
asserting model (refer to the loop variable worker and its attributes model /
model_id), or replace the unconditional assert with a conditional assertion that
permits None for wildcard entries. Ensure you keep the existing assert for
worker.url unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0abc0c71-7a56-4aa0-9066-35b9c61f9685
📒 Files selected for processing (3)
crates/protocols/src/worker.rse2e_test/router/test_worker_api.pymodel_gateway/src/core/worker.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/protocols/src/worker.rs`:
- Around line 579-581: The doc comment for the field model_id is inaccurate: it
states that None is serialized as `null` but the field has #[serde(default,
skip_serializing_if = "Option::is_none")] so None is omitted from JSON. Update
the comment on the model_id field (the pub model_id: Option<String> declaration)
to state that when None the field is omitted from serialization (not serialized
as null), and clarify that the value is computed from models[0].id
(single/multi) or omitted for wildcard.
In `@model_gateway/src/core/worker.rs`:
- Around line 936-939: The WorkerInfo construction in worker_to_info currently
derives model_id from spec.models.primary(), which ignores models_override and
can be stale for workers updated via set_models(); update worker_to_info to
compute model_id from the runtime-visible models instead (e.g., prefer
spec.models_override or the worker's current models view such as
worker.models().primary()) so that WorkerInfo.id uses the live primary model id
rather than the static spec.models.primary() value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6acfdf7a-10db-4848-a164-5d52d1e5ab7d
📒 Files selected for processing (3)
crates/protocols/src/worker.rse2e_test/router/test_worker_api.pymodel_gateway/src/core/worker.rs
What changed: - crates/protocols/src/worker.rs: remove skip_deserializing so Rust consumers can also read model_id from JSON responses. Remove with_model_id() helper since model_id is now computed inline. - model_gateway/src/core/worker.rs: compute model_id directly from spec.models.primary() in worker_to_info() instead of using with_model_id() builder. - e2e_test/router/test_worker_api.py: make model_id assertion conditional — wildcard workers may have model_id as None. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
722d83c to
51dd8b4
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
model_gateway/src/core/worker.rs (1)
938-938:⚠️ Potential issue | 🟠 MajorUse runtime-discovered models when deriving
model_id.At Line 938, deriving from
spec.models.primary()can return stalemodel_idafter lazy discovery (set_modelsupdatesmodels_override, notspec.models). This breaks the compatibility field for discovered workers.🔧 Proposed fix
pub fn worker_to_info(worker: &Arc<dyn Worker>) -> WorkerInfo { let metadata = worker.metadata(); let spec = metadata.spec.clone(); + let model_id = worker.models().into_iter().next().map(|m| m.id); WorkerInfo { id: worker.url().to_string(), - model_id: spec.models.primary().map(|m| m.id.clone()), + model_id, spec, is_healthy: worker.is_healthy(), load: worker.load(), job_status: None, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/core/worker.rs` at line 938, The assignment deriving model_id from spec.models.primary() can return stale values after lazy discovery; change the logic in worker.rs (where model_id: spec.models.primary().map(|m| m.id.clone()) is set) to prefer the runtime-discovered models_override (e.g. models_override.primary().map(|m| m.id.clone())) and fall back to spec.models.primary() only if models_override is empty, so the compatibility field uses the discovered model IDs updated by set_models.crates/protocols/src/worker.rs (1)
578-581:⚠️ Potential issue | 🟡 MinorFix
model_idserialization contract and doc wording.At Line 579,
Noneis omitted (notnull) because ofskip_serializing_if. Also consider making this field serialization-only to match the intended derived-response behavior.✏️ Proposed fix
- /// Primary model ID for backwards compatibility. - /// Computed from `models[0].id` (single/multi) or `null` (wildcard). - #[serde(default, skip_serializing_if = "Option::is_none")] + /// Primary model ID for backwards compatibility. + /// Computed from `models[0].id` (single/multi) or omitted when wildcard. + #[serde(default, skip_deserializing, skip_serializing_if = "Option::is_none")] pub model_id: Option<String>,#!/bin/bash # Verify current serde contract and find WorkerInfo deserialization call sites. rg -n -C2 'pub struct WorkerInfo|model_id: Option<String>|skip_deserializing|skip_serializing_if = "Option::is_none"' crates/protocols/src/worker.rs rg -nP --type=rust -C2 'serde_json::from_(str|slice|value)\s*::\s*<\s*WorkerInfo\s*>' rg -nP --type=rust -C2 '\bWorkerInfo\b.*Deserialize|Deserialize.*\bWorkerInfo\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/protocols/src/worker.rs` around lines 578 - 581, The doc and serde attributes for WorkerInfo::model_id are inconsistent: None is currently omitted (not serialized as null) due to skip_serializing_if, and you want this field to be serialization-only. Update the field's serde attributes to add skip_deserializing and keep default + skip_serializing_if (e.g. #[serde(skip_deserializing, default, skip_serializing_if = "Option::is_none")]) so model_id is emitted when present but ignored on input, and revise the doc comment on model_id to state that it is derived for responses and omitted when None (not serialized as null).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/protocols/src/worker.rs`:
- Around line 578-581: The doc and serde attributes for WorkerInfo::model_id are
inconsistent: None is currently omitted (not serialized as null) due to
skip_serializing_if, and you want this field to be serialization-only. Update
the field's serde attributes to add skip_deserializing and keep default +
skip_serializing_if (e.g. #[serde(skip_deserializing, default,
skip_serializing_if = "Option::is_none")]) so model_id is emitted when present
but ignored on input, and revise the doc comment on model_id to state that it is
derived for responses and omitted when None (not serialized as null).
In `@model_gateway/src/core/worker.rs`:
- Line 938: The assignment deriving model_id from spec.models.primary() can
return stale values after lazy discovery; change the logic in worker.rs (where
model_id: spec.models.primary().map(|m| m.id.clone()) is set) to prefer the
runtime-discovered models_override (e.g. models_override.primary().map(|m|
m.id.clone())) and fall back to spec.models.primary() only if models_override is
empty, so the compatibility field uses the discovered model IDs updated by
set_models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6da38676-9237-4c80-8597-8bd10d47c5d0
📒 Files selected for processing (3)
crates/protocols/src/worker.rse2e_test/router/test_worker_api.pymodel_gateway/src/core/worker.rs
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
2 participants
Summary
model_idfield in/workersAPI response for backwards compatibilityWorkerModelsrefactor removed the flatmodel_idfrom worker responses, breaking clients that parsed it for model registrationCloses #773
What changed
model_id: Option<String>toWorkerInfo, computed frommodels.primary().idviawith_model_id(). The field is serialization-only (skip_deserializing) and omitted for wildcard workers.worker_to_info()now calls.with_model_id()to populate the field.test_list_workersnow assertsworker.modelis present.Response format (before → after)
Before:
{"id": "...", "url": "http://...", "models": [{"id": "my-model"}], ...}After:
{"id": "...", "url": "http://...", "model_id": "my-model", "models": [{"id": "my-model"}], ...}Test plan
cargo test -p openai-protocol -p smg -- workerpassestest_list_workersassertsmodel_idis present in responsemodel_idfrom response (null/absent)Summary by CodeRabbit