Add math modality benchmarking support - #1604
Conversation
| results = pipeline.run(executor, initial_tasks=None) | ||
| success = True | ||
| except Exception as e: | ||
| logger.error(f"Pipeline failed: {e}") | ||
| results = [] | ||
| success = False | ||
|
|
||
| elapsed = time.perf_counter() - start | ||
|
|
||
| num_input_documents = TaskPerfUtils.get_aggregated_stage_stat(results, "extract_", "num_items_processed") | ||
| num_output_documents = TaskPerfUtils.get_aggregated_stage_stat(results, "jsonl_writer", "num_items_processed") | ||
|
|
||
| metrics = { | ||
| "is_success": success, | ||
| "time_taken_s": elapsed, | ||
| "num_output_tasks": len(results) if results else 0, | ||
| "num_input_documents": int(num_input_documents), |
There was a problem hiding this comment.
Four separate full dataset scans — consolidate into one pass
compute_extraction_metrics calls .count() four times on filtered Ray datasets. Each .count() triggers a full scan of all output JSONL files (read → filter → count). For any non-trivial benchmark output this quadruples the I/O cost of the post-processing step and inflates the wall-clock time visible in the results.
A single aggregation pass would be both faster and more accurate:
def compute_extraction_metrics(output_dir: str) -> dict:
metrics = {}
try:
jsonl_files = get_all_file_paths_under(output_dir, keep_extensions=[".jsonl"])
if not jsonl_files:
return metrics
ds = ray.data.read_json(jsonl_files)
def _count_types(batch: dict) -> dict:
types = batch.get("type", [])
texts = batch.get("text", [])
return {
"html": [sum(1 for t in types if t == "html")],
"text": [sum(1 for t in types if t == "text")],
"notebook": [sum(1 for t in types if t == "notebook")],
"html_empty": [
sum(1 for t, tx in zip(types, texts) if t == "html" and not (tx or "").strip())
],
}
agg = ds.map_batches(_count_types, batch_format="numpy").sum(
["html", "text", "notebook", "html_empty"]
)
metrics["type_html_count"] = int(agg["html"])
metrics["type_text_count"] = int(agg["text"])
metrics["type_notebook_count"] = int(agg["notebook"])
metrics["html_empty_text_count"] = int(agg["html_empty"])
except Exception as e:
logger.warning(f"Could not compute extraction metrics: {e}")
return metrics(The exact Ray aggregation API may need adjustment to match the version in use, but the principle of a single pass applies.)
| # Math Curation Dependencies | ||
| math_cpu = [ | ||
| "nemo_curator[text_cpu]", # Math examples use text processing utilities |
There was a problem hiding this comment.
boto3 version pin missing in math_cpu — inconsistent with inference-server
The inference-server extra pins boto3>=1.35, but math_cpu (and transitively math_cuda12) adds boto3 with no version constraint. When only the math_cpu extra is installed, pip/uv may resolve an older boto3 that lacks features or fixes present in 1.35+.
| # Math Curation Dependencies | |
| math_cpu = [ | |
| "nemo_curator[text_cpu]", # Math examples use text processing utilities | |
| "boto3>=1.35", # S3 range requests for Common Crawl WARC fetching |
| ds = ray.data.read_json(jsonl_files) | ||
| total = ds.count() | ||
| metrics["num_chunks_processed"] = total | ||
|
|
||
| text_lengths = [len(row.get("cleaned_text") or "") for row in ds.select_columns(["cleaned_text"]).iter_rows()] | ||
| if text_lengths: | ||
| metrics["avg_output_text_length"] = sum(text_lengths) / len(text_lengths) |
There was a problem hiding this comment.
Redundant full-dataset scan in
compute_llm_cleanup_metrics
ds.count() on line 220 triggers a complete scan of all output JSONL files. Then iter_rows() on line 223 immediately triggers a second full scan of the same dataset. The total variable will equal len(text_lengths) after the list comprehension completes, so the first scan is entirely wasteful.
| ds = ray.data.read_json(jsonl_files) | |
| total = ds.count() | |
| metrics["num_chunks_processed"] = total | |
| text_lengths = [len(row.get("cleaned_text") or "") for row in ds.select_columns(["cleaned_text"]).iter_rows()] | |
| if text_lengths: | |
| metrics["avg_output_text_length"] = sum(text_lengths) / len(text_lengths) | |
| ds = ray.data.read_json(jsonl_files) | |
| text_lengths = [len(row.get("cleaned_text") or "") for row in ds.select_columns(["cleaned_text"]).iter_rows()] | |
| total = len(text_lengths) | |
| metrics["num_chunks_processed"] = total |
| scores = [row["finemath_int_scores"] for row in ds.select_columns(["finemath_int_scores"]).iter_rows()] | ||
| if scores: | ||
| total = len(scores) | ||
| score_sum = sum(scores) | ||
| metrics["mean_finemath_score"] = score_sum / total if total > 0 else 0.0 | ||
|
|
||
| for i in range(6): | ||
| metrics[f"score_distribution_{i}"] = sum(1 for s in scores if s == i) | ||
|
|
||
| metrics["docs_score_ge_3"] = sum(1 for s in scores if s >= MIN_HIGH_QUALITY_SCORE) |
There was a problem hiding this comment.
All classifier scores loaded into memory at once
The list comprehension on line 194 materializes the entire finemath_int_scores column into a Python list before any aggregation is done. For a large benchmark output the scores list could consume significant memory. The subsequent sum(1 for s in scores if s == i) passes also re-iterate the same in-memory list six times.
Using iter_batches with incremental counters avoids holding all scores at once:
score_counts = [0] * 6
score_sum = 0
total = 0
for batch in ds.select_columns(["finemath_int_scores"]).iter_batches(batch_format="numpy"):
for s in batch["finemath_int_scores"]:
score_counts[min(s, 5)] += 1
score_sum += s
total += 1
if total > 0:
metrics["mean_finemath_score"] = score_sum / total
for i in range(6):
metrics[f"score_distribution_{i}"] = score_counts[i]
metrics["docs_score_ge_3"] = sum(score_counts[MIN_HIGH_QUALITY_SCORE:])| def test_deepcopy_extractor_with_lock(self) -> None: | ||
| extractor = MathContentExtractor() | ||
|
|
||
| cloned = copy.deepcopy(extractor) | ||
|
|
||
| assert cloned is not extractor | ||
| assert cloned._lock is not None |
There was a problem hiding this comment.
Deepcopy test doesn't verify lock independence
The test confirms the cloned lock is not None, but the key property of __getstate__/__setstate__ is that the cloned object gets a fresh, independent lock — not the same object shared with the original. Without asserting cloned._lock is not extractor._lock, a naive implementation that simply copies the reference (rather than creating a new lock) would still pass this test.
| def test_deepcopy_extractor_with_lock(self) -> None: | |
| extractor = MathContentExtractor() | |
| cloned = copy.deepcopy(extractor) | |
| assert cloned is not extractor | |
| assert cloned._lock is not None | |
| def test_deepcopy_extractor_with_lock(self) -> None: | |
| extractor = MathContentExtractor() | |
| cloned = copy.deepcopy(extractor) | |
| assert cloned is not extractor | |
| assert cloned._lock is not None | |
| assert cloned._lock is not extractor._lock |
| } | ||
|
|
||
| if num_input_documents > 0 and num_output_documents > 0: | ||
| metrics["extraction_success_rate"] = num_output_documents / num_input_documents |
There was a problem hiding this comment.
extraction_success_rate silently absent on complete extraction failure
The metric is only added when num_output_documents > 0. If all documents fail extraction (a real failure scenario worth benchmarking), num_output_documents is 0, so the condition and num_output_documents > 0 is false and extraction_success_rate is never written to metrics. The YAML benchmark config lists extraction_success_rate as an expected additional_metrics entry, so a missing key may cause silent mis-reporting or downstream errors in the benchmark framework.
The correct rate when output is zero is 0.0 (100% failure rate), not an absent metric. Only the division-by-zero case (num_input_documents == 0) warrants omitting the metric:
| } | |
| if num_input_documents > 0 and num_output_documents > 0: | |
| metrics["extraction_success_rate"] = num_output_documents / num_input_documents | |
| if num_input_documents > 0: | |
| metrics["extraction_success_rate"] = num_output_documents / num_input_documents |
| text_lengths = [len(row.get("cleaned_text") or "") for row in ds.iter_rows()] | ||
| total = len(text_lengths) | ||
| metrics["num_chunks_processed"] = total | ||
| if text_lengths: | ||
| metrics["avg_output_text_length"] = sum(text_lengths) / total | ||
|
|
||
| no_content = sum(1 for length in text_lengths if length == 0) | ||
| metrics["no_useful_content_count"] = no_content |
There was a problem hiding this comment.
iter_rows() materializes one Python dict per row
ds.iter_rows() yields one Python dict per document before the list comprehension extracts only the integer length. For a large LLM-cleanup run with many chunks this is noticeably slower and more memory-intensive than iter_batches. Since the previous double-scan issue (ds.count() + iter_rows()) has been fixed, consider converting the remaining iter_rows() call to an iter_batches loop to stay consistent with how compute_classifier_metrics was already refactored:
total = 0
sum_len = 0
no_content = 0
for batch in ds.iter_batches(batch_format="numpy"):
for text in batch.get("cleaned_text", []):
length = len(text or "")
sum_len += length
if length == 0:
no_content += 1
total += 1
metrics["num_chunks_processed"] = total
if total:
metrics["avg_output_text_length"] = sum_len / total
metrics["no_useful_content_count"] = no_content| _spec = importlib.util.spec_from_file_location("cc_index_lookup", _TUTORIAL_PATH) | ||
| _mod = importlib.util.module_from_spec(_spec) | ||
| _spec.loader.exec_module(_mod) |
There was a problem hiding this comment.
Module-level
spec_from_file_location can return None
importlib.util.spec_from_file_location returns None when the target file is not found (e.g. when _TUTORIAL_PATH does not exist in the deployed benchmarking environment). The very next line passes None directly into module_from_spec, which raises:
AttributeError: 'NoneType' object has no attribute 'submodule_search_locations'
Because this code runs at module level, the entire script fails to import — not just at runtime when the benchmark is executed — producing a deeply confusing traceback that does not mention the missing file. A guard with an early, descriptive error would make the failure actionable:
_spec = importlib.util.spec_from_file_location("cc_index_lookup", _TUTORIAL_PATH)
if _spec is None:
raise FileNotFoundError(
f"Could not load tutorial module from {_TUTORIAL_PATH}. "
"Ensure the tutorials directory is present relative to the benchmarking scripts."
)
_mod = importlib.util.module_from_spec(_spec)
_spec.loader.exec_module(_mod)| for batch in ds.iter_batches(batch_format="numpy"): | ||
| for s in batch["finemath_int_scores"]: | ||
| score_counts[min(s, 5)] += 1 | ||
| score_sum += s | ||
| total += 1 | ||
|
|
||
| if total > 0: | ||
| metrics["mean_finemath_score"] = score_sum / total |
There was a problem hiding this comment.
numpy scalar in metrics dict may cause JSON serialization failure
iter_batches(batch_format="numpy") yields numpy.int64 scalars for each s. The accumulator score_sum starts as a Python int but becomes numpy.int64 after the first score_sum += s. Consequently score_sum / total produces a numpy.float64, and metrics["mean_finemath_score"] is stored as a numpy.float64.
If write_benchmark_results serializes metrics with the standard json.dumps, this fails:
TypeError: Object of type float64 is not JSON serializable
The simplest fix is to cast at the assignment site:
| for batch in ds.iter_batches(batch_format="numpy"): | |
| for s in batch["finemath_int_scores"]: | |
| score_counts[min(s, 5)] += 1 | |
| score_sum += s | |
| total += 1 | |
| if total > 0: | |
| metrics["mean_finemath_score"] = score_sum / total | |
| for batch in ds.iter_batches(batch_format="numpy"): | |
| for s in batch["finemath_int_scores"]: | |
| score_counts[min(int(s), 5)] += 1 | |
| score_sum += int(s) | |
| total += 1 | |
| if total > 0: | |
| metrics["mean_finemath_score"] = score_sum / total |
Similarly, totals[k] += row[k] in compute_extraction_metrics and compute_llm_cleanup_metrics accumulates numpy scalars from iter_rows() into the totals dict, which then flow directly into metrics. Casting each row[k] to int before the += would prevent the same issue in those functions.
| except Exception as e: # noqa: BLE001 | ||
| logger.warning(f"S3 fetch failed for {filename}: {e}") | ||
| return None |
There was a problem hiding this comment.
RuntimeError from missing boto3 silently swallowed
The outer except Exception block catches all exceptions including the RuntimeError raised by _get_s3_client() when boto3 is not installed. Because self._s3_client is never set when the import fails, every call to _read_warc_record_s3 will:
- Enter
_get_s3_client(), acquire the lock, attemptimport boto3, fail, and raiseRuntimeError. - Have that
RuntimeErrorcaught here and downgraded to alogger.warning. - Return
None.
This means the entire output dataset will be silently empty — with thousands of "S3 fetch failed for …: boto3 is not installed" warnings — rather than producing a clear, early failure. The test_use_s3_without_boto3_raises_clear_error test verifies that _get_s3_client() raises correctly, but nothing tests the end-to-end behaviour through _read_warc_record_s3.
The fix is to let non-transient configuration errors propagate out of _read_warc_record_s3:
except RuntimeError:
raise # Re-raise configuration errors (e.g. missing boto3) immediately
except Exception as e: # noqa: BLE001
logger.warning(f"S3 fetch failed for {filename}: {e}")
return None| - name: math_cc_index_lookup | ||
| enabled: true | ||
| script: cc_index_benchmark.py | ||
| args: >- |
There was a problem hiding this comment.
It looks like there is no option for "xenna" vs "ray_data" for this one?
There was a problem hiding this comment.
Thanks, explicitly added "xenna" as the default, not tested on ray_data.
| # We need ray.put() below to broadcast query URLs, which triggers an | ||
| # implicit ray.init() if Ray isn't initialized yet. That bare init | ||
| # would lack the runtime_env that XennaExecutor normally sets (see | ||
| # XennaExecutor.run()), causing Xenna's GPU probe to fail. Mirror | ||
| # what XennaExecutor does: explicit ray.init with the env var BEFORE | ||
| # any other Ray calls. | ||
| os.environ.setdefault("RAY_EXPERIMENTAL_NOSET_CUDA_VISIBLE_DEVICES", "1") | ||
| ray_client = RayClient() | ||
| ray_client.start() | ||
|
|
||
| ray.init( | ||
| ignore_reinit_error=True, | ||
| runtime_env={ | ||
| "env_vars": { | ||
| "RAY_EXPERIMENTAL_NOSET_CUDA_VISIBLE_DEVICES": "1", | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Normally we don't want individual benchmarking scripts to initialize the Ray client on their own (#1593). I am wondering if there is a better way to do this.
There was a problem hiding this comment.
Agreed, removed this,ray.init is needed to set the env variables correctly before ray.put is called
|
|
||
|
|
||
| def _fill_null_text(text: str | None) -> str: | ||
| import pandas as pd |
There was a problem hiding this comment.
It can be a top-level import?
(extract, classifier, LLM cleanup) and a data preparation script. Benchmark changes: - Add math_pipeline_benchmark.py with per-stage TaskPerfUtils metrics consistent with other modalities, CLI flags for stage selection, and --input-blocksize for GPU utilization tuning - Add prepare_math_benchmark_data.py to enrich FineMath-4+ with Common Crawl binary content for offline benchmarking - Add math_preprocess, math_preprocess_classifier, and math_preprocess_llm_cleanup entries to nightly-benchmark.yaml - Install lynx in Dockerfiles for HTML-to-text extraction Stage fixes: - Fix deepcopy/pickle crash in MathContentExtractor by adding __getstate__/__setstate__ to strip unpickleable threading.Lock and magic.Magic before serialization (triggered by ProcessingStage.with_()) - Add S3 transport to CommonCrawlWARCReader via boto3 as an alternative to HTTPS, activated by use_s3=True or CC_USE_S3 env var - Add boto3 to math Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…load refactor - Remove ray_actors from executor choices in math benchmark scripts; - Add thread-safe lazy init (double-checked locking) for requests.Session and boto3 S3 client in CommonCrawlWARCReader, fixing a race condition when ThreadPoolExecutor threads concurrently initialize shared clients - Add __getstate__/__setstate__ to CommonCrawlWARCReader for pickle compatibility with Ray executors (threading.Lock is not picklable) - Fix s3_key_prefix sentinel: use `is not None` check so passing - Inline HF download helpers from tutorials/math/0_download.py into prepare_math_benchmark_data.py, removing dynamic importlib usage. Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…h_pipeline_benchmark.py - Set minimum version for boto3 to 1.35 in pyproject.toml and uv.lock for math-cpu dependencies. - Refactor JSONL file processing in math_pipeline_benchmark.py to improve performance by using batch iteration and column selection, enhancing metrics extraction for document types. Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…mprove performance - Update the computation of mean finemath scores and score distributions to utilize batch processing and direct column selection, enhancing efficiency. - Adjust the calculation of average output text length to reflect total processed chunks accurately. - Ensure metrics extraction is streamlined for both classifier and LLM cleanup stages. Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…h benchmarking pipeline - Add cc_index_benchmark.py: benchmarks CCIndexLookupStage using tutorial components (collect_unique_urls, get_cc_index_files, CCIndexLookupStage) with timing and metrics collection. - Add math_cc_index_lookup entry and cc_index datasets to nightly-benchmark.yaml; add Slack ping_on_failure contacts for all math benchmark entries. - Fix Ray GPU visibility for Xenna: set RAY_EXPERIMENTAL_NOSET_CUDA_VISIBLE_DEVICES=1 before ray.init() in both the benchmark script and tutorial to prevent Xenna's GPU probe from failing when ray.put is used. - Tutorial: switch collect_unique_urls from cuDF to pandas for reading/dedup (avoids potential GPU OOM for large datasets) - Retained lynx in the benchmark docker file Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…tion. Add a defensive tutorial module-load check in CC index benchmarking and cast Ray/NumPy batch values to Python ints during metric aggregation to avoid type-related runtime failures. Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…Xenna executor Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
…output rows metric Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
- Fix import hygiene and error propagation in benchmarking and WARC reader - Move prepare_math_benchmark_data.py to data_prep/ - Replace RayClient with ray.init(address="auto") in cc_index_benchmark - Moved pandas imports to module level - Propagate RuntimeError (missing boto3) through _read_warc_record_s3 and _read_warc_records_batch instead of swallowing it - Add test for boto3 RuntimeError propagation through the threadpool Signed-off-by: Ranjit Rajan <ranjitr@nvidia.com>
Benchmark changes:
math_pipeline_benchmark.pyto run benchmarkprepare_math_benchmark_data.pyto enrich FineMath-4+ with Common Crawl binary content for offline benchmarkingStage fixes:
Description
Usage
# Add snippet demonstrating usageChecklist