Repository navigation
Conversation
Decision models (Jev-style engines such as Nemotron Decide) do not answer
chat. They take a state and a map of typed questions (choice, noul, score)
and return one typed answer with probabilities per question over
POST /v1/systemone. Gym model servers had no route for this, so a harness
had to call the provider directly.
Add POST /v1/systemone to the base model server, mirroring /v1/embeddings:
the base default returns 501, the route is traced as gym.model.systemone in
the model_call span group, and it is declared non-generating so a call under
a token-capture prefix does not mark the rollout incomplete. openai_model
and inference_provider override it as a passthrough to {base_url}/systemone
(extra_body merged under the request, model replaced) through the new
NeMoGymAsyncOpenAI.create_systemone(). Provider errors follow each server's
existing upstream status and retry settings, as for embeddings.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Grzegorz Chlebus <gchlebus@nvidia.com>
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
No BLOCKER or RISK findings. The decision route follows the existing provider request, retry, status-propagation, and capture contracts. One non-blocking documentation-link suggestion is inline.
I reviewed all 11 changed files and their directly affected paths. This was a static review; I didn't execute tests or contact providers.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
LiteLLMModelServer inherited the openai_model passthrough, but a LiteLLM proxy has no /systemone route, so callers got a retried provider 404 surfaced as 500 instead of the documented 501. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Grzegorz Chlebus <gchlebus@nvidia.com>
In-process harness calls to /v1/systemone went out without the /ng-rollout/<id> prefix, so the gym.model.systemone span had no rollout_id. The route is declared non-generating, so the prefix does not affect token capture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Grzegorz Chlebus <gchlebus@nvidia.com>
Note that a propagated status must also be terminal when upstream_retry_policy retries, link the Decision Index kit for the question and answer types, and version-prefix the new internal links. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Grzegorz Chlebus <gchlebus@nvidia.com>
svcnvidia-nemo-ci
added
feature
yaoyu-33
left a comment
yaoyu-33
left a comment
There was a problem hiding this comment.
Review summary: SHIP
POST /v1/systemone follows the /v1/embeddings pattern the description promises: the base server answers 501 and declares the route non-generating, openai_model and inference_provider pass it through to {base_url}/systemone, litellm_model keeps the 501, and ServerClient adds the rollout prefix. I found no blocker or risk. The five inline comments are small notes, all optional.
What I checked. I traced a call from ServerClient.post (rollout prefix) through the capture middleware and traced_endpoint to _call_upstream / create_systemone, against a local stub provider, for both backends: the URL, the auth header, the extra_body and model merge, 422 propagation with and without propagate_upstream_http_status_codes, terminal statuses under upstream_retry_policy, and the 501 on every other in-repo model server (only openai_model and inference_provider pass the route through). The seven changed test files pass (605 tests), and the new tests fail when each production change is reverted. Not verified: a run against a real provider, and fern check.
Newer commits. The earlier automated SHIP covered 928b163e3. The litellm_model 501 (6a7a93424), the ServerClient prefix (8e4c56c71) and the docs commit (5fba98eaf) came after it, and I looked at those specifically.
For the Nemotron Decider evals. Decision calls work through openai_model or inference_provider pointed at a server that serves <url>/systemone. A Gym-launched vllm_model / local_vllm_model returns 501 for this route, as it does for embeddings, so a self-hosted decider has to be reached through one of those two backends.
PR hygiene
- The description says the route name, and whether it belongs on every model server, still needs a decision from core. A maintainer should settle that before merge: the name is baked into the base class, the
ServerClientallow-list, the span name and the docs. - AGENTS.md asks for an issue link or a reason none is needed. Could you link or open one that records that decision, since the Decision Index benchmark will build on this route?
- CI has not run yet:
copy-pr-botis waiting on validation of the head.
|
|
||
| ## Decisions | ||
|
|
||
| Every Model Server also exposes `POST /v1/systemone`, the decision API that decision models such as Jev serve; the [Decision Index kit](https://github.com/apolinario/decision-index) documents its question and answer types. A request carries `model`, a `state` (a string or a JSON object) and `questions`, a map of typed questions (`choice` with an option map, `noul` for a yes/no probability, `score` with ordered levels); the reply maps every question key to a typed answer with probabilities. The [OpenAI](/main/model-server/openai) and [Inference Providers](/main/model-server/inference-providers) backends implement it as a passthrough to `{base_url}/systemone`, with the same `extra_body` merge and `model` replacement as embeddings, and return the provider's JSON unchanged. Backends without a decision model return `501`. |
There was a problem hiding this comment.
Could you point to where score with ordered levels is specified? The Decision Index kit at its current commit documents choice and noul only (format.md), and its http engine takes a root URL and appends /v1/systemone itself, whereas this route calls {base_url}/systemone. A short note that openai_base_url (OpenAI) and base_url (Inference Providers) must end in /v1 would also help: a root URL comes back as a retried 404, which is an HTTP 500 on openai_model.
| config: LiteLLMModelServerConfig | ||
|
|
||
| async def systemone(self, body: dict = Body()) -> dict: | ||
| # A LiteLLM proxy does not serve /systemone, so keep the base 501 instead of the inherited passthrough. |
There was a problem hiding this comment.
Did a litellm_model run produce the 404 behind this override? The FAQ describes inference.nvidia.com as LiteLLM-backed (faq.mdx#L492-L493) and the validation here went through openai_model, so I am not sure a LiteLLM proxy never serves /systemone (it supports operator-defined pass-through routes). If the 501 stays by design, could the Decisions docs say to point openai_model at such a gateway?
| assert await server.embeddings({"input": "hi"}) == {"data": []} | ||
| assert server._client.create_embeddings.await_count == 2 | ||
|
|
||
| async def test_systemone_passes_every_question_type_through_unchanged(self) -> None: |
There was a problem hiding this comment.
openai_model/tests/test_app.py:546
Could you mirror two embeddings tests for the new route? test_embeddings_retry_through_upstream_policy (L535) would pin that systemone goes through _call_upstream, which the retry and terminal-status guidance in index.mdx#L75 relies on; calling self._client.create_systemone directly today keeps every new test green. And test_embeddings_extra_body_merge (inference_provider/tests/test_app.py#L151) would pin request-over-extra_body precedence for inference_provider, since test_systemone (L165) uses a non-conflicting key.
| # Decision API (SystemOne wire format). Decision-capable servers (e.g. openai_model, inference_provider) | ||
| # override systemone(); the base default returns 501, as for embeddings. | ||
| app.post("/v1/systemone")( | ||
| traced_endpoint(GymSpanGroup.MODEL_CALL, "gym.model.systemone", self.systemone, model_attributes) |
There was a problem hiding this comment.
base_responses_api_model.py:419
This puts the gym.model.systemone span in GymSpanGroup.MODEL_CALL, but that group's docstring in span_groups.py#L93 still lists only /v1/chat/completions, /v1/responses and /v1/messages. Could you add /v1/systemone there, next to the span-groups.mdx row you already updated?
| await self._raise_for_status(response, request_kwargs) | ||
| return await get_response_json(response) | ||
|
|
||
| async def create_systemone(self, **kwargs): |
There was a problem hiding this comment.
AGENTS.md asks for parameter and return annotations on new public methods; Any and Dict are already imported here.
| async def create_systemone(self, **kwargs): | |
| async def create_systemone(self, **kwargs: Any) -> Dict[str, Any]: |
|
/ok to test 5fba98e |
|
🌿 Preview your docs: https://nvidia-preview-pr-5471-4dd44752a10d.docs.buildwithfern.com/nemo/gym |
yaoyu-33
added
ready-to-merge
|
🤖 On it — handling this as babysit-ready-to-merge. |
|
I'll watch this ready-to-merge PR and report substantive progress. Running on |
|
🤖 On it — handling this as babysit-ready-to-merge. |
|
💬 The run failed: {"container_build":{"correction":"","duties_complete":false,"equivalent_attempt":0,"justification_evidence":"","preflight_complete":false,"preflight_evidence":"","reason":"not_requested","requested":false,"reuse_checked":false,"reuse_evidence":""},"do":[],"say":"","state":"working","wait":[{"item":"watch","on":"exec_terminal","operation_id":"exec-fde18d22a86857af517642d9"},{"item":"watch","on":"ci_watch_pr","pr":5471,"repo":"NVIDIA-NeMo/Gym"}]}. |
|
💬 I haven’t diagnosed the red CI on Can a platform operator restore the GitHub broker’s native Git authority so I can read the failing checks and logs? |
|
@gchlebus could you please rebase on merge and fix the merge conflict? |
|
The model provider is unavailable. Work is paused and automatic retries have stopped. Send a new message or retry when the provider is available to resume. |
|
/resolve-conflict |
Preserve the LiteLLM systemone fallback and the ChatCompletionsConverter update. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test b12b706 |
|
The label application does not own this session. |
|
I'll watch this ready-to-merge PR and report substantive progress. Running on |
Retain both pytest and ClientResponseError imports for the finish-reason and systemone tests. Preserve all automatically merged changes. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
The label application does not own this session. |
|
/ok to test 4dd4475 |
|
/ok to test 4dd4475 |
What does this PR do?
Adds
POST /v1/systemone, the decision API of Jev-compatible models such as Nemotron Decide and openjev, so a harness can call them through a Gym model server instead of the provider directly. It follows/v1/embeddings(#1964): the base server returns 501 and declares the route non-generating,openai_modelandinference_providerforward it to{base_url}/systemone,litellm_modelkeeps the 501, andServerClientadds the rollout prefix. A Decision Index benchmark on this route follows in a separate PR.Validation
pre-commit run --all-filespassed.Not covered: the route name and whether it belongs on every model server still need a decision from core.
Rollouts
N/A, no environment or agent change.
openai_modelagainst InferenceHubnemotron-decideandopenjev-4breturned typed answers, and withpropagate_upstream_http_status_codes: [422]a capacity refusal came back with the provider's body unchanged.Compatibility and benchmark impact
None for existing routes, configs or results; the new route returns 501 where a server does not implement it.
How to revert: revert this PR.
Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).