Remove USS sampling from the benchmark memory recorder

[fatimaanes]

Description

MemoryInfoRecorder sampled Unique Set Size on every update via
psutil.Process.memory_full_info(). That call walks the process page tables, so its cost
scales with resident size rather than being a constant counter read.

BenchmarkMonitor drives this recorder once per second from a background thread, and that
thread is started inside the timed region in all nine benchmark entry points
(entrypoints/runtime.py:186 plus the eight rl_games / rsl_rl / sb3 / skrl train and play
entry points). USS collection therefore ran concurrently with the workload each benchmark
was measuring.

Measured on this host (AMD EPYC 9124, Linux 6.8.0-87, psutil 5.9.8 per uv.lock), one
MemoryInfoRecorder.update() at 8 GB resident:

	before	after
update() median	71.42 ms	0.012 ms
allocating worker throughput (6 s)	12,320 ops	12,999 ops (+5.5%)
measurements emitted	12	8

RSS and VMS are read from memory_info(), which is a cheap counter read and is unaffected.

Why remove rather than budget or sample less often

An earlier proposal timed each sample against a budget and latched USS collection off once
it exceeded it. That keeps the metric nominally present while making it unreliable:

• The over-budget sample is still folded into the Welford statistics before the latch
  fires, so the reported mean/peak becomes an early-run prefix of a growing process rather
  than a run statistic.
• The budget is wall-clock around a call that releases the GIL, so on Linux it measures GIL
  re-acquisition latency as much as query cost and can latch off on a small process under
  thread contention.
• Whether the metric survives becomes a function of host memory size and page composition,
  so runs stop being comparable across machines.

Reducing the sampling interval has the same problem in weaker form: it lowers the duty cycle
but keeps an unbounded-cost call inside the timed region.

Because USS has no consumer (below), removing it avoids all of this and leaves no stale or
partial values behind.

The change is also deliberately confined to the recorder. The nine
with ... BenchmarkMonitor(benchmark, interval=1.0): lines are left byte-identical, because
downstream benchmark tooling pattern-matches the literal shape of those lines to inject
profiler capture anchors into Isaac Lab's entry points. Remedies that restructure the
with statement, rename the benchmark argument, or move sampling out of the monitor would
silently break that tooling; fixing the cost at its source does not.

Output change

These four measurements are no longer emitted:

• System Memory USS
• System Memory USS std
• System Memory USS peak
• System Memory USS n

MemoryInfoRecorder now always emits exactly 8 measurements (RSS and VMS, each with mean,
std, peak and n), so the emitted set is no longer platform-dependent.

I searched the repository, its history, docs, test fixtures and serialized output
expectations for consumers before removing anything:

• capture.py:346-348 reads only System Memory RSS, RSS std and RSS peak.
• The typed schema (benchmark/schema.py) has no USS field.
• formatters.py passes measurements through generically, so a removed row cannot raise.
• The console summary only matches names beginning Min/Max/Mean/Std.
• No JSON fixture, golden file, or documentation page lists a USS field.

The only in-repo references were the producer itself and assertions/comments in
test_recorders.py, both updated here. System Memory USS peak appears in a historical
CHANGELOG.rst entry, which is left untouched as a record of the past release.

On the downstream side: the benchmark harness that runs these workloads collects its own USS
figure directly from psutil rather than reading Isaac Lab's measurement, so removing this
field does not deprive it of the metric. Previously recorded System Memory USS series remain
in historical dashboard data; because they were produced while the sampler was perturbing the
run, they are not comparable with post-change runs and should be treated as a separate
baseline rather than a continuous series.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• This change requires a documentation update: no — no docs page documented the USS fields

Tests

Run with the repo .venv (Python 3.12.13, torch 2.11.0+cu128, psutil 5.9.8), pytest 9.1.1:

python -m pytest source/isaaclab/test/benchmark/test_recorders.py \
                 source/isaaclab/test/benchmark/test_memory_recorder_no_uss.py -q
# 73 passed in 4.64s

python -m pytest source/isaaclab/test/benchmark/ -q
# 11 failed, 346 passed, 1 skipped   (this branch)
# 11 failed, 327 passed, 1 skipped   (origin/develop e86463df8a, unmodified)

The failing set is byte-identical on both, so this branch introduces no regression. The 11
pre-existing failures are in test_api.py (10) and
test_asset_suite_runtime_semantics.py (1) and are unrelated to this change.

New regression coverage:

• test_memory_full_info_is_never_called — monkeypatches psutil.Process.memory_full_info
  to raise, so reintroducing the call fails the suite.
