Kit re-use per test session#6577
Conversation
github-actions
Bot
added
bug
Greptile SummaryThis PR introduces Kit session reuse across multiple test files within a single pytest subprocess, reducing the overhead of repeatedly starting and stopping the Isaac Sim Kit application. A new
Confidence Score: 4/5The batch execution path works correctly for normal runs, but startup hangs in a batch are reported as failures without the retry attempts that the single-file path performs, making the batch path more brittle and the error message actively misleading. The core session-reuse mechanism and JUnit report writing are sound. The one concrete defect is in _run_batch: when a batch subprocess hits a startup hang, the single-file path (_run_one_pass) would retry up to STARTUP_HANG_RETRIES=2 more times, but _run_batch calls capture_test_output_with_timeout exactly once and marks every file in the batch as STARTUP_HANG. The resulting error message ('retried N time(s)') actively conceals this by reporting retries that never happened, inflating false-failure counts and complicating root-cause analysis. tools/conftest.py — specifically the _run_batch function and its startup-hang error handling branch. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CI as conftest.py run_individual_tests
participant MB as _make_batches
participant RB as _run_batch
participant Sub as subprocess (pytest -p _kit_session_plugin)
participant KSP as _KitSessionPlugin
participant AL as AppLauncher (_shared_init)
CI->>MB: test_files, batch_size
MB-->>CI: [[file1, file2], [file3], ...]
loop each batch
alt multi-file batch
CI->>RB: batch, timeout, env
RB->>Sub: pytest -p _kit_session_plugin file1 file2
Sub->>KSP: pytest_configure → _patch_app_launcher
Sub->>AL: file1 AppLauncher(...) → original_init, saves _shared_launcher, no-ops close()
Sub->>AL: file2 AppLauncher(...) → copies __dict__ from _shared_launcher
Sub->>KSP: _kit_module_fence teardown file1 → _write_report + _reset_kit_state
Sub->>KSP: _kit_module_fence teardown file2 → _write_report + _reset_kit_state
Sub->>KSP: pytest_sessionfinish → drain _pending, flush remaining suites
RB-->>CI: xml_reports, file_statuses, failed_files
else single file
CI->>CI: _run_one_pass (existing path, with startup-hang retries)
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CI as conftest.py run_individual_tests
participant MB as _make_batches
participant RB as _run_batch
participant Sub as subprocess (pytest -p _kit_session_plugin)
participant KSP as _KitSessionPlugin
participant AL as AppLauncher (_shared_init)
CI->>MB: test_files, batch_size
MB-->>CI: [[file1, file2], [file3], ...]
loop each batch
alt multi-file batch
CI->>RB: batch, timeout, env
RB->>Sub: pytest -p _kit_session_plugin file1 file2
Sub->>KSP: pytest_configure → _patch_app_launcher
Sub->>AL: file1 AppLauncher(...) → original_init, saves _shared_launcher, no-ops close()
Sub->>AL: file2 AppLauncher(...) → copies __dict__ from _shared_launcher
Sub->>KSP: _kit_module_fence teardown file1 → _write_report + _reset_kit_state
Sub->>KSP: _kit_module_fence teardown file2 → _write_report + _reset_kit_state
Sub->>KSP: pytest_sessionfinish → drain _pending, flush remaining suites
RB-->>CI: xml_reports, file_statuses, failed_files
else single file
CI->>CI: _run_one_pass (existing path, with startup-hang retries)
end
end
Reviews (2): Last reviewed commit: "Address Greptile review: fix _pending dr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def pytest_sessionfinish(self, session, exitstatus) -> None: | ||
| """Flush any per-file report not yet written (e.g. fixture teardown skipped).""" | ||
| for filepath in list(self._suites): | ||
| self._write_report(filepath) |
There was a problem hiding this comment.
pytest_sessionfinish flushes suites but never drains _pending. Any test whose setup phase passed (creating an entry in _pending) but whose call phase never fires — because the session was aborted via --maxfail, KeyboardInterrupt, or a collection error — will simply disappear from the JUnit output. The conftest already wrote the file-level report path, so downstream tooling will count zero results for these tests rather than seeing them as errors, masking the gap entirely.
Consider iterating self._pending in pytest_sessionfinish and adding each orphaned case to its suite with an Error result before flushing.
| batch_env["ISAACLAB_REUSE_KIT_SESSION"] = "1" | ||
| # Make _kit_session_plugin importable in the subprocess (-p needs it on sys.path). |
There was a problem hiding this comment.
ISAACLAB_REUSE_KIT_SESSION=1 is injected into the subprocess environment but is never read by _kit_session_plugin.py or any other code. Session reuse is driven entirely by the -p _kit_session_plugin flag already present in the command. This dead env-var may mislead future maintainers into thinking it controls the reuse behaviour.
| batch_env["ISAACLAB_REUSE_KIT_SESSION"] = "1" | |
| # Make _kit_session_plugin importable in the subprocess (-p needs it on sys.path). | |
| # Make _kit_session_plugin importable in the subprocess (-p needs it on sys.path). |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| original_init = AppLauncher.__init__ | ||
|
|
||
| def _shared_init(self, launcher_args=None, **kwargs): | ||
| global _shared_launcher | ||
| if _shared_launcher is not None: | ||
| # Reuse the existing Kit session: copy all instance state. | ||
| self.__dict__.update(_shared_launcher.__dict__) | ||
| return | ||
| original_init(self, launcher_args, **kwargs) | ||
| _shared_launcher = self | ||
|
|
||
| AppLauncher.__init__ = _shared_init |
There was a problem hiding this comment.
SimulationApp
The shared-init patch intercepts AppLauncher.__init__ so subsequent files reuse the first launcher's state, but does nothing to intercept close() or __del__. If any test file in the batch calls something like simulation_app.close() in its teardown_module, the underlying SimulationApp is shut down, and every file that follows in the same batch will fail with cryptic startup/import errors rather than a clear "shared session was closed" message. There is no documented contract telling test authors that closing the launcher is forbidden in batch mode, nor any runtime guard that detects and surfaces this condition.
…rd SimulationApp.close - pytest_sessionfinish now drains _pending before flushing suites; tests interrupted between setup and call phases are recorded as Error entries instead of silently vanishing from JUnit reports. - Remove dead ISAACLAB_REUSE_KIT_SESSION env-var from _run_batch; session reuse is controlled entirely by the -p _kit_session_plugin flag. - Patch simulation_app.close() to a no-op after the shared launcher is created, preventing test-file teardown from shutting down Kit for remaining files in the batch.
|
@greptile review |
1 participant
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there