Skip to content

Add ray to each benchmarking entry and make sure no benchmarking/script has RayClient - #1593

Merged
praateekmahajan merged 2 commits into
NVIDIA-NeMo:mainfrom
praateekmahajan:praateek/verify-benchmarks-have-ray-setup
Mar 11, 2026
Merged

Add ray to each benchmarking entry and make sure no benchmarking/script has RayClient#1593
praateekmahajan merged 2 commits into
NVIDIA-NeMo:mainfrom
praateekmahajan:praateek/verify-benchmarks-have-ray-setup

Conversation

@praateekmahajan

@praateekmahajan praateekmahajan commented Mar 10, 2026

Copy link
Copy Markdown
Contributor
  1. Added missing ray: config (num_cpus: 64, num_gpus: 4) to:
    • domain_classification_xenna
    • embedding_generation_xenna
    • audio_fleurs
  2. Removed RayClient import, ray_client.start(), and ray_client.stop(). The Ray cluster is already started by run.py.
    • image_pipeline_benchmark.py
    • multimodal_mint1t_benchmark.py
  3. Fixed success_code = 0 (should be 1) in arxiv_e2e_pipeline_benchmark.py which masked benchmark failures as successes
  4. Replaced hardcoded XennaExecutor() in audio_fleurs_benchmark.py with setup_executor() + --executor CLI arg, consistent with all other benchmark scripts
  5. Fixed potential UnboundLocalError crashes across multiple scripts (exact_dedup_identification, score_filter, modifier, video_pipeline, image_pipeline, fasttext_filter, semdedup_identification, dedup_removal) by initializing variables before conditional/try blocks

Description

Usage

# Add snippet demonstrating usage

Checklist

  • I am familiar with the Contributing Guide.
  • New or Existing tests cover these changes.
  • The documentation is up to date with these changes.

…ent in it

Signed-off-by: Praateek <praateekm@gmail.com>
Comment on lines 183 to 198
@@ -187,16 +194,6 @@ def main() -> int:
rmm_pool_size=args.rmm_pool_size,
spill_memory_limit=args.spill_memory_limit,
)

except Exception as e:
print(f"Benchmark failed: {e}")
results = {
"params": vars(args),
"metrics": {
"is_success": False,
},
"tasks": [],
}
finally:
write_benchmark_results(results, args.benchmark_results_path)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled NameError can propagate past finally

run_exact_duplicate_identification_benchmark() has an internal except Exception block that sets success = False, but it does not define workflow_result when the exception occurs before line 77 (workflow_result = workflow.run(...)). The return at line 118 then references workflow_result, causing a NameError that escapes the inner function.

Previously, the outer except Exception in main() would have caught this and written clean fallback results. With the new try/finally-only pattern, the NameError propagates past finally (fallback results are still written correctly), but main() never returns — it exits via an unhandled Python exception traceback instead of a clean exit code.

If an unambiguous exit code is desired even in this edge case, consider adding a narrow guard in the inner function:

return {
    ...
    "tasks": workflow_result if success else [],
}

Comment on lines +156 to +159
ray:
num_cpus: 64
num_gpus: 4
enable_object_spilling: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

num_gpus: 4 vs actual GPU usage — worth verifying

domain_classification_xenna and embedding_generation_xenna (line 220-223) now both reserve 4 GPUs on the Ray cluster. Neither entry passes a --gpus or similar argument to their scripts, so it isn't immediately obvious how many GPUs the workloads actually consume at runtime.

If these benchmarks only use 1 GPU internally (similar to the audio_fleurs case), 3 GPUs per run are reserved but idle for the entire benchmark duration, potentially blocking other concurrent jobs. It may be worth confirming the per-script GPU allocation before landing this so the reservation matches actual utilisation.

This same concern applies to the embedding_generation_xenna entry at line 220.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it anyway uses 4 gpus we are just now being explicit it.

@praateekmahajan
praateekmahajan requested a review from ayushdg March 11, 2026 00:12
io_percentage = round(
(task_metrics["jsonl_reader"] + task_metrics["parquet_writer"]) * 100 / sum(task_metrics.values()), 2
)
reader_key = f"{input_filetype}_reader"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

requirements:
# Observed throughput of 2900 docs/sec so we allow a 5% buffer to account for variability
- metric: throughput_docs_per_sec
min_value: 2755

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nice thing about throughput requirements is that we don't need to update with numbers GPUs.

@@ -44,6 +43,7 @@ def run_audio_fleurs_benchmark( # noqa: PLR0913
split: str,
wer_threshold: float,
gpus: int,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to the PR but this arg is confusing IMO. The Cli claims its the number of GPUs to use but actually its the resource requirement for the inference stage

@praateekmahajan
praateekmahajan merged commit 0ba6869 into NVIDIA-NeMo:main Mar 11, 2026
52 checks passed
@sarahyurick sarahyurick mentioned this pull request Mar 11, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants