Skip to content

fix: preserve inner task names and raise on unknown task in EnvGroup#1216

Open
tashapais wants to merge 3 commits intoPrimeIntellect-ai:mainfrom
tashapais:fix/env-group-nested-routing
Open

fix: preserve inner task names and raise on unknown task in EnvGroup#1216
tashapais wants to merge 3 commits intoPrimeIntellect-ai:mainfrom
tashapais:fix/env-group-nested-routing

Conversation

@tashapais
Copy link
Copy Markdown
Contributor

@tashapais tashapais commented Apr 20, 2026

Summary

Two related fixes for EnvGroup task routing:

1. Nested EnvGroup silently misroutes all tasks to envs[0]

When an EnvGroup is wrapped inside another EnvGroup (as prime-rl does when registering user envs), the outer group was unconditionally overwriting the inner task names with a single outer name. The inner EnvGroup then could not find those outer names in its own env_map and silently fell back to envs[0] for every rollout, and scored everything as 0.0.

Fix: when a sub-env is itself an EnvGroup, register each of its task names in the outer env_map pointing to the inner group, and leave the inner task column in the dataset unchanged. Routing then flows outer -> inner group -> correct sub-env as expected.

2. Silent fallback masks misconfiguration

get_env_for_task was returning envs[0] for any unrecognized task. This made it impossible to notice routing failures. It now raises ValueError with the list of available task names.

Test plan

  • test_get_env_for_task updated: unknown task raises ValueError instead of returning envs[0]
  • test_nested_env_group_preserves_inner_tasks: wrapping an EnvGroup preserves inner task names in dataset and env_map
  • All existing TestEnvGroup and TestEnvGroupRubric tests still pass

Closes #1008


Note

Medium Risk
Changes core rollout routing behavior in EnvGroup (now raises on unknown tasks and alters nested task mapping), which could surface previously-hidden misconfigurations and break callers relying on fallback behavior.

Overview
EnvGroup task routing is now strict and nesting-safe. EnvGroup preserves inner task labels when an EnvGroup is wrapped inside another EnvGroup, expands the outer env_map to include the inner task names, and rejects task-name collisions at construction time.

Misconfiguration is no longer silent. get_env_for_task now raises a ValueError listing available tasks rather than defaulting to envs[0]; tests were updated/added for unknown-task errors, nested routing, and name-collision handling, and docs were updated to describe the new behavior (including env_names in the reference).

Reviewed by Cursor Bugbot for commit 50dbd92. Bugbot is set up for automated code reviews on this repo. Configure here.

When an EnvGroup is wrapped inside another EnvGroup, the outer group
was unconditionally overwriting the inner task names with a single
outer name, then silently falling back to envs[0] when routing failed.

Two fixes:
- When a sub-env is itself an EnvGroup, register each of its task names
  in the outer env_map pointing to the inner group, and keep the inner
  task column as-is so routing works through both levels.
- get_env_for_task now raises ValueError on unknown tasks instead of
  silently misrouting to envs[0], making misconfiguration immediately
  visible.

Fixes PrimeIntellect-ai#1008

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread verifiers/envs/env_group.py Outdated
Comment thread verifiers/envs/env_group.py Outdated
Comment thread verifiers/envs/env_group.py
@tashapais
Copy link
Copy Markdown
Contributor Author

Fixes #1008. Two changes: (1) when a sub-env is itself an EnvGroup, its inner task names are now registered in the outer env_map (pointing to the inner group) rather than being overwritten with the outer name -- routing then flows correctly through both levels; (2) get_env_for_task raises ValueError on unknown tasks instead of silently falling back to envs[0].

Three issues found in review:

1. Name collision: inner task names from a nested EnvGroup now raise
   ValueError at construction if they conflict with a sibling env's
   name, rather than silently overwriting the sibling's env_map entry.

2. Stale outer name: the outer name (e.g. "my_env") is now removed from
   env_map after its inner task names are expanded, so get_env_for_task
   on the outer name raises cleanly instead of routing to the inner group
   which would then also raise for the same unknown name.

3. Docs: update docs/environments.md to document the ValueError on unknown
   tasks, nested EnvGroup support, and the name-uniqueness requirement.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@tashapais
Copy link
Copy Markdown
Contributor Author

Addressed all three Bugbot findings in the latest commit (cebd14c):

  1. Name collision (medium): inner task names from a nested EnvGroup now raise ValueError at construction if they conflict with a sibling env's name, instead of silently overwriting it.

  2. Stale outer name (low): the outer name (e.g. "my_env") is now removed from env_map after its inner task names are expanded, so get_env_for_task("my_env") raises cleanly rather than returning the inner group which would then immediately raise for the same name.

  3. Docs (low): docs/environments.md updated to cover the ValueError on unknown tasks, nested EnvGroup support with a code example, and the name-uniqueness requirement.

@willccbb @mikasenghaas happy to make any adjustments.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit cebd14c. Configure here.

Comment thread verifiers/envs/env_group.py Outdated
Comment thread verifiers/envs/env_group.py Outdated
Comment thread docs/environments.md
Three issues from bugbot review:

1. The unconditional pop(name) removed a mapping just registered when
   the outer name happened to equal an inner task name. Now only pops
   if outer name is not itself an inner task name.

2. The nested EnvGroup registration logic was duplicated across the
   train and eval dataset branches. Extracted into _register_nested_env_map
   so the collision check, registration, and conditional pop live in one
   place.

3. docs/reference.md EnvGroup entry was not updated to reflect the
   ValueError on unknown tasks or nested EnvGroup support.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@tashapais
Copy link
Copy Markdown
Contributor Author

Addressed the new bugbot findings in commit 50dbd92:

  1. Unconditional pop (medium): pop(name) is now conditional -- it only removes the outer name if that name is not itself one of the inner task names, preventing it from undoing a registration just made.

  2. Duplicated logic (low): Extracted _register_nested_env_map helper so the collision check, registration, and conditional pop live in one place. Both the train and eval dataset branches now call the helper.

  3. reference.md not updated (low): docs/reference.md EnvGroup entry now documents the ValueError on unknown tasks, nested EnvGroup support, and the name-uniqueness requirement.

Copy link
Copy Markdown
Member

@mikasenghaas mikasenghaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, i quite like this. we don't need this for prime-rl anymore since we moved away from EnvGroup for multi-env training but I still think supporting arbitrary nesting of EnvGroup might be nice. thoughts? @willccbb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EnvGroup nested in outer EnvGroup silently misroutes all tasks to envs[0]

2 participants