Share one Kit app across test files instead of booting it per file - #6853
Draft
mataylor-nvidia wants to merge 7 commits into
Draft
Share one Kit app across test files instead of booting it per file#6853mataylor-nvidia wants to merge 7 commits into
mataylor-nvidia wants to merge 7 commits into
Conversation
Kit-dependence is currently a property of importing a test file: 156 test modules construct AppLauncher at module scope, so Isaac Sim boots during pytest collection. Because nothing declares that dependency, tools/conftest.py has to run every test file in its own subprocess, paying Kit startup once per file. Introduce the two pieces needed to change that: launch_kit() is an idempotent module-scope replacement for AppLauncher. The first test module in a process boots Kit; later modules receive the running app, so a pytest run covering several files pays startup once. It raises rather than silently returning a mismatched app when a file asks for cameras after a camera-less boot. The kit / kit_cameras / kitless markers let a file declare which launch configuration it needs, so files that can share a process can be grouped without importing them. kit_solo opts a file out of any such grouping. test_kit_marker_contract.py keeps the markers from drifting: it checks by AST that a file's declaration matches what it does at module scope. The checks are AST-based rather than text-based because several kit-free files mention AppLauncher only in a docstring saying they do not use it. Files are not yet required to carry a marker; _ENFORCED_ROOTS is empty and grows per package as files are migrated. No test file changes behaviour: nothing is marked kit or kitless yet, and no file calls launch_kit() yet. The guard found one pre-existing bug on its first run. test_operational_space assigned pytestmark twice, and the second assignment discarded arm_ci, so the file had been excluded from the ARM CI lane. Merged into a single list.
mataylor-nvidia
force-pushed
the
mataylor/kit-test-markers
branch
from
August 2, 2026 20:33
8c337c5 to
57ffdbd
Compare
Replace the module-scope AppLauncher construction in the Kit-dependent files under source/isaaclab/test/sim with launch_kit(), and declare the matching kit or kit_cameras marker on each file. Because launch_kit() is idempotent, a pytest process covering several of these files now boots Kit once instead of once per file. Nothing forces them into one process yet -- tools/conftest.py still runs a subprocess per file -- so this changes how the files launch Kit, not how CI schedules them. 24 files map to `kit` and 4 to `kit_cameras`. The two groups must not share a process in that order: a camera-enabled app can serve tests that do not need cameras, but cameras cannot be enabled after startup, so launch_kit() raises rather than handing back an app that would silently fail to render. The transform is applied by tools/codemods/kit_launch_migration.py, added here because ~125 files in other packages remain to migrate. It edits line ranges in place rather than round-tripping through ast.unparse, which would discard comments and isort directives, and it preserves each launch call's position so the Kit-dependent imports below it still run after Kit starts. The codemod refuses anything it cannot rewrite without changing behaviour, and reports it. In particular it rejects a conditional launch such as `AppLauncher(...).app if _USE_KIT else None`, which test_mjcf_converter.py and test_urdf_converter.py use so they can run kitlessly when the standalone importer wheel is installed; collapsing that ternary would have made the boot unconditional. It also refuses a file that references AppLauncher for anything other than the launch call, since the import is removed.
Whether migrating the remaining ~125 test files off module-scope AppLauncher is worth doing depends on how much Kit startup actually costs, which is not something the current pipeline reports directly. Add two temporary jobs that run the same 30 files from source/isaaclab/test/sim and differ only in how many Kit apps they boot. kit-reuse-probe-per-file keeps the default test-path of "tools", so tools/conftest.py gives each file its own subprocess and Kit boots 30 times. kit-reuse-probe-batched points pytest at the files directly, so they share one process and launch_kit() boots Kit once. The difference between the two job durations is what reuse is worth per 30 files. Both jobs list their files explicitly instead of selecting with `-m kit`, because pytest's marker filtering deselects tests but still imports every collected module, and importing a kit_cameras module calls launch_kit(cameras=True) regardless of whether its tests will run. The batched job lists the four kit_cameras files first: a camera-enabled app can serve tests that do not need cameras, but cameras cannot be enabled after startup, so the opposite order makes launch_kit() raise. Files in TESTS_TO_SKIP are excluded from both sides so the jobs cover the same tests. To let a job bypass the per-file orchestrator, run-package-tests gains a test-path input. It defaults to "tools", the value that was previously hard-coded, so every existing caller is unaffected. Both jobs are continue-on-error and are meant to be deleted once the measurement is recorded.
mataylor-nvidia
force-pushed
the
mataylor/kit-test-markers
branch
from
August 2, 2026 22:19
57ffdbd to
ded9dfd
Compare
The per-file runner grants the first camera-enabled test file an extra 700 s of timeout, because that file compiles RTX shaders (~600 s) on a cold cache. It identified such files by searching their source for the literal string "enable_cameras=True". Migrating a file to launch_kit(cameras=True) removes that literal, so the buffer stopped being applied and the file was killed at the 120 s startup deadline instead. That is what happened to test_simulation_stage_in_memory.py in the kit-reuse-probe-per-file job: it was reported as a startup hang at 120.94 s having run no tests. Match the marker and the launch_kit call as well as the old literal, so the buffer applies both before and after a file is migrated. Also narrow the probe to the 24 `kit` files and drop the four `kit_cameras` ones from both sides. The cold shader compile is roughly thirty times the Kit startup the probe is trying to measure, so including those files tells us about shader caching rather than about app reuse.
8 tasks
The kit-reuse-probe-batched job surfaced two ways a test file can misbehave once it no longer has a Kit process to itself. test_simulation_stage_in_memory.py aborted the interpreter immediately after collection, with no Python traceback, while the same test passes in its own process. Creating the stage in memory is sensitive to what else has already touched the stage or the extension set. The cause is not understood yet, so mark the file kit_solo to keep it out of any future batching rather than leave a landmine for whoever wires that up. test_views_xform_prim.py calls enable_extension() at module scope, so in a shared process it mutates the running app's extension set during collection, before any test runs. That is harmless today and the file is not being changed, but it is the kind of import-time side effect that batching turns into a cross-file interaction, so say so at the call site. Neither change affects how these tests run today; both files still get their own process from tools/conftest.py.
The batched probe was passing over files rather than classifying them: the four kit_cameras files had been dropped from both sides to keep the measurement clean, which meant three files that can share an app were being run one-app-each for no reason. Mark the files that genuinely cannot share, and batch everything else. test_views_xform_prim.py is the one this run identified. Its test_compare_get_world_poses_with_isaacsim reaches Isaac Sim's SimulationManager, a process-global singleton that caches the PhysxScene wrapping /physicsScene. In a shared process that prim belongs to a stage an earlier file has already torn down, so the cached wrapper is dangling and the test fails with "Accessed invalid expired 'PhysicsScene' prim". The other 62 tests in the file are fine; the marker is per file, so the file goes solo until SimulationManager can be reset between files. That leaves 26 of the directory's files sharing one app, up from 24, with two marked kit_solo and one already in TESTS_TO_SKIP. The three kit_cameras files are listed first in the batched job because a camera-enabled app can serve tests that do not need cameras while the reverse makes launch_kit() raise. Both jobs run the identical 26 so the durations stay comparable.
Both probe jobs carried a hand-written list of the files that can share a Kit app, duplicated between them in two different formats. That has to be edited by hand whenever a file is added, renamed, or reclassified, and the two copies have to be kept identical or the timing comparison silently stops comparing like with like. A stale list is wrong quietly rather than loudly. The markers already record which files can share an app, so make them the only source. tools/kit_test_files.py selects the files marked kit or kit_cameras, drops those marked kit_solo and those in TESTS_TO_SKIP, and puts the kit_cameras files first because a camera-enabled app can serve tests that do not need cameras while the reverse makes launch_kit() raise. Each job calls it in a step and passes the result through, so the two jobs cannot disagree with each other or with the markers. A marker expression still cannot replace this: pytest's -m deselects tests but imports every collected module regardless, so it cannot stop a kit_cameras module from calling launch_kit(cameras=True) in a run that booted without cameras. The list has to be settled before pytest starts. Markers are read from the source text rather than by importing the modules, since importing a Kit-dependent test module boots Kit. test_kit_marker_contract.py now also checks the two invariants a caller depends on: the derived list matches the markers, and cameras sort first. Verified the ordering check fails when the script's ordering is reversed.
kellyguo11
added a commit
that referenced
this pull request
Aug 3, 2026
# Description Fixes #5302. Fixes #6853. Manual direct workflows call the cloner after spawning their assets, but most of them only authored per-environment collision groups on CPU—or did not author them at all. The PhysX replication path currently uses USD collision filtering instead of PhysX environment IDs, so CUDA replicas could collide across environments and constrain articulated joints despite valid effort targets. This change: - applies collision filtering on every PhysX simulation device for all affected in-repo tasks and standalone scripts using the manual direct-workflow cloning path; - preserves each affected workflow's global ground or terrain collision paths; - guards the filtering by backend so Newton behavior is unchanged. The deterministic 4,096-environment reproduction now reports identical cart velocity in every environment after 20 steps at 100 N: ```text min=6.7209768 m/s, max=6.7209768 m/s, mean=6.7209768 m/s ``` ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## Screenshots Not applicable. ## Validation - Audited standalone cloning scripts and updated the two affected direct-workflow paths. - Confirmed the original 4,096-environment constant-effort diagnostic produces identical velocities across replicas. - Confirmed the Newton MJWarp control remains consistent across four replicas. - `uv run isaaclab -f` - `uv run --no-sync python tools/changelog/cli.py check issue-5302-base` - Confirmed Cartpole rendering correctness passes for the PhysX/Isaac Sim RTX and PhysX/Newton renderer combinations (2 passed). - Confirmed the registered Cartpole and Shadow Hand camera tasks match their updated collision-group stage goldens. ## Checklist - [x] I have read and understood the contribution guidelines. - [x] I have run the pre-commit checks. - [x] Documentation changes are not required for this behavior-only fix. - [x] My changes generate no new warnings. - [x] Existing environment tests cover the affected behavior. - [x] I have added a changelog fragment for the touched package. - [x] My name already exists in `CONTRIBUTORS.md`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Kit-dependence is currently a property of importing a test file. 156 test modules construct
AppLauncherat module scope, so Isaac Sim boots during pytest collection:Nothing declares that dependency, so
tools/conftest.pycompensates by running every test file in its own subprocess. Kit startup is therefore paid once per file, and there is no way to ask "which tests actually need Kit?" without importing them.Change
launch_kit()(isaaclab/test/launch.py) replaces the module-scopeAppLauncher. It is idempotent: the first module in a process boots Kit, later modules get the running app. It stays at module scope because a test module's own imports (pxr,omni, ...) run at collection, before any fixture could help.kit/kit_cameras/kitlessmarkers let a file declare its launch configuration, so files that can share a process can be grouped without importing them.kit_soloopts a file out.test_kit_marker_contract.pykeeps the markers from drifting by checking, via AST, that each file's declaration matches what it does at module scope. It is AST-based rather than text-based because several kit-free files mentionAppLauncheronly in a docstring saying they do not use it — a grep flags those, an AST walk does not.conftest.pyis unchanged.Scope
source/isaaclab/test/simis migrated as the pilot: 27 files map tokit, 4 tokit_cameras. The other ~125 files across other packages are untouched and keep working exactly as before — an unmarked file is treated as legacy and still gets its own process._ENFORCED_ROOTSin the guard is empty, so no file is yet required to carry a marker; it grows per package as migration proceeds.The transform is applied by
tools/codemods/kit_launch_migration.py, added because those ~125 files remain. It edits line ranges in place rather than round-tripping throughast.unparse, which would discard comments and isort directives, and preserves each launch call's position so Kit-dependent imports below it still run after Kit starts.Ordering constraint
kitandkit_camerasfiles must not share a process in that order. A camera-enabled app can serve tests that do not need cameras, but cameras cannot be enabled after startup, solaunch_kit()raises rather than handing back an app that would silently fail to render.Relatedly,
pytest -mdeselects tests but still imports every collected module, so marker filtering alone cannot keep akit_camerasfile out of akitrun. Both probe jobs list their files explicitly for that reason.Probe (temporary)
kit-reuse-probe-per-fileandkit-reuse-probe-batchedrun the same 30 files fromsource/isaaclab/test/simand differ only in how many Kit apps they boot — 30 versus 1. The gap between the two job durations is what reuse is worth per 30 files, and it decides whether migrating the remaining files is justified. Files inTESTS_TO_SKIPare excluded from both sides so the two jobs cover the same tests.To let a job bypass the per-file orchestrator,
run-package-testsgains atest-pathinput defaulting to"tools"— the previously hard-coded value — so every existing caller is unaffected.Note that migrating files does not by itself speed up CI, because
tools/conftest.pystill gives each file its own process. Teaching the runner to batch same-profile files is separate work, deliberately not in this PR.Incidental fix
The guard found a real bug on its first run:
test_operational_space.pyassignedpytestmarktwice, and the second assignment discardedarm_ci, so that file had been excluded from the ARM CI lane. Merged into a single list.Testing
test_kit_marker_contract.pypasses (~1.7s, no Kit). Verified it has teeth by planting five deliberate violations — wrong marker vs.AppLauncher,kitlessimportingomni,unitcallinglaunch_kit,kit_cameraswithoutcameras=True, two profile markers at once — and confirming each rule fires.uv run isaaclab -fclean.🤖 Generated with Claude Code