Makes DataGenConfig.max_num_failures actually bound a Mimic generation run

[2047767028-lang]

Description

DataGenConfig.max_num_failures is documented as "Maximum number of failures allowed before stopping generation" and eighteen shipped Mimic environment configs set it to 25. Nothing reads it.

$ git grep -c max_num_failures
source/isaaclab/isaaclab/envs/mimic_env_cfg.py:1          # the definition
source/isaaclab_mimic/isaaclab_mimic/envs/*.py:18         # eighteen assignments

No third entry: there is no read anywhere in the repository, and the field has been inert since Isaac Lab Mimic was introduced in #179. env_loop counts num_failures and then never consults it; its only termination is

check_val = num_success if generation_guarantee else num_attempts
if check_val >= generation_num_trials:

With generation_guarantee = True (which those same eighteen configs also set), a task whose success rate is low retries without any bound, and the one knob that claims to stop that does nothing. On our own data generation this cost 337 attempts for 10 demos before we discovered the cap was not real.

Reproduction, on a stock task

./isaaclab.sh -p scripts/imitation_learning/isaaclab_mimic/generate_dataset.py \
  --task Isaac-Stack-Cube-Franka-IK-Rel-Mimic-v0 \
  --input_file ./datasets/annotated_dataset.hdf5 \
  --output_file /tmp/out.hdf5 \
  --generation_num_trials 30 --num_envs 10 --headless

FrankaCubeStackIKRelMimicEnvCfg sets max_num_failures = 25. Observed:

	successes	failures	attempts
before	30	50	80
after (max_num_failures = 25)	12	25	37

Before the change the run passed the configured cap and kept going, ending at twice it. After, it stops on the attempt that reaches 25 failures and says so:

Reached 25 failures (max_num_failures=25) after 12/30 successes. Exiting.

On the default

Wiring the field up exactly as written would abort the primary documented workflow. At the ~36% success rate measured above, --generation_num_trials 1000 needs on the order of 1800 failures, so a cap of 25 would end the run after roughly forty attempts. Since the eighteen assignments were inert when they were written and no current behaviour depends on them, they are removed here and the default becomes None, meaning no limit. Runs behave exactly as they do today unless a limit is explicitly requested.

If maintainers would rather the shipped configs keep an active cap, that is a one-line change per config and I am happy to make it — it just needs a value that suits large runs.

Tests

source/isaaclab_mimic/test/test_generation_failure_cap.py drives env_loop with a fake environment whose step() consumes scripted attempt outcomes and refills the action queue, so the loop's own termination logic runs unmodified with no simulator or dataset behind it; a step-count fuse turns an unbounded loop into a failed assertion. Six cases: the cap stops a run that never succeeds; None keeps the guarantee unbounded as before; enough successes still end the run first; a cap reached short of the target ends it; attempt-based termination with the guarantee off is untouched (×2).

Against the unpatched loop the two cap-dependent cases fail ('fuse' == 'exited', and (3, 4) == (2, 4) — the loop overran the cap and collected an extra success) and the four asserting unchanged behaviour pass.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package
• I have added my name to the CONTRIBUTORS.md or my name already exists there

🤖 Generated with Claude Code

https://claude.ai/code/session_01JFz91VMgmrjS4XR6f9Q9Z2

[greptile-apps]

Greptile Summary

The PR activates DataGenConfig.max_num_failures as an optional Mimic generation bound, changes its default to no limit, removes inert per-task assignments, and adds regression coverage.

• Stops success-guaranteed generation when the configured failure count is reached.
• Preserves existing shipped-task behavior by defaulting the cap to None and removing prior assignments of 25.
• Adds changelog entries and focused loop tests.

Confidence Score: 4/5

The attempt-based generation regression should be fixed before merging because a finite failure cap can terminate a run before its requested attempt count.

The new condition is unconditional even though both the configuration contract and existing branch logic distinguish success-guaranteed runs from fixed-attempt runs.

Files Needing Attention: source/isaaclab_mimic/isaaclab_mimic/datagen/generation.py and source/isaaclab_mimic/test/test_generation_failure_cap.py

Important Files Changed

Filename	Overview
source/isaaclab_mimic/isaaclab_mimic/datagen/generation.py	Adds the failure-bound termination, but applies it to attempt-based generation despite that mode's documented fixed-attempt contract.
source/isaaclab/isaaclab/envs/mimic_env_cfg.py	Changes the cap to an optional field and documents it as applicable to success-guaranteed generation.
source/isaaclab_mimic/test/test_generation_failure_cap.py	Covers the primary capped and uncapped paths, but its attempt-mode cases do not use a cap below the requested attempt count.
source/isaaclab_mimic/isaaclab_mimic/envs/franka_stack_ik_rel_mimic_env_cfg.py	Removes the formerly inert task-specific cap so the task retains unbounded success-guaranteed behavior by default.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Completed generation attempt] --> B{Guarantee enabled?}
  B -->|Yes| C{Successes reached target?}
  B -->|No| D{Attempts reached target?}
  C -->|Yes| E[Exit]
  D -->|Yes| E
  C -->|No| F{Failure cap configured and reached?}
  D -->|No| F
  F -->|Yes| E
  F -->|No| A

Loading

_{Reviews (1): Last reviewed commit: "Adds Kai Pei to CONTRIBUTORS.md" | Re-trigger Greptile}

[greptile-apps]

Failure cap overrides attempt mode

When generation_guarantee=False and max_num_failures is lower than generation_num_trials, this unconditional check exits after the capped number of failures instead of completing the requested number of attempts, violating attempt-based generation's documented behavior.

Suggested change

	if max_num_failures is not None and num_failures >= max_num_failures:
	if generation_guarantee and max_num_failures is not None and num_failures >= max_num_failures:

Knowledge Base Used: Imitation learning and data generation

[2047767028-lang]

Good catch, fixed in 3bcd74c — the check is now gated on generation_guarantee.

You are right that the docstring and the code disagreed, and gating is the better way to resolve it rather than loosening the wording: with the guarantee off, the existing check_val >= generation_num_trials already bounds the run, so the cap has nothing to add there and must not cut a fixed-attempt run short.

I also extended the field's docstring to say so explicitly, so the "does this apply to attempt mode" question does not have to be re-derived from the loop.

The attempt-mode test was parametrised over caps of None and 100 against 10 requested attempts, so it never exercised a cap that could actually fire — exactly the gap you identified. It now also runs with a cap of 3, which would have ended the run at 3 attempts before this change and completes all 10 after it.

[isaaclab-review-bot]

Isaac Lab Review Bot

The failure cap is now enforced in env_loop while preserving uncapped behavior by default, but two issues need correction: runtime behavior does not match the newly documented generation_guarantee scope, and the new regression test uses the wrong repository license header.

• Design and architecture: Keeping the failure bound alongside the existing termination checks is consistent with the current generation-loop design, and removing the previously inert per-environment limits preserves existing shipped-task behavior. The cap’s intended relationship to attempt-based generation must, however, be made consistent across implementation and documentation.
• API: max_num_failures is widened to int | None and defaults to None. Its docstring says it is meaningful only with generation_guarantee, but the unconditional loop check can also terminate attempt-based runs early. Either gate the check on the guarantee or document and test its effect in both modes.
• Implementation: The counter check correctly handles None and runs when attempt counters change. However, it currently ignores generation_guarantee, contrary to the documented contract. The new test file also uses an Apache-2.0 SPDX header rather than the repository’s current BSD-3-Clause source-file template.

Minor fixes needed. Posted 2 actionable findings inline.

Automated review; human maintainers own approval decisions.

[isaaclab-review-bot]

🟡 Warning · Api — Failure cap ignores documented generation_guarantee scope

This check runs regardless of generation_guarantee, but the docstring added in mimic_env_cfg.py states the field is "Only meaningful together with :attr:generation_guarantee". With the guarantee off and a cap below generation_num_trials, the run now ends before the requested attempts complete, which the documented contract says cannot happen. Either gate this condition on generation_guarantee or drop the "only meaningful" wording and document the attempt-based effect.

[isaaclab-review-bot]

🟡 Warning · Implementation — New file uses non-repository SPDX header

This new source file declares SPDX-License-Identifier: Apache-2.0 with Copyright (c) 2024-2026, while adjacent Isaac Lab sources such as mimic_env_cfg.py use the repository template Copyright (c) 2022-2026 with BSD-3-Clause. New source files must use the repository's current header template; copy the header from a neighbouring isaaclab_mimic file so the contributed file's licensing is stated correctly.

[2047767028-lang]

I think this one is a false positive — the file is in the right place for an Apache-2.0 header.

.pre-commit-config.yaml runs insert-license twice, splitting on path:

      - id: insert-license
        files: \.(pyi?|ya?ml)$
        args: [--license-filepath, .github/LICENSE_HEADER.txt, --use-current-year]
        exclude: "source/isaaclab_mimic/|scripts/imitation_learning/isaaclab_mimic/"
  # Apache 2.0 license for mimic files
      - id: insert-license
        files: ^(source/isaaclab_mimic|scripts/imitation_learning/isaaclab_mimic)/.*\.py$
        args: [--license-filepath, .github/LICENSE_HEADER_MIMIC.txt, --use-current-year]

.github/LICENSE_HEADER_MIMIC.txt is Copyright (c) 2024-<year> / SPDX-License-Identifier: Apache-2.0, and this file is source/isaaclab_mimic/test/test_generation_failure_cap.py, so that is the template it gets — matching every other file in source/isaaclab_mimic/test/.

mimic_env_cfg.py, which the finding compares against, lives in source/isaaclab/, which the first hook covers with the BSD-3-Clause template. That file is BSD in this same PR, and correctly so.

Happy to change it if maintainers prefer otherwise, but as it stands pre-commit would rewrite a BSD header here back to Apache-2.0.

[AntoineRichard]

Hi @2047767028-lang thanks a lot for the PR, @peterd-NV will take a look!

[peterd-NV]

Thanks for catching and fixing this. The changes look good to me. I have a minor comment about the handling of parallel envs and how that affects the strictness of the threshold (not a merge blocker).

[peterd-NV]

This limit is currently checked only after all generation attempts are completed by all envs during a parallel run. In theory this means that the actual number of trials can slightly exceed the max_num_failures as multiple envs cross that threshold.

The test case currently tests a single env so this would not occur. Handling of the parallel env case and an additional test can be added if the limit should be strict.

[2047767028-lang]

You are right, and the overshoot is measurable. Driving env_loop with scripted attempt outcomes, the recorded failure count for max_num_failures=5:

num_envs	attempts ending on the same step	failures	overshoot
1	1	5	0
2	2	6	1
4	4	8	3
10	10	10	5
4	staggered episode lengths	5	0

So the overshoot is the number of attempts that end on the step which crosses the bound, minus one: zero for a single environment, at most num_envs - 1, and zero whenever episodes happen to end on separate steps.

I could not find a way to drive it to zero. Those attempts are already complete when the bound is read — their episodes ended on the same step, so the outcomes exist before anything can react to the crossing. Suppressing them would mean not counting episodes the recorder has already written.

The obvious fix is also worse than it looks. Having run_data_generator return once the bound is reached deadlocks the run: waypoint.py puts one action per environment and awaits env_action_queue.join(), while env_loop blocks until qsize() == env.num_envs, so a single generator leaving parks the loop forever. Measured with the generators as real coroutines, num_envs=4 with staggered episodes and num_envs=10 both hang, and in the cases that do not hang the failure count is unchanged (8 and 6) — the overshoot never came from newly started attempts.

So 9fe2ba7 documents the guarantee on the field rather than changing behaviour, and adds two tests pinning both sides of it: exact when attempts end on separate steps, bounded by num_envs - 1 when they end together.

Would you like the strict version anyway? Making the bound exact would need env_loop to wait on a count of live generators instead of env.num_envs, so a generator can leave without stalling the loop — and even then it would only stop attempts from starting, not the ones already in flight on the crossing step. Happy to write it if you want it; it touches the generation loop itself, which felt like more than this fix should carry.

[kellyguo11]

run-ci