Repository navigation
feat(observability): return the environment session id and allow correlation without full capture - #2613
feat(observability): return the environment session id and allow correlation without full capture#2613waple0820 wants to merge 2 commits into
Conversation
dc13dab to
48e6f66
Compare
48e6f66 to
d3267f0
Compare
|
Rebased onto main and out of draft. This PR used to be stacked on #2611 and carried its commit, so the diff looked larger than the change. Unstacked — the two are independent now and can land in either order. One connection to the error-handling design in #2750 that was not in the original description: the The field here is optional and defaults to absent, so it does not require that policy change — it just makes it possible. |
d3267f0 to
7e4463b
Compare
7e4463b to
bbb6f1c
Compare
bbb6f1c to
339b0ea
Compare
|
This PR should remain diagnostic observability work for #2610/#2831. The provider env_session_id is opaque correlation metadata; it must not become the allocation, deduplication, authorization, or cleanup identity. Those semantics belong to caller-created _ng_session_id and _ng_session_close_token in #2609. Rebase this after #2609 establishes that distinction. |
339b0ea to
cd90d9d
Compare
|
@ananthsub agreed on the distinction, and it is now stated at the definition rather than only in the PR body: # Diagnostic correlation only. It is not the allocation, deduplication, authorization
# or cleanup identity - those belong to the caller-created `_ng_session_id` and its
# close capability (#2609), which exist precisely because a provider handle is learned
# from this response and is therefore unavailable when the response is lost.
env_session_id: Optional[str] = NoneTwo tests hold it: absent by default on both ends, and surviving the seed-to-verify round trip. The PR is 2 files, +36/-1 — the Ready whenever #2609 lands; nothing here conflicts with it. |
cd90d9d to
5140be0
Compare
4916530 to
38003f3
Compare
…rify training side learns nothing about the handle the environment actually allocated — the container, the browser context, the provider session that consumed quota — so a rollout record and a provider-side log can only be joined on a timestamp. `env_session_id` is optional on both `BaseSeedSessionResponse` and `BaseVerifyResponse`, opaque to Gym, and absent unless an environment reports one, so nothing changes for an environment that does not. It is diagnostic correlation only. The allocation, deduplication, authorization and cleanup identity is the caller-created `_ng_session_id` and its close capability in NVIDIA-NeMo#2609, which exist precisely because a provider handle is learned from the seed response and is therefore unavailable when that response is lost. The docstring says so at the definition, since the two are easy to conflate. This originally also added a `rollout_correlation_enabled` key so the rollout prefix could reach resources servers without turning on model-call capture. NVIDIA-NeMo#2783 removes that need by making correlation independent of the observability gate, so that half is dropped here. No training framework needs a transport change: verl and NeMo-RL already carry the whole verify response as `full_result`. Terminology: this is the environment session created by `/seed_session`, not the vLLM router KV-cache affinity of NVIDIA-NeMo#2570 / NVIDIA-NeMo#2347 / NVIDIA-NeMo#2369. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
arena and math_with_judge assert on the sorted key list of the whole verify payload, so adding the field to BaseVerifyResponse breaks them. Expectations only -- no behavior change. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
38003f3 to
847b1c1
Compare
|
@ananthsub what's the status of this, should we merge? |
|
Closing this. Since #3717 the seed request carries |
Closes #2610
Builds on #2611 (both touch
BaseVerifyResponse); please review that one first.#2114 landed the rollout correlation contract, but two gaps keep incident attribution manual.
It is one-directional.
BaseSeedSessionResponseis an empty model andBaseVerifyResponsecarries no identifier, so the environment-side handle — the container, browser context or provider session that actually consumed quota — never reaches the training side. #2122 states this explicitly: "No rollout identifier is added to public request schemas."It is gated on model-call capture.
server_utils.py:341-348only applies the rollout prefix to resources servers whenobservability_enabledis true, and that flag defaults off, socurrent_rollout_id()isNoneinside resources servers exactly in the runs where an incident happens.What this changes
plus a
rollout_correlation_enabledglobal-config key that turns on the resources-server prefix without turning on capture. It defaults to false, so nothing changes for existing runs — happy to flip the default if you would rather have correlation on by default.Why it matters
When a run degrades, the training side has rollout ids, the provider has environment session ids, and the only join key is a timestamp. Reconstructing which orphaned sessions belonged to which aborted rollouts across our own incident (1172 of 1280 rollouts over 20 steps) was done by hand; with a returned session id it would have been a
groupby. This is also the environment-side half of #238.No training framework needs a transport change: verl and NeMo-RL already carry the whole verify response as
full_result.Terminology note: this is exclusively the environment session created by
/seed_session, not the vLLM router KV-cache affinity of #2570 / #2347 / #2369.Tests
Round trip through seed and verify, plus
test_rollout_correlation.py: correlation is off by default, and the new flag prefixes resources-server calls without full observability.