[benchmarking] Multiple fixes to stabilize the nightly benchmark suite

[rlratzel]

Summary

Bundles eight independent fixes uncovered while triaging the Curator
nightly benchmark suite against main (reference nemo-ci pipelines
52840568,
53098506,
53323567,
53333039,
and 53423863),
plus three environmental/observability changes added to enable fair A100
vs EOS comparison runs.

Pass-rate progression: 23/37 → 28/37 → 6/8 (scoped) → 1/2 (scoped) → 40/41
(full suite, 53423863). Sole remaining failure is video_embedding_xenna,
a new entry merged in from upstream main that needs a missing NVENC
library — out of scope for this PR.

1. Make undefined env vars in config non-fatal by default (b737202d)

• resolve_env_vars previously raised ValueError on the first undefined
  ${VAR} reference, halting the entire benchmark session even when the
  missing var was used by only a few entries.
• Default behavior is now to substitute an empty string and log a warning
  so unrelated entries can still run.
• Adds --strict-config-check CLI flag to run.py to restore the old
  fail-fast behavior.

2. New --entries-exact flag, exact-match entry filtering (ab1d4385, e8c6a52f, 7b351693)

Fixes a substring-aliasing bug in CI per-job invocations:

• --entries uses pytest's -k substring expression evaluator, so
  --entries audio_tagging_tts_xenna also matches
  audio_tagging_tts_xenna_repeat. In CI, where each per-job script runs
  --entries <entry-name>, this caused the non-_repeat job to also
  execute the _repeat entry, polluting that entry's per-entry results
  dir and crashing the subsequent legitimate _repeat SLURM job with
  Capture file ... already exists at Ray cluster setup.
• Adds --entries-exact accepting a comma-separated list of exact entry
  names. Every supplied name must match a configured (enabled) entry,
  otherwise the run aborts with a ValueError listing the unknown names
  alongside the available entry names.
• Mutually exclusive with --entries (CLI and Session.from_dict both
  enforce this).
• benchmarking/tools/ci_benchmark_launcher.sh switched from --entries
  to --entries-exact.
• Interactive --entries substring expression semantics are unchanged.

3. Bump too-tight timeout_s values for several entries

Multiple nightly-benchmark.yaml entries had timeout_s values that
didn't cover the actual wall time on the EOS H100 runner. These split
into two categories:

