Skip to content

Add Math pipeline - #1058

Merged
thomasdhc merged 54 commits into
NVIDIA-NeMo:mainfrom
ronjer30:math-pipeline
Mar 3, 2026
Merged

Add Math pipeline#1058
thomasdhc merged 54 commits into
NVIDIA-NeMo:mainfrom
ronjer30:math-pipeline

Conversation

@ronjer30

@ronjer30 ronjer30 commented Sep 9, 2025

Copy link
Copy Markdown
Contributor

Description

Implements extraction and classification of mathematical content as the starting point for a math pipeline.

Usage

See examples/math/README.md

Checklist

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

@copy-pr-bot

copy-pr-bot Bot commented Sep 9, 2025

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@sublimationAC

Copy link
Copy Markdown

Thank you for your work and submission. I'd like to ask if this is the code for the paper "NEMOTRON-CC-MATH: A 133 BILLION-TOKENSCALE HIGH QUALITY MATH PRETRAINING DATASET"? How do you plan to merge this into the updated master branch?
I look forward to your response.

Comment thread nemo_curator/stages/math/classifiers/finemath.py
Comment thread nemo_curator/stages/math/classifiers/finemath.py
Comment thread nemo_curator/stages/math/classifiers/finemath.py Outdated
Comment thread nemo_curator/stages/math/download/html_extractors/lynx.py
Comment thread nemo_curator/stages/math/download/html_extractors/lynx.py Outdated
Comment thread nemo_curator/stages/math/download/html_extractors/lynx.py Outdated
@sublimationAC

Copy link
Copy Markdown

dditional validation before any workflow

Dear ronjer30,

I attempted to run your PR using the latest nemo-curator:25.07 image on only one node. However, I keep encountering this error:

"Task was killed due to the node running low on memory. Memory on the node (IP: *****, ID: *****) where the task (task ID: xxxxxx name=StageWorker.init, pid=263827, memory used=0.08GB) was running was 205.81GB / 216.21GB (0.951878), which exceeds the memory usage threshold of 0.95. "

To address this issue, I reduced the data size to no more than 100 words, yet the error still occurs. Do you have any suggestions on how to resolve this? Thank you!

Signed-off-by: RanjitR <ranjitr@nvidia.com>
Signed-off-by: RanjitR <ranjitr@nvidia.com>
- Enhanced LynxExtractor with error handling
- Improved FineMathClassifier with center cropping and optimized defaults
- Added comprehensive test coverage for all changes

Signed-off-by: RanjitR <ranjitr@nvidia.com>

@greptile-apps greptile-apps Bot left a comment

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.

45 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@staticmethod
def _mid_slice(s: str, n: int) -> str:
m = len(s) // 2
b, e = max(0, m - n), min(m + n, len(s) - 1)

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.

logic: off-by-one error truncates last character when text doesn't need cropping

Suggested change
b, e = max(0, m - n), min(m + n, len(s) - 1)
b, e = max(0, m - n), min(m + n, len(s))

@ronjer30

ronjer30 commented Nov 4, 2025

Copy link
Copy Markdown
Contributor Author

dditional validation before any workflow

Dear ronjer30,

I attempted to run your PR using the latest nemo-curator:25.07 image on only one node. However, I keep encountering this error:

"Task was killed due to the node running low on memory. Memory on the node (IP: *****, ID: *****) where the task (task ID: xxxxxx name=StageWorker.init, pid=263827, memory used=0.08GB) was running was 205.81GB / 216.21GB (0.951878), which exceeds the memory usage threshold of 0.95. "

To address this issue, I reduced the data size to no more than 100 words, yet the error still occurs. Do you have any suggestions on how to resolve this? Thank you!

Please try with nvcr.io/nvidia/nemo-curator:25.09 and follow steps in examples/math/README.md to run either script.

@greptile-apps

greptile-apps Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a complete math data pipeline for NeMo Curator, covering HuggingFace dataset download (0_download.py), CC index lookup, text preprocessing (WARC reading + Lynx-based HTML extraction), LLM-based cleanup (LLMCleanupStage with vLLM), and FineMath quality classification. The implementation is substantial and generally well-structured, with good test coverage across all new stages.

Key changes:

  • New VLLMModel wrapper with proper cache_dirdownload_dir pass-through and safe output handling
  • New CommonCrawlWARCReader for direct HTTPS range-request fetching from CC (no AWS credentials needed)
  • MathContentExtractor combining python-magic, Lynx, and Jupyter notebook extraction
  • TokenSplitterStage (paragraph-aware token chunking) + LLMCleanupStage + ChunkMergeStage pipeline trio
  • FineMathClassifier composite stage wrapping HuggingFaceTB/finemath-classifier

Issues found:

  • TokenSplitterStage lacks a cache_dir parameter, so model weights are always downloaded to the HuggingFace default cache even when users pass --cache_dir to the pipeline — inconsistent with LLMCleanupStage
  • ChunkMergeStage's default separator="\n" combined with chunks that already end with "\n\n" (from TokenSplitterStage) produces triple newlines (\n\n\n) between merged chunks instead of consistent paragraph breaks
  • For Qwen3 models (non-Qwen3.5+), three disable-thinking mechanisms are applied at once: user_prompt suffix " /no_think", system_content = " /no_think" (leaving the system role with no actual instruction), and enable_thinking=False in apply_chat_template — the first two are redundant when the third is used

