Skip to content

Commit 3bcd74c

Browse files
Gates the failure cap on generation_guarantee, as its own docstring promised
Review on #7433 caught a contradiction I introduced: the field's docstring says it is meaningful only together with generation_guarantee, but the loop check ran unconditionally. With the guarantee off and a cap below generation_num_trials, a fixed-attempt run would have ended before delivering the attempts it was asked for. The guarantee is also the only mode that needs the cap. Without it the existing check already stops on generation_num_trials attempts, so there is no unbounded run to bound and the cap has nothing to add. Gating the condition makes the code match the contract rather than loosening the contract to match the code. The attempt-mode test was parametrised over caps of None and 100 against 10 requested attempts, so it never exercised a cap that could fire; a cap of 3 is added, which is the case the reviewer identified as uncovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JFz91VMgmrjS4XR6f9Q9Z2
1 parent c7e64b5 commit 3bcd74c

3 files changed

Lines changed: 17 additions & 8 deletions

File tree

source/isaaclab/isaaclab/envs/mimic_env_cfg.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,13 @@ class DataGenConfig:
4141
max_num_failures: int | None = None
4242
"""Maximum number of failed generation attempts before stopping, or None for no limit.
4343
44-
Only meaningful together with :attr:`generation_guarantee`. With the guarantee enabled,
45-
generation keeps retrying until :attr:`generation_num_trials` demos succeed, so a task whose
46-
success rate is low can run for an unbounded number of attempts; this caps that. Defaults to
47-
None so the guarantee keeps its usual meaning unless a limit is asked for.
44+
Only applies together with :attr:`generation_guarantee`. With the guarantee enabled, generation
45+
keeps retrying until :attr:`generation_num_trials` demos succeed, so a task whose success rate is
46+
low can run for an unbounded number of attempts; this caps that. Defaults to None so the
47+
guarantee keeps its usual meaning unless a limit is asked for.
48+
49+
With the guarantee disabled, generation already stops after :attr:`generation_num_trials`
50+
attempts and this field is ignored, so setting it cannot cut a fixed-attempt run short.
4851
"""
4952

5053
seed: int = 1

source/isaaclab_mimic/isaaclab_mimic/datagen/generation.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,11 @@ def env_loop(
141141
break
142142

143143
# with the success guarantee on, nothing else bounds the run: a task that rarely
144-
# succeeds retries forever. max_num_failures is the opt-in bound on that.
144+
# succeeds retries forever. max_num_failures is the opt-in bound on that. Without the
145+
# guarantee the check above already stops on generation_num_trials attempts, so the
146+
# cap has nothing to add and must not cut a fixed-attempt run short.
145147
max_num_failures = env.cfg.datagen_config.max_num_failures
146-
if max_num_failures is not None and num_failures >= max_num_failures:
148+
if generation_guarantee and max_num_failures is not None and num_failures >= max_num_failures:
147149
print(
148150
f"Reached {num_failures} failures (max_num_failures={max_num_failures}) after"
149151
f" {num_success}/{generation_num_trials} successes. Exiting."

source/isaaclab_mimic/test/test_generation_failure_cap.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,13 @@ def test_cap_reached_before_enough_successes_ends_the_run():
108108
assert (succ, fail) == (2, 4)
109109

110110

111-
@pytest.mark.parametrize("max_num_failures", [None, 100])
111+
@pytest.mark.parametrize("max_num_failures", [None, 3, 100])
112112
def test_attempt_based_termination_is_unchanged(max_num_failures):
113-
"""With the guarantee off the run still stops on ``generation_num_trials`` attempts."""
113+
"""With the guarantee off the run stops on ``generation_num_trials`` attempts, cap or no cap.
114+
115+
A cap of 3 against 10 requested attempts is the case that matters: the fixed-attempt contract
116+
says the run delivers the attempts it was asked for, so the cap must not end it at 3.
117+
"""
114118
how, _, _, attempts = _run(
115119
[False] * 100, max_num_failures=max_num_failures, generation_num_trials=10, generation_guarantee=False
116120
)

0 commit comments

Comments
 (0)