-
Notifications
You must be signed in to change notification settings - Fork 320
Add ray to each benchmarking entry and make sure no benchmarking/script has RayClient #1593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -153,6 +153,10 @@ entries: | |||||||||||||||||
| - number_of_domains_predicted | ||||||||||||||||||
| ping_on_failure: | ||||||||||||||||||
| - U022P8CDX40 # Sarah Yurick | ||||||||||||||||||
| ray: | ||||||||||||||||||
| num_cpus: 64 | ||||||||||||||||||
| num_gpus: 4 | ||||||||||||||||||
| enable_object_spilling: false | ||||||||||||||||||
| requirements: | ||||||||||||||||||
| # Observed throughput of 2900 docs/sec so we allow a 5% buffer to account for variability | ||||||||||||||||||
| - metric: throughput_docs_per_sec | ||||||||||||||||||
|
|
@@ -213,6 +217,10 @@ entries: | |||||||||||||||||
| ping_on_failure: | ||||||||||||||||||
| - U022P8CDX40 # Sarah Yurick | ||||||||||||||||||
| - U07JL5K0L10 # Praateek Mahajan | ||||||||||||||||||
| ray: | ||||||||||||||||||
| num_cpus: 64 | ||||||||||||||||||
| num_gpus: 4 | ||||||||||||||||||
| enable_object_spilling: false | ||||||||||||||||||
| requirements: | ||||||||||||||||||
| # Observed throughput of 8600 docs/sec so we allow a 5% buffer to account for variability | ||||||||||||||||||
| - metric: throughput_docs_per_sec | ||||||||||||||||||
|
|
@@ -409,7 +417,7 @@ entries: | |||||||||||||||||
| # ensure the total number of documents processed is correct | ||||||||||||||||||
| - metric: num_documents_processed | ||||||||||||||||||
| exact_value: 2119489 | ||||||||||||||||||
| # account for stochastic filters | ||||||||||||||||||
| # account for stochastic filters | ||||||||||||||||||
| - metric: num_kept_documents | ||||||||||||||||||
| min_value: 2090470 | ||||||||||||||||||
| max_value: 2090490 | ||||||||||||||||||
|
|
@@ -442,7 +450,7 @@ entries: | |||||||||||||||||
| # ensure the total number of documents processed is correct | ||||||||||||||||||
| - metric: num_documents_processed | ||||||||||||||||||
| exact_value: 2119489 | ||||||||||||||||||
| # account for stochastic filters | ||||||||||||||||||
| # account for stochastic filters | ||||||||||||||||||
| - metric: num_kept_documents | ||||||||||||||||||
| min_value: 2090470 | ||||||||||||||||||
| max_value: 2090490 | ||||||||||||||||||
|
|
@@ -583,6 +591,10 @@ entries: | |||||||||||||||||
| --split=dev | ||||||||||||||||||
| --wer-threshold=5.5 | ||||||||||||||||||
| --gpus=1 | ||||||||||||||||||
| ray: | ||||||||||||||||||
| num_cpus: 64 | ||||||||||||||||||
| num_gpus: 4 | ||||||||||||||||||
| enable_object_spilling: false | ||||||||||||||||||
|
Comment on lines
+594
to
+597
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GPU over-provisioning in The Every other GPU-using entry in this file (e.g.
Suggested change
|
||||||||||||||||||
| sink_data: | ||||||||||||||||||
| - name: slack | ||||||||||||||||||
| ping_on_failure: | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,9 +23,8 @@ | |
| from typing import Any | ||
|
|
||
| from loguru import logger | ||
| from utils import write_benchmark_results | ||
| from utils import setup_executor, write_benchmark_results | ||
|
|
||
| from nemo_curator.backends.xenna import XennaExecutor | ||
| from nemo_curator.pipeline import Pipeline | ||
| from nemo_curator.stages.audio.common import GetAudioDurationStage, PreserveByValueStage | ||
| from nemo_curator.stages.audio.datasets.fleurs.create_initial_manifest import CreateInitialManifestFleursStage | ||
|
|
@@ -44,6 +43,7 @@ def run_audio_fleurs_benchmark( # noqa: PLR0913 | |
| split: str, | ||
| wer_threshold: float, | ||
| gpus: int, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| executor: str = "xenna", | ||
| **kwargs, # noqa: ARG001 | ||
| ) -> dict[str, Any]: | ||
| """Run the audio fleurs benchmark and collect comprehensive metrics.""" | ||
|
|
@@ -65,7 +65,7 @@ def run_audio_fleurs_benchmark( # noqa: PLR0913 | |
| logger.info(f"WER threshold: {wer_threshold}") | ||
| logger.info(f"GPUs: {gpus}") | ||
|
|
||
| executor = XennaExecutor() | ||
| executor_obj = setup_executor(executor) | ||
| pipeline = Pipeline(name="audio_inference", description="Inference audio and filter by WER threshold.") | ||
|
|
||
| # Add stages | ||
|
|
@@ -106,7 +106,7 @@ def run_audio_fleurs_benchmark( # noqa: PLR0913 | |
| ) | ||
| ) | ||
|
|
||
| results = pipeline.run(executor) | ||
| results = pipeline.run(executor_obj) | ||
|
|
||
| logger.success("Benchmark completed successfully") | ||
|
|
||
|
|
@@ -126,6 +126,7 @@ def main() -> int: | |
| parser.add_argument("--lang", default="hy_am", help="Language code") | ||
| parser.add_argument("--split", default="dev", help="Dataset split to use") | ||
| parser.add_argument("--wer-threshold", type=float, default=5.5, help="WER threshold for filtering") | ||
| parser.add_argument("--executor", default="xenna", choices=["xenna", "ray_data"], help="Executor to use") | ||
| parser.add_argument("--gpus", type=int, default=1, help="Number of GPUs to use") | ||
|
|
||
| args = parser.parse_args() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,9 +124,11 @@ def run_removal_benchmark( # noqa: PLR0913 | |
| for k, v in TaskPerfUtils.aggregate_task_metrics(workflow_run_result).items() | ||
| if k.endswith("_process_time_mean") | ||
| } | ||
| io_percentage = round( | ||
| (task_metrics["jsonl_reader"] + task_metrics["parquet_writer"]) * 100 / sum(task_metrics.values()), 2 | ||
| ) | ||
| reader_key = f"{input_filetype}_reader" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
| writer_key = f"{output_filetype}_writer" | ||
| io_time = task_metrics.get(reader_key, 0) + task_metrics.get(writer_key, 0) | ||
| total_time = sum(task_metrics.values()) | ||
| io_percentage = round(io_time * 100 / total_time, 2) if total_time > 0 else 0 | ||
|
|
||
| return { | ||
| "metrics": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,13 @@ def main() -> int: | |
| logger.info("=== Exact Duplicate Identification Benchmark Starting ===") | ||
| logger.info(f"Arguments: {vars(args)}") | ||
|
|
||
| results = { | ||
| "params": vars(args), | ||
| "metrics": { | ||
| "is_success": False, | ||
| }, | ||
| "tasks": [], | ||
| } | ||
| try: | ||
| results = run_exact_duplicate_identification_benchmark( | ||
| input_path=args.input_path, | ||
|
|
@@ -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) | ||
|
Comment on lines
183
to
198
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled
Previously, the outer 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 [],
} |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,6 @@ | |
| from loguru import logger | ||
| from utils import setup_executor, write_benchmark_results | ||
|
|
||
| from nemo_curator.core.client import RayClient | ||
| from nemo_curator.pipeline import Pipeline | ||
| from nemo_curator.stages.file_partitioning import FilePartitioningStage | ||
| from nemo_curator.stages.image.embedders.clip_embedder import ImageEmbeddingStage | ||
|
|
@@ -172,9 +171,6 @@ def run_image_pipeline_benchmark(args: argparse.Namespace) -> dict[str, Any]: | |
|
|
||
| def main() -> int: | ||
| """Main entry point for image pipeline benchmark.""" | ||
| ray_client = RayClient() | ||
| ray_client.start() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this a noop if cluster is started already?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| parser = argparse.ArgumentParser( | ||
| description="Image curation pipeline benchmark with embedding generation and quality scoring" | ||
| ) | ||
|
|
@@ -255,23 +251,17 @@ def main() -> int: | |
| logger.info("=== Image Pipeline Benchmark Starting ===") | ||
| logger.info(f"Arguments: {vars(args)}") | ||
|
|
||
| results = { | ||
| "params": vars(args), | ||
| "metrics": { | ||
| "is_success": False, | ||
| }, | ||
| "tasks": [], | ||
| } | ||
| try: | ||
| results = run_image_pipeline_benchmark(args) | ||
|
|
||
| except Exception as e: | ||
| error_traceback = traceback.format_exc() | ||
| print(f"Benchmark failed: {e}") | ||
| logger.debug(f"Full traceback:\n{error_traceback}") | ||
| results = { | ||
| "params": vars(args), | ||
| "metrics": { | ||
| "is_success": False, | ||
| }, | ||
| "tasks": [], | ||
| } | ||
| finally: | ||
| write_benchmark_results(results, args.benchmark_results_path) | ||
| ray_client.stop() | ||
|
|
||
| # Return proper exit code based on success | ||
| return 0 if results["metrics"]["is_success"] else 1 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num_gpus: 4vs actual GPU usage — worth verifyingdomain_classification_xennaandembedding_generation_xenna(line 220-223) now both reserve 4 GPUs on the Ray cluster. Neither entry passes a--gpusor 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_fleurscase), 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_xennaentry at line 220.There was a problem hiding this comment.
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.