Confidence Score: 3/5

  • Safe to merge with minor issues — no critical runtime failures, but separator inconsistency and missing cache_dir in TokenSplitterStage should be addressed before widespread use
  • The PR is a large, well-tested feature addition with good structure. Previous thread items have been addressed (cache_dir now passed to vLLM, outputs[0] guard, logger import, /no_think token). Remaining issues are design inconsistencies (missing cache_dir in TokenSplitterStage, triple-newline separator mismatch) and a redundant Qwen3 no-think mechanism — none cause hard crashes but can affect data quality and usability in custom environments.
  • nemo_curator/stages/math/modifiers/chunking.py (missing cache_dir), nemo_curator/stages/math/modifiers/merge_chunks.py (separator mismatch), nemo_curator/stages/math/modifiers/llm_cleanup.py (Qwen3 /no_think redundancy)

Important Files Changed

Filename Overview
nemo_curator/stages/math/modifiers/llm_cleanup.py LLM cleanup stage using vLLM. Qwen3 handling applies /no_think to both user prompt and system content while also using enable_thinking=False, causing redundant/conflicting no-think signals and an empty system instruction for Qwen3 models.
nemo_curator/stages/math/modifiers/chunking.py Token-based text splitter stage. Missing cache_dir parameter means model is always downloaded to the HuggingFace default cache, inconsistent with LLMCleanupStage which properly exposes and uses cache_dir.
nemo_curator/stages/math/modifiers/merge_chunks.py Chunk merge stage. Default separator="\n" joined with chunks that already end with "\n\n" (from TokenSplitterStage) creates triple newlines (\n\n\n) between merged chunks, breaking consistent paragraph spacing.
nemo_curator/models/vllm_model.py New vLLM model wrapper with clean cache_dir pass-through to download_dir, safe outputs[0] handling, and per-model-family sampling param customization. Looks solid.
nemo_curator/stages/math/download/extract.py New math content extractor combining magic-based MIME detection, Lynx for HTML, and notebook handling. Lazy initialization pattern is correct; thread safety via double-checked locking looks fine.
nemo_curator/stages/math/classifiers/finemath.py FineMath classifier with center-crop preprocessing. The _mid_slice fix (using min(m + n, len(s))) is correctly applied. Composite stage decomposition looks correct.
nemo_curator/stages/text/download/common_crawl/download.py New CommonCrawlWARCReader added for direct HTTPS range-request fetching from CC. Session pooling and parallel fetch via ThreadPoolExecutor are well-structured; logger import is properly present.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[0_download.py\nHuggingFace Parquet download] --> B[1_cc_index_lookup.py\nCommon Crawl index lookup]
    B --> C[2_text_preprocess.py\nCommonCrawlWARCReader + MathExtractStage]
    C --> D[3_llm_cleanup.py\nTokenSplitterStage → LLMCleanupStage → ChunkMergeStage]
    D --> E[4_quality_classifier.py\nFineMathClassifier]
    E --> F[5_deduplication.py\nFuzzyDeduplication]

    subgraph MathExtractStage
        C1[MathContentExtractor]
        C1 --> C2{magic MIME detect}
        C2 -->|HTML| C3[LynxExtractor]
        C2 -->|Notebook| C4[_notebook_to_text]
        C2 -->|Text| C5[raw text]
    end

    subgraph LLM Cleanup Pipeline
        D1[TokenSplitterStage\nparagraph-aware chunking] --> D2[LLMCleanupStage\nvLLM inference]
        D2 --> D3[ChunkMergeStage\ngroupby + concat]
    end

    subgraph FineMathClassifier
        E1[CenterCropTextStage] --> E2[TokenizerStage]
        E2 --> E3[FineMathModelStage\nDeBERTa sequence classifier]
    end
Loading

Last reviewed commit: 3171358

@greptile-apps greptile-apps Bot left a comment

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.

28 files reviewed, 12 comments

Edit Code Review Agent Settings | Greptile

Comment thread tutorials/math/3_quality_classifier.py Outdated


def build_pipeline(input_glob: str, output_dir: str) -> Pipeline:
p = Pipeline(name="math_quality_classifier", description="...")

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.

style: placeholder description should be replaced with meaningful text describing the pipeline's purpose

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!

Comment on lines +200 to +202
if row["chunk_id"] < len(result.data) - 1:
# Non-last chunks should end with separator
assert row["text"].endswith("\n") or len(result.data) == 1

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.

logic: condition logic is incorrect - when there's only one chunk, it should not end with separator regardless of chunk_id

Comment thread examples/math/README.md Outdated
--input DATA_DIR \
--output OUTPUT_DIR \
--model microsoft/phi-4 \
--prompt HTML_TO_TEXT_PROMPT \

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.

style: HTML_TO_TEXT_PROMPT appears to be undefined - should this be a literal string or reference to a constant? Should HTML_TO_TEXT_PROMPT be replaced with an actual prompt string or is this a placeholder that users should replace?

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!

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.

This is intentional. HTML_TO_TEXT_PROMPT is the name of a constant that is used to fetch the prompt from prompts.py