• Pre-existing tight limits that overshot during Ray teardown (Add style check #3.1).
• "No-op" timeouts — set back when the entry effectively did no work
  (e.g. rpv2 data was unreadable, or fuzzy_id_generator artifacts were
  stale and the test failed fast); once the underlying issue was fixed
  and the test ran for real, the historical ceiling was too tight (Add style check #3.2,
  Add style check #3.3).

3.1 ndd_ray_serve_dp4: 700 → 1200 (ab1d4385)

• SLURM-killed at the 700s --time ceiling ~13s after its benchmark
  subprocess succeeded — Ray teardown overhead pushed total wall over the
  wire.
• 1200s gives ~70% headroom over the observed warm-run wall.
• Reference failed job:
  https://gitlab-master.nvidia.com/dl/JoC/nemo-ci/-/jobs/327777542

3.2 exact_dedup_identification: 500 → 1500 (21647b4f)

• The 500s value was set when the test was effectively a no-op (the rpv2
  dataset was unreadable due to filesystem permissions, so the test failed
  fast on stat()). Once rpv2 access was restored, the test SLURM-killed
  at 68% (516/755) of the "Inserting into shuffler" phase.
• Shuffler runs at ~1.83 it/s × 755 items = ~410s on EOS H100, plus Ray
  cluster setup + dataset stat + post-shuffle dedup compute + cleanup ≈
  realistic wall 800-1000s.
• 1500s gives ~50% headroom.
• Reference failed job:
  https://gitlab-master.nvidia.com/dl/JoC/nemo-ci/-/jobs/329434352

3.3 dedup_removal_*: 1100/1500 → 1800/1800 (c2f3a6b0)

Once the upstream fuzzy_id_generator artifacts were refreshed (see
section 4 — the delete_scratch: false fix made this possible), the
dedup_removal tests actually ran the workload for the first time:

• dedup_removal_raydata: observed wall ~1419s (92% complete when killed
  at the prior 1100s ceiling) → 1800s (~27% headroom over the estimated
  1500s full wall).
• dedup_removal_xenna: passed at 1523s wall with the prior 1500s ceiling
  (zero margin) → 1800s as a preemptive bump to match raydata and absorb
  run-to-run variance.

Reference: nemo-ci pipeline 53333039 / leaf 53333328 — raydata killed by
SLURM TIME LIMIT during normal pipeline execution while xenna finished
with zero headroom.

4. Install lynx in the CI benchmark launcher (d0eac901)

The math benchmarks (math_preprocess, math_preprocess_classifier,
math_preprocess_llm_cleanup) shell out to the lynx text browser via
nemo_curator/stages/math/download/html_extractors/lynx.py for HTML
extraction. lynx is not in the Curator container, so those benchmarks
fail with RuntimeError: lynx executable not found in PATH.

lynx is GPL-licensed, so we deliberately do not bake it into the
redistributable Curator image. Instead it is installed transiently in the
existing benchmark container at CI run time, used during the run, and
discarded with the container. The published image stays GPL-free; the
apt-installed lynx only lives for the lifetime of each CI container.

5. (Reverted) Preserve scratch dir for fuzzy_dedup_identification (63691509 then a3c38ac6)

Initial commit 63691509 added delete_scratch: false to the
fuzzy_dedup_identification entry to keep its scratch artifacts around for
the downstream dedup_removal_* benchmarks. In practice the consumption
path is the canonical dataset path:

{datasets_path}/cleaned_exact_dedup_all_cc_fuzzy_output_nightly_container_paths/
  fuzzy_id_generator.json
  FuzzyDuplicateIds/

Operators promote a known-good fuzzy_dedup output to this path once, and
the dedup_removal entries consume it from there on every subsequent
pipeline — there is no same-pipeline dependency. Given that workflow, the
per-entry delete_scratch: false override is unnecessary and just leaves
unused data on lustre across runs. Reverted by a3c38ac6; the entry now
uses the session-level default (delete_scratch: true).

6. Range domain_label_games_count metric +/-1% (0c375b7e, tightened in ab31cf11)

Both domain_classification_raydata and domain_classification_xenna
had a requirement on domain_label_games_count with exact_value: 149816.
Run-to-run output of the classifier drifts by several hundred to a few
thousand classifications, causing repeated false failures while the actual
benchmark (throughput, total docs, number of domains predicted) is
healthy.

Loosens the metric to a +/-1% range (min_value: 148318,
max_value: 151314) which still catches genuine regressions of the
classifier output while tolerating normal run-to-run variability.
domain_label_news_count is left at exact_value: 2817 pending further
investigation.

7. Raise A100 container memory cap to host capacity (fd6e9cfd)

The benchmarking container on the A100 host was capped at 1 TiB RAM (and
512 GiB shm, derived as 50% of container memory) by
_max_container_memory_bytes = 1 * _TB in
benchmarking/tools/gen_runscript_vars.py. The A100 host actually has
~2 TiB RAM and ~1008 GiB /dev/shm — comparable to the EOS H100 host.
The artificial cap made A100's env.json report ~50% of host RAM and
~25% of host shm, surfacing a fake environmental delta in A100 vs EOS
comparison runs even though the workload (Ray plasma capped at 500 GB
on both sides) was bounded identically.

Raises the cap to 2 * _TB so A100's container memory and shm reflect
host capacity. After this change, A100 env.json reports
total_system_memory_bytes ≈ 2.16 × 10¹² and shm_size_bytes ≈
1.08 × 10¹² — matching EOS.

Also removes an experimental CURATOR_SHM_SIZE_BYTES env-var block in
ci_benchmark_launcher.sh that attempted to cap EOS-side /dev/shm via
mount -o remount,size=…. Pyxis containers on EOS don't grant
CAP_SYS_ADMIN, so the remount silently fell back to the WARNING branch
and the cap never applied. Raising A100 up to match EOS is the
maintainable direction (the EOS shm ceiling cannot be capped down without
nemo-ci infrastructure changes), so the toggle is no longer needed.

8. Background per-GPU stats recorder (c2fd47e6)

Adds benchmarking/runner/gpu_stats_recorder.py — GPUStatsRecorder,
a context-managed daemon thread that polls all GPUs via gpustat
(already a curator dep) and writes one row per (timestamp, GPU) pair to
{session_entry_path}/gpustats.csv while each entry's benchmark
subprocess runs.

CSV columns:

column	content
timestamp_utc	ISO-8601 timestamp
gpu_id	NVML index
utilization_gpu_pct	compute utilization 0-100
utilization_memory_pct	memory_used / memory_total * 100
temperature_c	GPU temperature in °C
processes	JSON-encoded list of {pid, username, command, gpu_memory_usage}

Configurable via a new top-level YAML key in nightly-benchmark.yaml:

gpu_stats_recorder:
  interval_s: 1.0   # 1 Hz; set to 0 to disable

The recorder polls all NVML-visible GPUs, independent of
CUDA_VISIBLE_DEVICES. This lets us verify post-run whether Ray/Xenna
actually honored the visible-device mask in cross-host comparison runs
(any nonzero util on a masked GPU index = leakage).

Replaces the prior shell-based nvidia-smi -l poller (block removed from
ci_benchmark_launcher.sh) which lived outside the runner lifecycle,
wrote ad-hoc file names, and required a separate env-var toggle to enable.

9. Wire NEMO_CI_* env vars + Slack viewer-URL block (da4c6760)

Curator-side plumbing for an upcoming nemo-ci-hosted developer-launch
wrapper (curator_benchmark_launch.py, still under design). All three
edits are gated behind new env vars / flags so today's nightly schedule
is byte-identical to before.

• ci_benchmark_launcher.sh:

  • Honors NEMO_CI_SESSION_NAME when set; otherwise picks nightly-<TS>
    for scheduled (cron) pipelines and the legacy benchmark_run_<id>
    name for direct launch_pipeline.py launches.
  • Composes a run-viewer URL only when NEMO_CI_VIEWER_HOST is set,
    using the host-side lustre path and the resolved session name.
  • Passes --viewer-url and --run-reason through to run.py when set.

• run.py: new --viewer-url and --run-reason CLI flags. When
  --run-reason is set it lands in env_dict (and thus env.json +
  the Slack environment block). When --viewer-url is set the Slack
  sink's sink_config["viewer_url"] is patched in-process before
  sink.initialize(...) — no YAML schema change required.

• slack_sink.py: SlackSink reads optional viewer_url from
  sink_config; SlackParentMessage accepts viewer_url and renders
  a *Results viewer:* <{url}|open run> section between the
  overall-status block and the environment table, only when set.

The new NEMO_CI_* namespace is shared with the upcoming nemo-ci
launcher work and is intentionally generic (not curator-specific) so
other modules can opt in later.

Verification

Across multiple nemo-ci pipelines:

• 53098506
  — first verification of commits 1, 2, 3.1, 3.2: 28/37 pass (up from
  23/37 baseline). Confirmed audio_tagging_tts_xenna_repeat works
  (--entries-exact), ndd_ray_serve_dp4 works (timeout bump),
  exact_dedup_identification works (rpv2 + timeout),
  audio_tagging_tts_xenna works without HF_SECRET_KEY set.
• 53323567
  — scoped 8-entry run of commits 4, 5, 6: 6/8 pass (math_preprocess*
  all pass thanks to lynx install; domain_classification_* pass with the
  metric range; fuzzy_dedup_identification passes). The two failures
  (dedup_removal_*) were caused by a same-pipeline race with
  fuzzy_dedup_identification — fixed by operators promoting fresh
  fuzzy_id_generator.json artifacts to the canonical dataset path
  between runs.
• 53333039
  — scoped 2-entry run with fresh artifacts in place: dedup_removal_xenna
  passes (1523s, no margin); dedup_removal_raydata SLURM-killed by TIME
  LIMIT at 92% completion → addressed by commit c2f3a6b0 (Add style check #3.3).
• 53423863
  — full-suite verification after Add style check #3.3: 40/41 pass. Sole failure is
  video_embedding_xenna (new entry merged from upstream main, missing
  NVENC library — separate follow-up).
• 53584394
  — in flight. Verifies Add batched decorator #7 (cap bump doesn't break EOS) and Fix noisy CUDA shutdown #8 (recorder
  always-on writes gpustats.csv); also captures empirical evidence of
  whether CUDA_VISIBLE_DEVICES=0,1,2,3 actually restricts Ray/Xenna to
  4 GPUs on EOS.

Test plan

Undefined env vars

• audio_tagging_tts_xenna ran successfully in 53098506 with
  HF_SECRET_KEY unset.
• Run with --strict-config-check; confirm it still exits with the
  original ValueError.
• Run an unmodified config (all env vars defined); confirm no
  behavior change.

--entries-exact

• CI per-job invocation ran exactly one entry per job in 53098506
  and 53323567; no cross-entry pollution observed.
• --entries-exact <typo> exits with an error listing the unknown
  name and the available entry names.
• --entries-exact a,b,c runs only those three entries in YAML order.
• Passing both --entries and --entries-exact exits with a
  "mutually exclusive" error.

Timeout bumps

• ndd_ray_serve_dp4 finished without TIME LIMIT cancellation in
  53098506.
• exact_dedup_identification finished without TIME LIMIT in 53098506.
• dedup_removal_raydata and dedup_removal_xenna both finish
  without TIME LIMIT cancellation (verified in 53398187 / 53423863).

lynx install

• math_preprocess* entries all passed in 53323567 — RuntimeError: lynx executable not found no longer fires.

fuzzy_dedup scratch retention (now reverted)

• One-time artifact promotion completed: fresh
  fuzzy_id_generator.json + FuzzyDuplicateIds/ written to the
  canonical dataset path.
• dedup_removal_* consume the promoted artifacts in a clean
  pipeline (verified in 53398187).
• After artifact promotion, the per-entry delete_scratch: false
  override on fuzzy_dedup_identification is no longer needed and has
  been reverted (commit a3c38ac6).

domain_label_games_count range

• domain_classification_* both passed in 53323567 with the new
  range satisfied.

A100 container memory cap (#7)

• On the A100 nightly host, after pulling commit fd6e9cfd, a fresh
  run produces env.json with total_system_memory_bytes ≈ host total
  (~2.16 × 10¹²) and shm_size_bytes ≈ ~1.08 × 10¹².
• EOS pipelines continue to succeed (no regression observed from
  the unrelated ci_benchmark_launcher.sh shm-block removal —
  verified in 53584394 run-up).

GPUStatsRecorder (#8)

• After 53584394 completes, gpustats.csv exists under each
  per-entry session dir on EOS lustre.
• CSV header matches timestamp_utc,gpu_id,utilization_gpu_pct,utilization_memory_pct,temperature_c,processes.
• Setting gpu_stats_recorder.interval_s: 0 in the YAML disables
  the recorder (no file written); default (omitted key) gives 1 Hz.
• Recorder polls 8 rows per timestamp on an EOS node regardless of
  CUDA_VISIBLE_DEVICES (NVML-visible, not CUDA-runtime-visible).

🤖 Generated with Claude Code

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[praateekmahajan]

I'm wondering why this happened now?

[praateekmahajan]

Because even dynamo should not take 2700s rn since we upgraded versions to 1.1.0 (i.e.we can reduce that)

[rlratzel]

I need to investigate that. I know we had success on the DGX-A100 machine with 700s, but I don't know yet if a larger timeout is needed for the other machine running nightlies because it's slower in general, or if something else is causing a longer runtime on the other machine.

[praateekmahajan]

LGTM! Thanks for this 🙏

[greptile-apps]

Greptile Summary

This PR bundles eight independent fixes to stabilize the Curator nightly benchmark suite, improving the pass rate from 23/37 to 40/41 entries. Changes span timeout bumps, a new --entries-exact CLI flag to fix CI substring-matching aliasing, non-fatal undefined env-var handling, lynx transient install for math benchmarks, a GPUStatsRecorder background thread, and several observability/environment alignment improvements.

• --entries-exact (session.py, run.py, ci_benchmark_launcher.sh): replaces pytest-style substring --entries in CI per-job invocations to prevent one job from accidentally matching and running a sibling entry, with strict validation that every supplied name exists.
• GPUStatsRecorder (gpu_stats_recorder.py, run.py): new daemon-thread context manager that polls all NVML-visible GPUs at 1 Hz and writes per-GPU rows to gpustats.csv alongside each benchmark entry; configurable via top-level YAML gpu_stats_recorder.interval_s.
• Slack viewer URL / run-reason (slack_sink.py, run.py): new --viewer-url and --run-reason CLI flags intended to surface a results-viewer link in Slack parent messages, but the in-process sink_config patch does not propagate to self.viewer_url which was already set at init time.

Confidence Score: 4/5

Safe to merge for the benchmark stability fixes; the viewer-URL feature introduced here will silently not work until the one-line attribute assignment is corrected.

The core fixes — timeout bumps, --entries-exact, non-fatal env vars, lynx install — are all straightforward and well-verified in the referenced CI pipelines. The GPUStatsRecorder is a best-effort side-channel that cannot affect benchmark correctness. The only functional defect is that --viewer-url won't surface in Slack messages because self.viewer_url is set at SlackSink.init time and the post-init sink_config patch has no effect; this breaks a new observability feature but nothing that was working before.

benchmarking/run.py and benchmarking/runner/sinks/slack_sink.py for the viewer_url propagation issue; benchmarking/runner/gpu_stats_recorder.py for the CSV header discrepancy vs. documentation.

Important Files Changed

Filename	Overview
benchmarking/runner/gpu_stats_recorder.py	New GPUStatsRecorder context-manager daemon thread; CSV header (9 columns) is wider than the 6-column format documented in the PR description and test plan, and there is a narrow write-after-close window if the background thread survives the 10 s join timeout in stop().
benchmarking/run.py	Adds --entries-exact, --strict-config-check, --viewer-url, --run-reason CLI flags and wires GPUStatsRecorder; --viewer-url patches sink_config but not sink.viewer_url, so the URL is silently dropped from Slack messages.
benchmarking/runner/sinks/slack_sink.py	Adds viewer_url support to SlackParentMessage and SlackSink; self.viewer_url is read from sink_config only at init time and never refreshed, making the in-process patch from run.py a no-op.
benchmarking/runner/session.py	Adds entries_exact parameter to from_dict with correct mutual-exclusivity guard and informative ValueError when unknown names are supplied; logic is correct.
benchmarking/runner/utils.py	Refactors resolve_env_vars to accept a strict flag; non-strict mode now warns and substitutes empty string instead of raising ValueError. Clean implementation.
benchmarking/tools/ci_benchmark_launcher.sh	Switches --entries to --entries-exact, adds lynx install, session-name resolution, and optional viewer-URL construction; viewer URL is built with http:// which may not match the actual host scheme.
benchmarking/nightly-benchmark.yaml	Timeout bumps for exact_dedup_identification, dedup_removal_*, ndd_ray_serve_dp4; domain_label_games_count loosened to ±1% range; gpu_stats_recorder config added. All changes match the rationale in the PR.
benchmarking/tools/gen_runscript_vars.py	Raises _max_container_memory_bytes from 1 TiB to 2 TiB to reflect actual A100 host capacity. One-line change, straightforward.

Sequence Diagram

sequenceDiagram
    participant CLI as run.py (main)
    participant Session
    participant SlackSink
    participant RunEntry as run_entry()
    participant GPURec as GPUStatsRecorder
    participant Subprocess

    CLI->>Session: "from_dict(config, entries_exact=[...])"
    Session-->>CLI: session (filtered entries)

    CLI->>SlackSink: __init__(sink_config)
    Note over SlackSink: self.viewer_url = sink_config.get(viewer_url) = None

    CLI->>SlackSink: "sink_config[viewer_url] = args.viewer_url"
    Note over CLI,SlackSink: self.viewer_url unchanged (still None)

    CLI->>SlackSink: initialize(session_name, session, env_dict)
    SlackSink->>SlackSink: "_create_parent_message(viewer_url=self.viewer_url=None)"

    loop for each entry
        CLI->>RunEntry: run_entry(..., gpu_stats_recorder_interval_s)
        RunEntry->>GPURec: __enter__() start()
        GPURec-->>RunEntry: daemon thread polling gpustats.csv
        RunEntry->>Subprocess: run_command_with_timeout(cmd, timeout)
        Subprocess-->>RunEntry: run_data
        RunEntry->>GPURec: __exit__() stop()
        GPURec-->>RunEntry: CSV flushed and closed
        RunEntry-->>CLI: success/failure
        CLI->>SlackSink: register_benchmark_entry_finished(result)
    end

    CLI->>SlackSink: finalize()

Loading

_{Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile}

[greptile-apps]

--viewer-url silently dropped — self.viewer_url is never updated after __init__

self.viewer_url is assigned once from sink_config.get("viewer_url") at init time. In run.py, the CLI flag patches sink.sink_config["viewer_url"] = args.viewer_url after init, but that only mutates the backing dict — self.viewer_url still holds None when initialize() later calls _create_parent_message(viewer_url=self.viewer_url). The URL will never appear in the Slack parent message.

The fix is to also set sink.viewer_url = args.viewer_url directly in run.py, or to re-read it at the start of initialize().

[greptile-apps]

Potential write-after-close if background thread outlives 10 s join timeout

stop() calls self._stop_event.set(), then self._thread.join(timeout=10.0). If _poll_once blocks longer than 10 s (e.g., NVML hangs), the join returns but the thread is still alive. _csv_file.close() is then called immediately, leaving the running thread free to call self._csv_writer.writerow(...) on a closed file. Since _poll_loop swallows all exceptions, the next write silently fails or raises an OSError that is only logged at WARNING level — no data is lost for the benchmark itself, but the CSV could be left truncated or corrupted.

[greptile-apps]

CSV header in code does not match the documented 6-column format

The PR description and the test plan both say the expected CSV header is timestamp_utc,gpu_id,utilization_gpu_pct,utilization_memory_pct,temperature_c,processes, but the actual HEADER constant here contains 9 fields, adding power_draw_w, power_limit_w, and fan_speed_pct between temperature_c and processes. Any downstream consumer or CI validation that checks for the 6-column format documented in the test plan (CSV header matches timestamp_utc,...,processes) will fail.

[greptile-apps]

Viewer URL constructed over plain HTTP, not HTTPS

VIEWER_URL="http://${NEMO_CI_VIEWER_HOST}/..." hardcodes http://. If the viewer host is exposed over HTTPS (which is typical for internal dashboards), the link sent to Slack will be an unclickable or redirected HTTP URL. Consider using https:// by default, or parameterizing the scheme as part of NEMO_CI_VIEWER_HOST.

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!

[greptile-apps]

Since self.viewer_url is already set from sink_config at init time, patching sink.sink_config["viewer_url"] has no effect on the value _create_parent_message will see. Setting the attribute directly ensures the URL is visible when initialize() runs.

Suggested change

	if args.viewer_url:
	for sink in session.sinks:
	if getattr(sink, "name", None) == "slack":
	sink.sink_config["viewer_url"] = args.viewer_url
	if args.viewer_url:
	for sink in session.sinks:
	if getattr(sink, "name", None) == "slack":
	sink.sink_config["viewer_url"] = args.viewer_url
	sink.viewer_url = args.viewer_url