Repository navigation
Conversation
The key terminology page called rollout and trajectory aliases and defined a "task attempt" as one rollout. The code uses the terms differently: EpisodeId identifies one execution of a rollout, a retry adds an attempt to the same rollout, and TrajectoryRecord is the data recorded for one episode. Rewrite the glossary to match the code. A rollout is one sample of a task and gets one reward. An episode is one execution of a rollout, and only the final episode's result counts. A trajectory is the record of an episode, made of agent invocations, turns, and steps. Attempt now means only the retry number, and trace now means an OpenTelemetry trace. Each entry names the matching term in other tools where one exists. Update the architecture and environments pages, which used "task attempt" for an episode and called the trajectory a rollout. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-4893-592b0c3c14f6.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — three RISK documentation-contract mismatches remain, plus one NOTE about ATIF step counting.
The separation between sampling and retries is useful, but the new definitions still misdescribe recorded turns, unscored failures, and checkpoint-restored episodes. These are documentation findings; no runtime regression is asserted.
Reviewed all three changed pages and traced the relevant execution, persistence, checkpoint, and export contracts. Validation was static only: tests and the Fern build were not executed. Suggested wording should be verified before adoption.
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.
| | **Agent** | Model + agent harness. | | ||
| | **Rollout driver** | Coordinates an episode and records its trajectory. | | ||
| | **Agent invocation** | One conversation held by the root agent or by a subagent within an episode. | | ||
| | **Turn** | One exchange within an agent invocation: a request to the agent and the agent's answer. A turn contains one or more steps. | |
There was a problem hiding this comment.
RISK — Distinguish agent exchanges from recorded trajectory turns
Failure / blast radius: One request to Simple Agent can make a tool-calling model request followed by a final-answer model request. The glossary predicts one turn containing two steps, but _create_episode() emits one TrajectoryTurn per model response (producer). The existing regression explicitly expects two turns, numbered 1 and 2, with step_count values 1 and 1 (assertions). Readers using this hierarchy to interpret ng_trajectory will therefore misidentify its boundaries and counts.
Correction: Qualify the Turn row and the diagram's turn label rather than changing the producer to fit the glossary. A replacement row could be:
| **Turn** | A harness-reported exchange or decision within an agent invocation. Simple Agent records each model response as a `TrajectoryTurn`, including tool-calling responses, so one request to the agent can produce several recorded turns. A user-facing dialogue exchange or agent activation is a separate boundary. |Change the diagram label to “harness-reported exchange or decision.” The invariant is that interaction-level terminology must not imply a different persisted TrajectoryTurn boundary.
Preservation / acceptance (not executed): Retain the existing two-turn assertion above, including step_count == [1, 1]; do not merge those records. A final answer without tools should still produce one recorded turn. Multiple tool calls requested in one model response should remain one recorded turn, not one turn per tool. Keep the existing user-facing meaning of multi-turn dialogue, but distinguish it from the saved-record boundary.
| | **Task attempt** | One rollout for a specific task. Multiple attempts per task capture different approaches and support pass@k. | | ||
| | **Trace** | Debug-oriented log of a rollout (timing, tool I/O, metadata) beyond the scored trajectory. | | ||
| | **Run** | One collection or evaluation job over one or more tasksets. Not the agent server's `/run` endpoint; call that "the agent's `/run` episode". | | ||
| | **Rollout** | One sample of a task. Each rollout gets one reward. `num_repeats` sets how many rollouts each task gets, and metrics such as pass@k count rollouts. A rollout is identified by its task and its repeat index. Other tools call this a *sample* (pass@k papers, Inspect) or a *trial* (Harbor). | |
There was a problem hiding this comment.
RISK — Do not promise a reward for infrastructure-failed rollouts
Failure / blast radius: A rollout whose last episode returns a terminal failure, or exhausts its retry budget, does not necessarily receive a reward. _episode_record() creates a failure record without a reward, and collection writes it to the failures sidecar rather than the scored output (conversion, persistence). Thus “each rollout gets one reward” incorrectly suggests that missing verification is a scored result, which matters when readers interpret metric denominators.
Correction: Replace the unconditional reward claims here and in the overview/tree, Episode, and Reward entries with the conditional contract. Suggested wording:
Each successfully scored rollout has one reward, supplied by its completing episode. An infrastructure-failed rollout can remain unscored; failed episodes are excluded from scores by default. Explicit failure/missing-rollout counting options can add metric-input zeros without inventing verifier rewards in the saved artifacts.
Keep the task/repeat identity and num_repeats definitions. The invariant is one selected score for a scored rollout—not a guaranteed verifier reward for every scheduled sample.
Preservation / acceptance (not executed): Retain the existing failure-counting regression: one HTTP-500 failure plus one reward-1 success yields one scored input and mean 1.0 by default; explicitly counting that failure yields two metric inputs and mean 0.5, while the failure sidecar still has no reward. Also check the documented retry example: an infrastructure failure followed by successful verification contributes only the successful reward, whereas num_repeats=2 creates two independent rollout identities.
| ### State | ||
|
|
||
| Per-task state that changes as the agent takes actions. Each task attempt starts from a clean state, this ensures attempts are independent and rewards are attributable to the agent's actions on that task. | ||
| Per-task state that changes as the agent takes actions. Each episode starts from a clean state, so episodes are independent and rewards are attributable to the agent's actions on that task. |
There was a problem hiding this comment.
RISK — Exempt checkpoint continuations from the clean-state guarantee
Failure / blast radius: A checkpoint restore increments the episode attempt while retaining session state. ResourcesParticipant.install() restores the saved state and binds the same session ID to next_attempt(...) (implementation). The existing counter regression performs two increments in attempt 1, restores into attempt 2, and expects the next increment to return 3—not 1 (regression). Under this PR's episode definition, those are different episodes, so they neither start clean nor constitute independent samples. This misstates the contract for readers implementing checkpoint-aware state/session handling.
Correction: Replace this paragraph with:
Per-task mutable state changes as the agent takes actions. A fresh rollout, or a retry restarted from its input, starts with clean, isolated state. A checkpoint-restored episode instead continues saved state from the same rollout; such episodes are not independent samples.
Make the corresponding qualification in the glossary's State, Episode, and Session entries and the “Each episode follows” introduction: setup-to-verify is the normal fresh-execution flow, not a requirement that restored episodes reseed. For Session, explicitly allow saved state/session identity to be restored for the rollout's next episode. A restored Simple Agent execution already has a regression asserting that it skips resource seeding (assertion).
Preservation / acceptance (not executed): Keep clean isolation for new samples and restart-from-input retries; do not change runtime behavior to reset restored state. Retain the counter regression's results 3 then 4 and its unchanged session ID, and the restored-agent regression's absence of /seed_session. Static acceptance: the edited definitions must distinguish fresh/restarted execution from checkpoint continuation while retaining the same logical rollout ID and incremented attempt.
| | **Rollout driver** | Coordinates an episode and records its trajectory. | | ||
| | **Agent invocation** | One conversation held by the root agent or by a subagent within an episode. | | ||
| | **Turn** | One exchange within an agent invocation: a request to the agent and the agent's answer. A turn contains one or more steps. | | ||
| | **Step** | One model call and the tool calls it requests. ATIF and agent RL papers use "step" in the same sense. | |
There was a problem hiding this comment.
NOTE — Limit the ATIF equivalence to agent steps
Failure / blast radius: ATIF's step is not universally one model call: Gym also exports initial system and user messages as steps (exporter). The existing two-model-call fixture exports four steps—system, user, agent, agent—and total_steps == 4 (step assertions, total). The unqualified equivalence can make readers interpret ATIF totals as model-call counts.
Correction: Replace this row with:
| **Step** | Here, one model call and the tool calls it requests. This corresponds to an ATIF agent step; ATIF also represents system and user messages as steps, so its total step count is not a model-call count. |This preserves the intended model/tool-cycle definition without redefining the external format.
Preservation / acceptance (not executed): Retain the existing export assertions for [system, user, agent, agent] and total_steps == 4. The first agent step must still contain both parallel tool calls, while the final no-tool answer remains another agent step. Static acceptance: the glossary explicitly says ATIF agent step, not ATIF step generally.
What does this PR do?
The key terminology page says rollout and trajectory are aliases, and it defines a "task attempt" as one rollout. The code uses these words differently.
EpisodeIdinnemo_gym/episode_types.pyidentifies "one physical attempt of a logical rollout": a retry underNEMO_GYM_MAX_ROLLOUT_ATTEMPTSkeeps the rollout ID and incrementsattempt, whilenum_repeatscreates new rollouts.TrajectoryRecordinnemo_gym/rollout_observability.pyis the data recorded for an episode. A reader who learns the vocabulary from the docs therefore misreads the code, and the architecture page contradicts the glossary on the same site.This PR rewrites the glossary to match the code:
num_repeatsand pass@k count rollouts.seed_session()throughverify(). A rollout usually has one episode; after an infrastructure failure Gym can run it again as a new episode, and only the final episode's result becomes the reward./runendpoint.The overview opens with the run → task → rollout → episode → trajectory hierarchy. Entries name the matching term in other tools where one exists: a rollout is a sample in pass@k papers and Inspect and a trial in Harbor, an episode runs from reset to done as in Gymnasium, and a step matches ATIF's step.
about/architecture.mdxcalled each task attempt an episode and the resulting trajectory a rollout; it now uses the same definitions.about/concepts/environments.mdxreplaces "task attempt" with "episode" where it describes per-episode state and scoring.No issue tracks this. The change is docs-only and sets the vocabulary that later reviewer-team and code-organization changes refer to.
Validation
npm run checkfromfern/(fern check)pre-commit run --fileson the changed pagesRollouts
N/A: docs-only change with no runtime behavior.
Compatibility and benchmark impact
None. No page moves, so no redirects are needed.
Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).