Comment thread examples/math/README.md Outdated
--input_filetype parquet
```

This will chunk the data and clean each chunk, creating output in `OUTPUT_CHUNK_DIR/cleanup_*/` with:

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.

syntax: OUTPUT_CHUNK_DIR is undefined but should likely be OUTPUT_DIR based on the command above

Suggested change
This will chunk the data and clean each chunk, creating output in `OUTPUT_CHUNK_DIR/cleanup_*/` with:
This will chunk the data and clean each chunk, creating output in `OUTPUT_DIR/cleanup_*/` with:

user_prompt = self.system_prompt.format(text=text)

if is_qwen3_30b_a3b:
user_prompt = user_prompt + " /nothink"

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.

logic: typo in special token - should likely be "/nothinking" not "/nothink". Is "/nothink" the correct special token for Qwen3-30B-A3B, or should this be "/nothinking"?

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.

From the Huggingface entry, the correct token for this is /no_think. Will make the change

Comment thread nemo_curator/utils/prompts.py Outdated
"""

mind_two_students = """Convert the context below as a multi-turn discussions between two students who are working on their assignment related to the given context. \
Make sure that their discussions strictly adhere to the context below and remains faithful to infomration in the context. \

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.

syntax: typo: 'infomration' should be 'information'

Suggested change
Make sure that their discussions strictly adhere to the context below and remains faithful to infomration in the context. \
Make sure that their discussions strictly adhere to the context below and remains faithful to information in the context. \

Comment thread nemo_curator/utils/prompts.py Outdated
"""

mind_interview = """Conduct an interview-style conversation where one participant acts as the interviewer, asking questions exclusively related to the content provided, while the other participant serves as the subject matter expert, providing detailed responses based on the content. \
Make sure that their discussions strictly adhere to the context below and remains faithful to infomration in the context. \

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.

syntax: typo: 'infomration' should be 'information'

Suggested change
Make sure that their discussions strictly adhere to the context below and remains faithful to infomration in the context. \
Make sure that their discussions strictly adhere to the context below and remains faithful to information in the context. \

Comment thread nemo_curator/utils/prompts.py Outdated
mind_problem_solving = """Convert the context below as a multi-turn problem-solving conversation where participants
analyze challenges or scenarios presented in the content and brainstorm solutions within the context of the provided
material, avoiding speculation or unrelated discussions. Make sure that their conversation strictly adhere to the
context below and remains faithful to infomration in the context. Please DONOT add any new information/reference other

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.

syntax: typo: 'infomration' should be 'information'

Suggested change
context below and remains faithful to infomration in the context. Please DONOT add any new information/reference other
context below and remains faithful to information in the context. Please DONOT add any new information/reference other

Comment thread nemo_curator/utils/prompts.py Outdated
mind_layman_knowall = """Imagine you are presenting the content below step-by-step to a layman. While you are presenting,
the layman has a lot of followup questions regarding your presentation. You answer the questions step-by-step with chain-of-thoughts.
Design this interaction between you and the layman as a multi-turn conversational manner. \
Make sure that the interaction strictly adhere to the context below and remains faithful to infomration in the context. \

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.

syntax: typo: 'infomration' should be 'information'

Suggested change
Make sure that the interaction strictly adhere to the context below and remains faithful to infomration in the context. \
Make sure that the interaction strictly adhere to the context below and remains faithful to information in the context. \

Comment thread nemo_curator/utils/prompts.py Outdated
mind_debate = """Convert the context below as a multi-turn debate-style conversation where the participants present arguments
and counterarguments based solely on the content provided, without introducing external information or personal opinions. Each
participant defends others arguments step-by-step with chain-of-thoughts. \
Make sure that the conversation strictly adhere to the context below and remains faithful to infomration in the context. \

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.

syntax: typo: 'infomration' should be 'information'

Suggested change
Make sure that the conversation strictly adhere to the context below and remains faithful to infomration in the context. \
Make sure that the conversation strictly adhere to the context below and remains faithful to information in the context. \

@sarahyurick sarahyurick left a comment

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.

Very nice, left a couple of high level comments for now. I am excited to test this out myself.

Comment thread examples/math/README.md Outdated
Comment thread examples/math/README.md Outdated
```

## Prerequisites
- GPU(s) with CUDA for the HF model

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.

Suggested change
- GPU(s) with CUDA for the HF model
- GPU(s) with CUDA for the Hugging Face model

Comment thread nemo_curator/stages/math/download/__init__.py
Comment on lines +94 to +96
def inputs(self) -> tuple[list[str], list[str]]:

return ["data"], [self.text_field, self.n_tokens_field]

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.

Suggested change
def inputs(self) -> tuple[list[str], list[str]]:
return ["data"], [self.text_field, self.n_tokens_field]
def inputs(self) -> tuple[list[str], list[str]]:
return ["data"], [self.text_field, self.n_tokens_field]

Comment thread tests/stages/math_stages/__init__.py
Comment on lines +85 to +89
p.add_stage(
Modify(modifier_fn=fill_null_text, input_fields="text", output_fields="text").with_(
resources=Resources(cpus=0.5)
)
)

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.

I am curious where some of these resource assignments are coming from. I belive Modify is probably better with cpus=0.5, did you set this because that's what you were observing?

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.

The resource assignment for this and other tasks was initially set to 0.5 CPUs to accommodate local development environment constraints. I've now increased it to 1 CPU across the examples, pending further performance testing. I believe it its useful to keep this setting explicit in the example so users can easily adjust the CPU allocation to match their specific infrastructure needs, thoughts?

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.

Got it, that's ok with me.

model_name=model,
text_field="text",
max_length_tokens=chunk_length,
).with_(resources=Resources(cpus=1))

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.

The default is 1 CPU, so with_ is not needed here.

min_p=min_p,
max_tokens=max_tokens,
cache_dir=cache_dir,
).with_(resources=Resources(cpus=1, gpus=1))

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.

It looks like this is the default already, so with_ can be removed.

Comment thread examples/math/run_quality_classifier.py Outdated
Comment on lines +45 to +46
"finemath_classifier_tokenizer": {"resources": Resources(cpus=0.5)},
"finemath_classifier_model": {"resources": Resources(cpus=1, gpus=1)},

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.

I think these are already the defaults, or should be overridden in their class definitions instead of the user needing to set them here.

Comment thread nemo_curator/stages/math/modifiers/llm_cleanup.py Outdated

@greptile-apps greptile-apps Bot left a comment

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.

Additional Comments (1)

  1. nemo_curator/stages/text/download/common_crawl/download.py, line 52 (link)

    logic: inconsistent dependency checking - uses self._check_s5cmd_installed() but helper function is _check_s5cmd_installed(). Should this be calling the global helper function?