• test_no_uss_keys_in_runtime_data, test_get_data_measurement_names — no USS key or row
  is emitted.
• test_rss_and_vms_means_are_tracked — scripted RSS/VMS values, asserting mean, peak and n.
• test_benchmark_monitor_never_queries_uss, test_monitor_reports_no_recorder_exception —
  a live BenchmarkMonitor over a real benchmark, asserting the recorder still updates, the
  monitor records no exception, and its thread is joined.
• test_finalized_output_contains_no_uss, test_supported_formatters_still_serialize —
  omniperf, json, osmo and summary all still write output containing no USS field.
• test_entrypoints_keep_monitor_and_recorder_configuration — guards that each of the nine
  entry points still constructs a BenchmarkMonitor with use_recorders=True.

Lint and hooks:

ruff 0.14.10 check   -> All checks passed  (benchmark source + tests)
ruff 0.14.10 format  -> clean
codespell 2.4.1      -> clean
python tools/changelog/cli.py check --include-worktree
                     -> All modified packages have valid changelog fragments.

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/isaaclab/changelog.d/
• I have added my name to the CONTRIBUTORS.md or my name already exists there

🤖 Generated with Claude Code

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Removes USS memory sampling from benchmark recorder.

The PR appears safe to merge; the remaining feedback concerns the reliability of its regression tests.

Findings

1.  Entrypoint guard misses call changes ▶
2.  Thread count can mislead ▶

Summary

The PR removes per-tick USS collection and its four emitted measurements while retaining RSS and VMS recording. It adds regression coverage and a changelog fragment. Two test assertions should be tightened so they reliably guard the intended entrypoint shape and monitor lifecycle.

_{Reviews (1) · Last reviewed commit: "Merge branch 'develop' into fix/benchmar..."}

[isaaclab-review-bot]

Isaac Lab Review Bot

The recorder-local change removes expensive USS sampling while preserving RSS/VMS behavior, but it also removes established runtime keys and four serialized measurement rows without the repository-required deprecation bridge. The test additions include ineffective, duplicated, source-grep, and private-state assertions, and the implementation adds an overlong comment with a cost claim inconsistent with the supplied measurement.

• Design and architecture: Keeping the performance fix inside MemoryInfoRecorder avoids disturbing the nine benchmark monitor call sites and preserves the existing monitoring architecture. However, outright removal of USS must be replaced with a compatibility path that keeps the old behavior available during a documented deprecation period with a targeted warning and transition coverage.
• API: MemoryInfoRecorder.get_runtime_data() no longer provides uss_mean, uss_std, uss_peak, or uss_n, and get_data() no longer emits the four corresponding System Memory USS measurements. RSS and VMS names, units, statistics, and conversion behavior remain unchanged, but the removed producer outputs are established externally observable API.
• Compatibility and deprecation: Breaking changes: the System Memory USS, System Memory USS std, System Memory USS peak, and System Memory USS n serialized measurements, together with the uss_* runtime keys, are removed without an opt-in compatibility path, targeted deprecation warning, migration guidance, removal schedule, or transition tests. The changelog announces the removal but does not satisfy the required deprecation cycle.
• Implementation: The USS Welford state, peak tracking, memory_full_info() call, and emitted rows are consistently removed, while the RSS/VMS paths remain intact. Before merge, the implementation needs a deprecation bridge rather than immediate removal. The explanatory block at _get_runtime_info() should also be shortened and its unsupported “hundreds of milliseconds” claim corrected.
• Style consistency: The surrounding recorder structure remains consistent, but the new eight-line USS comment is disproportionate to adjacent section comments and duplicates release-note rationale. Its claim of hundreds of milliseconds at a few GB conflicts with the supplied 71.42 ms measurement at 8 GB; retain only the concise invariant that page-table walking must not occur in the timed region.
• Test quality: The direct recorder tests for avoiding memory_full_info(), excluding USS runtime/output keys, and preserving RSS/VMS statistics cover the core behavior. Cleanup is needed: the first monitor test can still pass after USS sampling returns because the exception is swallowed after RSS is recorded; it should be consolidated with the monitor exception test. The entrypoint test greps unchanged source using unrelated substring assertions, the finalized omniperf case duplicates the parametrized formatter case, and the hasattr checks pin private storage despite existing public-boundary coverage.

Significant concerns. Posted 6 actionable findings inline.

Automated review; human maintainers own approval decisions.

[AntoineRichard]

run-ci