30 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment thread examples/math/run_all_preprocess.py Outdated
Comment on lines +142 to +144
logger.error(f"Failed to process {name}: {e}")
# We might want to continue to next dataset or stop here
# raise e

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.

style: silently continues on subprocess failures which could mask critical issues - consider adding option to fail fast

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!

Comment thread nemo_curator/models/vllm_model.py Outdated
sampling_params=self._sampling_params,
use_tqdm=False,
)
return [out.outputs[0].text for out in outputs]

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.

logic: assumes outputs[0] exists without checking - could fail if vLLM returns empty outputs array

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.

Added a check to see if outputs are empty before indexing

@vikalluru
vikalluru force-pushed the math-pipeline branch 2 times, most recently from f23aaea to e3e7856 Compare November 20, 2025 18:47

@greptile-apps greptile-apps Bot left a comment

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.

30 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment thread tutorials/math/run_all_preprocess.py Outdated
Comment on lines +27 to +88
"input": "/lustre/fsw/portfolios/llmservice/users/rkarimimahab/data/finemath/original-data/finemath-4plus/*.parquet",
"output": "finemath_4_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"FINEMATH_3": {
"input": "/lustre/fsw/portfolios/llmservice/users/rkarimimahab/data/finemath/original-data/finemath-3plus/*.parquet",
"output": "finemath_3_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"OWM": {
"input": "hf://open-web-math/open-web-math",
# "input": "/home/sasatheesh/data/20t/jsonls/nv-math/open-web-math/*.parquet", # Local path alternative
"output": "owm_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"INFIWEBMATH_4": {
"input": "/lustre/fsw/portfolios/llmservice/users/rkarimimahab/data/finemath/original-data/infiwebmath-4plus/*.parquet",
"output": "infiwebmath_4_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"INFIWEBMATH_3": {
"input": "/lustre/fsw/portfolios/llmservice/users/rkarimimahab/data/finemath/original-data/infiwebmath-3plus/*.parquet",
"output": "infiwebmath_3_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"MEGAMATH_PRO": {
"input": "/lustre/fsw/portfolios/llmservice/projects/llmservice_fm_text/adlr-stem/megamath_dataset/megamath-web-pro/*.parquet",
"output": "megamath_pro_processed",
"fetch_cc": True,
"columns": {
"warc_filename": "warc_filename",
"offset": "warc_record_offset",
"length": "warc_record_length",
},
},
"MEGAMATH_WEB": {
"input": "/lustre/fsw/portfolios/llmservice/projects/llmservice_fm_text/adlr-stem/megamath_dataset/megamath-web/**/*.parquet",

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.

style: hardcoded NVIDIA internal paths make this non-portable for external users - consider adding configuration file or environment variable support

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!

Comment thread tutorials/math/5_llm_cleanup.py Outdated
parser.add_argument("--input", required=True, help="Input directory or glob pattern for JSONL/Parquet files")
parser.add_argument("--output", required=True, help="Output directory for cleaned JSONL files")
parser.add_argument("--model", required=True, help="Model identifier (e.g., microsoft/phi-4)")
parser.add_argument("--prompt", required=True, help="Prompt name from prompts module (e.g., HTML_TO_TEXT_PROMPT)")

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.

style: example references undefined HTML_TO_TEXT_PROMPT - should provide actual prompt name or indicate it's a placeholder

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!

return result.stdout
else:
# Only log warning on failure
logger.warning(f"Failed to fetch WARC record {filename} at {offset}: {result.stderr.decode('utf-8')}")

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.

logic: logger is not imported but used here, will cause NameError at runtime

Suggested change
logger.warning(f"Failed to fetch WARC record {filename} at {offset}: {result.stderr.decode('utf-8')}")
from loguru import logger

(add this import at the top of the file)

self.top_k = top_k
self.min_p = min_p
self.max_tokens = max_tokens
self.cache_dir = cache_dir

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.

style: cache_dir parameter is stored but never passed to LLM initialization - consider using it or removing the parameter

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!

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.

+1 do we want to add it to setup/setup_on_node?

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.

Yes, absolutely! The cache_dir parameter should be passed to the vLLM LLM initialization in the setup method. Here's the fix:

Suggested change
self.cache_dir = cache_dir
llm_kwargs: dict[str, Any] = {
"model": self.model,
"enforce_eager": False,
"trust_remote_code": True,
}
if final_max_model_len is not None:
llm_kwargs["max_model_len"] = final_max_model_len
if self.cache_dir is not None:
llm_kwargs["download_dir"] = self.cache_dir
if model_spec:
llm_kwargs["tensor_parallel_size"] = model_spec.tensor_parallel_size
llm_kwargs["max_num_batched_tokens"] = model_spec.max_num_batched_tokens
self._llm = LLM(**llm_kwargs)

The vLLM LLM constructor uses download_dir parameter to specify where model weights should be cached/downloaded, which corresponds to the cache_dir parameter in your wrapper.

@greptile-apps greptile-apps Bot left a comment

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.

30 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

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.

Additional Comments (1)

  1. nemo_curator/stages/text/download/common_crawl/download.py, line 52 (link)

    logic: _check_s5cmd_installed() should be the global function, not a method

30 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

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.

30 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@greptile-apps greptile-apps Bot left a comment

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.

32 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread tutorials/math/README.md Outdated
--prompt HTML_TO_TEXT_PROMPT \
--chunk_data \
--chunk_length 5000 \
--input_filetype parquet

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.

syntax: --input_filetype parquet conflicts with line 128 which states input is JSONL files from Step 3

Suggested change
--input_filetype parquet
--input_filetype jsonl

Comment thread tutorials/math/README.md Outdated
--prompt HTML_TO_TEXT_PROMPT_CODE \
--chunk_data \
--chunk_length 5000 \
--input_filetype parquet

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.

syntax: --input_filetype parquet specified but the flow suggests JSONL files should be used at this step

Suggested change
--input_filetype parquet
--input_filetype jsonl

Comment thread tests/stages/math_stages/__init__.py

@greptile-apps greptile-apps Bot left a comment

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.

33 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Comment thread nemo_curator/models/vllm_model.py Outdated
Comment on lines +42 to +67
_MODELS: dict[str, ModelSpec] = {
"microsoft/phi-4": ModelSpec("microsoft/phi-4", 16_384, 1, 8_192, 128),
"google/gemma-3-12b-it": ModelSpec("google/gemma-3-12b-it", 131_072, 1, 8_192, 128),
"google/gemma-3-27b-it": ModelSpec("google/gemma-3-27b-it", 131_072, 2, 4096, 64),
"google/gemma-3-27b-it-32k": ModelSpec("google/gemma-3-27b-it", 32_768, 2, 4096, 64),
"Qwen/Qwen2-7B-Instruct": ModelSpec("Qwen/Qwen2-7B-Instruct", 131_072, 1, 8_192, 128),
"Qwen/Qwen2-72B-Instruct": ModelSpec("Qwen/Qwen2-72B-Instruct", 131_072, 4, 4096, 64),
"mistralai/Mistral-7B-Instruct-v0.3": ModelSpec(
"mistralai/Mistral-7B-Instruct-v0.3", 32768, 1, 8_192, 128
),
"deepseek-ai/DeepSeek-V3": ModelSpec("deepseek-ai/DeepSeek-V3", 163840, 8, 4096, 64),
"Qwen/Qwen2.5-32B-Instruct": ModelSpec("Qwen/Qwen2.5-32B-Instruct", 32_768, 8, 4096, 64),
"Qwen/Qwen2.5-72B-Instruct": ModelSpec("Qwen/Qwen2.5-72B-Instruct", 131_072, 8, 4096, 64),
"Qwen/Qwen3-30B-A3B": ModelSpec("Qwen/Qwen3-30B-A3B", 32_768, 2, 8_192, 64),
"Qwen/Qwen3-30B-A3B-Instruct-2507": ModelSpec(
"Qwen/Qwen3-30B-A3B-Instruct-2507", 262144, 4, 4096, 32
),
"openai/gpt-oss-20b": ModelSpec("openai/gpt-oss-20b", 131_072, 1, 8_192, 128),
"openai/gpt-oss-120b": ModelSpec("openai/gpt-oss-120b", 131_072, 4, 8_192, 64),
"Qwen/Qwen3-Coder-30B-A3B-Instruct": ModelSpec(
"Qwen/Qwen3-Coder-30B-A3B-Instruct", 262144, 4, 8_192, 256
),
"nvidia/NVIDIA-Nemotron-Nano-9B-v2": ModelSpec(
"nvidia/NVIDIA-Nemotron-Nano-9B-v2", 131_072, 1, 8_192, 128
),
}

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.

Should we populate these using AutoConfig, etc. instead of hardcoding?

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.

I agree. I've removed these in the latest commit.

Some points to note:

  • The ModelSpec consisted of model_name, max_model_len, tensor_parallel_size, max_num_batched_tokens and batch_size, in that order
  • Only the max_model_len can be fetched from AutoConfig.
  • for tensor_parallel_size, I've added a function that gets the GPU count and sets tp_size as the lowest power of 2 less than or equal to gpu_count.
  • I've changed max_num_batched_tokens to a kwarg and set to a default of 4096, lower of 4096 and 8192.
  • batch_size was an unused param so I removed it

self.top_k = top_k
self.min_p = min_p
self.max_tokens = max_tokens
self.cache_dir = cache_dir

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.

+1 do we want to add it to setup/setup_on_node?

except (TypeError, AttributeError, ValueError):
return False

def _get_text_mime_types(self) -> set[str]:

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.

We could consider moving this function (and the functions below it) to a separate utility file.

CC_BASE_URL = "https://data.commoncrawl.org/"


def _check_s5cmd_installed() -> bool:

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.

I see this function is originally defined for DocumentDownloader and used for Common Crawl and ArXiv. Can you move it to a shared script (maybe https://github.com/NVIDIA-NeMo/Curator/blob/main/nemo_curator/stages/text/download/utils.py)?

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.

Bumping this comment.

length = int(row[self.warc_record_length_col])

# Build the URL
url = f"{CC_BASE_URL}{filename}"

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.

Perhaps urllib would be a safer way to do this.

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.

Bumping this comment.

Comment on lines +215 to +233
def test_inputs_outputs_cleanup_mode(self):
"""Test inputs and outputs methods in cleanup mode."""
stage = LLMCleanupStage(model="test-model", system_prompt="Clean: {text}")

inputs = stage.inputs()
outputs = stage.outputs()

assert inputs == (["data"], ["text", "n_tokens"])
assert outputs == (["data"], ["cleaned_text"])

def test_inputs_outputs_classification_mode(self):
"""Test inputs and outputs methods in classification mode."""
stage = LLMCleanupStage(model="test-model", system_prompt="Classify: {text}", classification=True)

inputs = stage.inputs()
outputs = stage.outputs()

assert inputs == (["data"], ["text", "n_tokens"])
assert outputs == (["data"], ["label"])

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.

We can remove these.

Comment on lines +235 to +266
def test_setup_initializes_llm(self):
"""Test that setup method initializes LLM and sampling params."""
stage = LLMCleanupStage(model="test-model", system_prompt="Clean: {text}")

stage.setup()

assert stage._model._llm is not None
assert stage._model._sampling_params is not None
assert isinstance(stage._model._llm, MockLLM)
assert isinstance(stage._model._sampling_params, MockSamplingParams)

def test_setup_qwen3_model(self):
"""Test setup method with Qwen3 model (special sampling params)."""
stage = LLMCleanupStage(model="Qwen/Qwen3-30B-A3B", system_prompt="Clean: {text}", top_k=10, min_p=0.1)

stage.setup()

assert stage._model._sampling_params is not None
# Qwen3 models should have top_k and min_p set
assert hasattr(stage._model._sampling_params, "top_p")
assert hasattr(stage._model._sampling_params, "top_k")
assert hasattr(stage._model._sampling_params, "min_p")

def test_setup_non_qwen3_model(self):
"""Test setup method with non-Qwen3 model."""
stage = LLMCleanupStage(model="microsoft/phi-4", system_prompt="Clean: {text}")

stage.setup()

assert stage._model._sampling_params is not None
# Non-Qwen3 models should only have top_p
assert hasattr(stage._model._sampling_params, "top_p")

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.

We can remove these.

Comment on lines +268 to +295
def test_setup_with_cache_dir(self):
"""Test setup method with cache_dir specified."""
stage = LLMCleanupStage(model="test-model", system_prompt="Clean: {text}", cache_dir="/test/cache")

stage.setup()

assert stage._model._llm is not None
assert stage._model.cache_dir == "/test/cache"

def test_setup_with_max_tokens(self):
"""Test setup method with max_tokens specified."""
stage = LLMCleanupStage(model="test-model", system_prompt="Clean: {text}", max_tokens=500)

stage.setup()

assert stage._model._sampling_params is not None
assert stage._model._sampling_params.max_tokens == 500

def test_setup_max_tokens_fallback(self):
"""Test setup method falls back to max_model_len when max_tokens is None."""
stage = LLMCleanupStage(
model="test-model", system_prompt="Clean: {text}", max_model_len=16000, max_tokens=None
)

stage.setup()

assert stage._model._sampling_params is not None
assert stage._model._sampling_params.max_tokens == 16000

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.

We can remove these.

Comment thread tutorials/math/run_all_preprocess.py Outdated
# =========================================================================
"FINEMATH_4PLUS": {
# HuggingFace: hf://HuggingFaceTB/finemath/finemath-4plus
"input": "/lustre/fsw/portfolios/llmservice/users/rkarimimahab/data/finemath/original-data/finemath-4plus/*.parquet",

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.

These inputs aren't accessible for non-Nvidians though, right?

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.

These will need to be paths to the datasets downloaded locally. @vikalluru - Think these have moved to datasets.json but still applies

Comment thread tutorials/math/README.md Outdated
Extract and preprocess text from raw web data:

```bash
python tutorials/math/run_text_preprocess.py \

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.

What do you think about renaming the scripts to 1_run_text_preprocess.py, 2_run_quality_classifier.py, etc. to better represent the order these scripts are run?

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.

Agreed

@greptile-apps greptile-apps Bot left a comment

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.

34 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
@sarahyurick

Copy link
Copy Markdown
Contributor

/ok to test dab732d

@greptile-apps greptile-apps Bot left a comment

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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@sarahyurick sarahyurick left a comment

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.

It looks like there are some merge conflicts that need to be resolved.

As a follow-up, we will need to add benchmarks under https://github.com/NVIDIA-NeMo/Curator/tree/main/benchmarking too.

Comment thread nemo_curator/models/vllm_model.py Outdated
Comment thread nemo_curator/models/vllm_model.py Outdated
Comment thread nemo_curator/stages/math/modifiers/chunking.py Outdated
Comment thread nemo_curator/stages/math/modifiers/llm_cleanup.py Outdated
Comment thread nemo_curator/stages/math/modifiers/llm_cleanup.py Outdated
Comment thread tutorials/math/1_cc_index_lookup.py Outdated
Comment thread tutorials/math/2_text_preprocess.py Outdated
Comment thread tutorials/math/3_quality_classifier.py Outdated
Comment thread tutorials/math/4_deduplication.py Outdated
Comment thread tutorials/math/5_llm_cleanup.py Outdated
  After LLM cleanup, documents are split across multiple rows (one per
  chunk). This adds a merge stage that deduplicates, filters invalid
  chunks (NO USEFUL CONTENT, empty, null), sorts by chunk_id, and
  concatenates text back into one row per document. Token count columns
  are summed; metadata columns use first().

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
ChunkMergeStage (Step 6) — post-LLM chunk reassembly. After LLM
cleanup splits documents into multiple rows per chunk, this stage merges
them back into one row per document by deduplicating, filtering invalid
chunks, sorting by chunk_id, and concatenating text. Token counts are
summed; metadata columns preserved via first().

New files:
 - nemo_curator/stages/math/modifiers/merge_chunks.py
 - tests/stages/math_stages/modifiers/test_merge_chunks.py (11 tests)
 - tutorials/math/6_postprocess.py

Signed-off-by: Sukrit Rao <sukritr@ndia.com>
  - Update copyright headers from 2025 to 2026 across 18 math pipeline
  - Fix off-by-one bug in CenterCropTextStage._mid_slice where len(s) - 1
    truncated the last character; updated tests to expect correct behavior

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
  - Merge 5_llm_cleanup.py + 6_postprocess.py into 3_llm_cleanup.py with
    ChunkMergeStage as an inline pipeline stage
  - Renumber: quality classifier → step 4, deduplication → step 5
  - Delete old scripts (3-6) replaced by new numbering
  - Update README: diagram, summary table, step sections, script references

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
- Fix dedup check: use FuzzyDuplicateIds (conditional) not fuzzy_id_generator.json (always written)
- Add MathExtractStage dataclass to extract.py; update tutorials + tests to use it
- Fix stale import in 3_llm_cleanup.py (modifiers path moved in upstream merge)
- Add try/finally around ray_client.stop() in 2_text_preprocess.py and 4_quality_classifier.py

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>

@sarahyurick sarahyurick left a comment

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.

Thank you!

Comment thread nemo_curator/stages/math/modifiers/chunking.py Outdated
Comment thread nemo_curator/stages/math/modifiers/llm_cleanup.py Outdated
Comment thread tests/stages/math_stages/modifiers/test_llm_cleanup.py Outdated
Comment thread tests/stages/text/download/arxiv/test_stage.py Outdated
Comment thread tests/stages/text/download/arxiv/test_stage.py Outdated
Comment thread tests/stages/text/download/arxiv/test_stage.py Outdated
Comment thread tests/stages/text/download/common_crawl/test_download.py Outdated
Comment thread tests/stages/text/download/common_crawl/test_download.py Outdated
Comment thread tests/stages/text/download/common_crawl/test_download.py Outdated
Comment thread tests/stages/text/download/common_crawl/test_download.py Outdated
- Use local_files_only=True and self.model_name in math stage setup
- Align download tests with upstream (DocumentIterateExtractStage, wget retry flags, copyright years)

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
@greptile-apps

greptile-apps Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor
Additional Comments (3)

nemo_curator/stages/math/modifiers/llm_cleanup.py, line 291
Qwen3 thinking-disable logic is inconsistent with VLLMModel

The is_qwen3_30b_a3b check uses an exact string match for "Qwen/Qwen3-30B-A3B", while VLLMModel.setup() uses the broader check "Qwen3" in self.model or "qwen3" in self.model.lower() to enable Qwen3-specific sampling parameters (top_k, min_p).

This inconsistency means that for other Qwen3 thinking models (e.g., Qwen/Qwen3-8B, Qwen/Qwen3-14B, etc.), VLLMModel will correctly apply Qwen3 sampling parameters but LLMCleanupStage will not inject /no_think into the prompt or pass enable_thinking: False to apply_chat_template. The LLM will then produce verbose chain-of-thought outputs that pollute the cleaned text.

Consider aligning the check with the one in VLLMModel:

is_qwen3_thinking = "Qwen3" in self.model_name or "qwen3" in self.model_name.lower()
is_qwen3_30b_a3b = self.model_name == "Qwen/Qwen3-30B-A3B"

# Apply /no_think for all Qwen3 thinking models (or at minimum match VLLMModel's check)

nemo_curator/utils/prompts.py, line 18
Unclosed ** bold markdown in prompts

Item 6 in HTML_TO_TEXT_PROMPT (and item 7 in HTML_TO_TEXT_PROMPT_CODE at approximately line 49) is missing the closing ** for the bold markdown. This could affect how LLMs interpret and re-format the instruction, since the bold span bleeds into the subsequent items.

6) **Do not remove or discard any part of the code. If any code blocks contain errors or formatting issues, make minimal changes to make them runnable, but otherwise leave them exactly as they are.**

The same fix applies to HTML_TO_TEXT_PROMPT_CODE item 7.


nemo_curator/stages/math/download/extract.py, line 185
Empty records produces schema-less DataFrame

When all rows fail extraction, records will be an empty list and pd.DataFrame(records) creates a DataFrame with zero rows and zero columns — no schema at all. Downstream stages that check for specific columns (e.g., "text", "url", "type") on the returned batch will raise a KeyError.

Compare this with ChunkMergeStage, which preserves the schema on empty output via pd.DataFrame(columns=df.columns). Consider using the extractor's declared output columns here:

output_cols = self.extractor.output_columns()
if self.filename_col:
    output_cols = [*output_cols, self.filename_col]

return DocumentBatch(
    task_id=batch.task_id,
    dataset_name=batch.dataset_name,
    data=pd.DataFrame(records) if records else pd.DataFrame(columns=output_cols),
    _metadata=batch._metadata,
    _stage_perf=batch._stage_perf,
)

@sarahyurick

Copy link
Copy Markdown
Contributor

/ok to test 1a5c645

- Move vLLM initialization from setup() to setup_on_node() to prevent
  torch.compile cache race condition with concurrent workers (NVIDIA-NeMo#1514)
- Broaden Qwen3 /no_think check to all Qwen3 models; use
  enable_thinking=False for Qwen3.5+ which dropped the prompt switch
- Preserve DataFrame schema on empty extraction batches
- Close unclosed ** bold markdown in prompts

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
@sarahyurick

Copy link
Copy Markdown
Contributor

/ok to test 66b7192

@greptile-apps

greptile-apps Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor
Additional Comments (3)

nemo_curator/stages/text/download/common_crawl/download.py, line 1571
Thread safety race condition in _get_session()

_get_session() is called from _read_warc_record(), which is executed concurrently by multiple threads in _read_warc_records_batch(). Multiple threads can simultaneously observe self._session is None as True and each proceed to create their own requests.Session and mount adapters, resulting in redundant sessions and wasted resources. Although individual requests.Session instances are thread-safe for making requests, the lazy-initialization pattern here is not.

Move session creation to a setup() method so it happens once per stage lifecycle, or use a lock to guard the initialization:

import threading

def __init__(self, ...):
    ...
    self._session = None
    self._session_lock = threading.Lock()

def _get_session(self) -> requests.Session:
    if self._session is None:
        with self._session_lock:
            if self._session is None:
                session = requests.Session()
                adapter = requests.adapters.HTTPAdapter(
                    pool_connections=self.max_workers,
                    pool_maxsize=self.max_workers * 2,
                    max_retries=self.max_retries,
                )
                session.mount("https://", adapter)
                session.mount("http://", adapter)
                self._session = session
    return self._session

nemo_curator/stages/math/modifiers/llm_cleanup.py, line 1154
inputs() declares n_tokens_field as required but process() treats it as optional

inputs() declares [self.text_field, self.n_tokens_field] as required input columns. However, process() gracefully handles the case where n_tokens_field is absent with if self.n_tokens_field in df.columns:. If the pipeline framework validates input columns against inputs() before calling process(), batches without n_tokens_field will fail at the validation step — even though the implementation can handle them.

If n_tokens_field is truly optional (e.g. when chunking is not used), remove it from the required inputs declaration:

    def inputs(self) -> tuple[list[str], list[str]]:
        return ["data"], [self.text_field]

nemo_curator/utils/gpu_utils.py, line 1789
get_max_model_len_from_config ignores cache_dir

AutoConfig.from_pretrained uses the default HuggingFace cache regardless of whether a custom cache_dir was specified when the model was originally downloaded via snapshot_download. When VLLMModel is initialized with a non-default cache_dir, snapshot_download downloads the model weights to that directory. But when VLLMModel.setup() later calls get_max_model_len_from_config(self.model), AutoConfig looks in the default cache, potentially triggering an unnecessary network round-trip (or failing in an offline/air-gapped environment).

Pass cache_dir as a parameter so the correct location is used:

def get_max_model_len_from_config(model: str, cache_dir: str | None = None) -> int | None:
    config = AutoConfig.from_pretrained(model, trust_remote_code=True, cache_dir=cache_dir)
    ...

And in VLLMModel.setup():

final_max_model_len = get_max_model_len_from_config(self.model, cache_dir=self.cache_dir)

@sarahyurick sarahyurick left a comment

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.

Blocking this PR temporarily, until @raosukrit67 adds additional changes as discussed.

- Fix enable_thinking not reaching Qwen3 chat template (was nested
  in dict instead of passed as top-level kwarg)
- Add @Property to VLLMModel.model_id_names to match base class
- Add --text-field arg to quality classifier for post-cleanup use
- Add output_filetype to dedup workflow to preserve JSONL format
- Error handling in gpu_utils: raise on zero GPUs, try/except for
  AutoConfig, pass cache_dir through
- Always define _model_kwargs in LLMCleanupStage to prevent
  AttributeError after serialization
- Make chunk split/merge guards consistent in LLM cleanup tutorial
- Make lynx tests run without lynx by mocking shutil.which
- Thread-safe lazy init in MathContentExtractor
- Update tests for np.ndarray returns and removed autocast param

Signed-off-by: Sukrit Rao <sukritr@nvidia.com>
@sarahyurick

Copy link
Copy Markdown
Contributor

/ok to test 3171358

@greptile-apps

greptile-apps Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor
Additional Comments (3)

nemo_curator/stages/math/modifiers/chunking.py, line 56
Missing cache_dir parameter inconsistent with LLMCleanupStage

TokenSplitterStage lacks a cache_dir parameter. Both setup_on_node (which downloads via snapshot_download) and setup (which loads with local_files_only=True) always target the default HuggingFace cache, ignoring any user-specified cache location.

In tutorials/math/3_llm_cleanup.py, users typically pass --cache_dir to the script, which flows to LLMCleanupStage but silently bypasses TokenSplitterStage. In environments where the default ~/.cache/huggingface/hub is restricted or on a different filesystem than the desired cache, this will cause TokenSplitterStage to either download to the wrong location or fail to load the tokenizer.

Consider adding cache_dir: str | None = None and threading it through snapshot_download and AutoTokenizer.from_pretrained similarly to how LLMCleanupStage handles it:

def __init__(self, ..., cache_dir: str | None = None):
    ...
    self.cache_dir = cache_dir

def setup_on_node(self, ...) -> None:
    snapshot_download(repo_id=self.model_name, cache_dir=self.cache_dir, local_files_only=False)

def setup(self, ...) -> None:
    self._tokenizer = AutoTokenizer.from_pretrained(
        self.model_name, cache_dir=self.cache_dir, local_files_only=True
    )

nemo_curator/stages/math/modifiers/merge_chunks.py, line 120
Separator mismatch creates triple newlines in merged output

Chunks produced by TokenSplitterStage (default separator "\n\n") retain the separator on all non-last paragraphs (see chunking.py line 80: para_to_add = para if is_last else para + self.separator). This means each chunk except the very last one ends with "\n\n".

ChunkMergeStage then joins chunks with "\n" (default separator here). For two adjacent chunks the join produces:

chunk1_text\n\n + "\n" + chunk2_text  →  chunk1_text\n\n\nchunk2_text

This results in three consecutive newlines between merged chunks, creating an extra blank line compared to the original document's paragraph spacing ("\n\n"). For training data quality this inconsistency could be undesirable.

Consider either:

  • Defaulting ChunkMergeStage.separator to "" (since chunk boundaries already carry the original separator), or
  • Stripping the trailing separator from each chunk text before joining.

nemo_curator/stages/math/modifiers/llm_cleanup.py, line 167
Redundant /no_think placement and empty system instruction for Qwen3

For Qwen3 (non-Qwen3.5+) models, three separate mechanisms are applied simultaneously to disable chain-of-thought:

  1. user_prompt is suffixed with " /no_think"
  2. system_content is set to " /no_think" (the entire system message is this token — no actual instruction is provided in the system role)
  3. apply_chat_template is called with enable_thinking=False (line 180)

enable_thinking=False in the chat template is the correct, canonical way to suppress thinking in Qwen3. Having /no_think in the user prompt on top of it is redundant, and setting the entire system content to " /no_think" means the model receives no system-level guidance (the actual prompt instruction from self.system_prompt only appears in the user role).

Consider removing the /no_think suffix and the system_content = " /no_think" assignment, and relying solely on enable_thinking=False. If backward-compat with tokenizers that don't support enable_thinking is needed, keep only the user-prompt suffix and leave system_content = "".

@thomasdhc thomasdhc left a comment

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.

Approved

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.

8 participants