Add LLM-as-a-judge to eval suite and bump NDD version - #2324
Add LLM-as-a-judge to eval suite and bump NDD version#2324sarahyurick wants to merge 21 commits into
Conversation
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| files_per_partition: int | None, | ||
| ) -> Pipeline: | ||
| """Build one streaming Curator reader → optional language gate → NDD stages → filters → writer pipeline.""" | ||
| # TODO: Add an optional TokenLengthFilter stage before NDD stages so prompts |
There was a problem hiding this comment.
Big TODO. Right now I just do string truncation in the jinja file. But we should decide the best course of action.
Also, since some extractors might return null while others return content, we should decide whether those examples are evaluated or not? Right now I force the empty string and still give it to the judge.
There was a problem hiding this comment.
Opened a Linear issue, since this PR is already fairly large: https://linear.app/nvidia/issue/NMCUR-402/enhance-llm-judge-eval-to-filtermodify-by-token-count-limits.
| extract_reasoning_content: false | ||
| scores: | ||
| - name: best_extraction | ||
| description: Select the candidate that is most useful as clean document text. |
There was a problem hiding this comment.
I had Codex generate all the Jinja, scoring, etc. prompts. Just a POC for now.
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
ayushdg
left a comment
There was a problem hiding this comment.
Minor comments around dependencies. Thanks a lot for the bump!
| "kaldiio; sys_platform == 'never'", | ||
| "levenshtein; sys_platform == 'never'", | ||
| "numba==0.65.0", # Override RAPIDS/legacy caps for the inference image; vLLM 0.22 requires numba 0.65.0 | ||
| "packaging>=25", # Required by data-designer==0.9.1; overrides Lightning's packaging<25 cap from nemo-toolkit[asr] |
There was a problem hiding this comment.
Let's add a followup ticket to bump asr. I think some of these overrides can be avoided.
There was a problem hiding this comment.
Opened a GitHub issue: NVIDIA-NeMo/Speech#16144 and a Linear issue: https://linear.app/nvidia/issue/NMCUR-404/relax-the-lightning240-constraint-for-nemo-speech-asr.
| "xgrammar>=0.1.32", # Override vllm's ==0.1.29 pin to address CVE GHSA-7rgv-gqhr-fxg3 (DoS via multi-layer nesting) | ||
| "sqlfluff>=4.2.0", # Address CVE-2026-46373/46374 (parser DoS); overrides data-designer-engine==0.5.5 sqlfluff<4 cap | ||
| "pandas>=3.0.0, <= 3.0.4" # Override data-designers constraint of <3 | ||
| "sqlfluff>=4.2.0", # Address CVE-2026-46373/46374 (parser DoS); data-designer-engine==0.9.1 supports sqlfluff>=4.1,<5 |
There was a problem hiding this comment.
We can probably get rid of this and do uv lock --upgrade-package sqlfluff to have uv pick up a newer version rather than needing an override now.
| [[package]] | ||
| name = "cryptography" | ||
| version = "50.0.0" | ||
| version = "49.0.0" |
There was a problem hiding this comment.
Can we also override cryptography. Looks like NDD adds a pin but should work with newer versions. It's just following older conventions of keeping an upper pin: https://github.com/NVIDIA-NeMo/DataDesigner/pull/821/changes.
>=50 is recommended for https://github.com/NVIDIA-NeMo/Curator/security/dependabot/217.
We can also open an issue in NDD to remove the upper pin on cryptography
| [[package]] | ||
| name = "gitpython" | ||
| version = "3.1.58" | ||
| version = "3.1.57" |
There was a problem hiding this comment.
Do we know if a specific package is downgrading this?
There was a problem hiding this comment.
Not sure. Looks like the latest is 3.1.60. I can do uv lock --upgrade-package GitPython.
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
VibhuJawa
left a comment
There was a problem hiding this comment.
Still understanding the workflow. Left an initial review . I wonder if we should do pairwise comparisons as a first pass vs score based judges ?
| } | ||
| ``` | ||
|
|
||
| NDD adds one top-level column for each judge. A judge named `extraction_quality` with a `quality` score produces a result shaped like this: |
There was a problem hiding this comment.
Quick question: Is each judge 1 LLM call or multiple LLM calls ?
There was a problem hiding this comment.
Each judges entry (under execution.stages.name) in the YAML file is 1 LLM call per row (no matter how many scores it has).
cc_extract_example/text_extraction_qwen_judge.yaml: 2judges= 2 calls/rowcc_extract_example/text_extraction_qwen_gemma_judges.yaml: 4judges= 4 calls/row
|
|
||
|
|
||
| def _place_filters(config: dict[str, object], stages: list[dict[str, object]]) -> list[list[dict[str, object]]]: | ||
| """Place top-level filters after the NDD stage that produces their judge column.""" |
There was a problem hiding this comment.
For my understanding: What is the use of place filters produces their judge column
There was a problem hiding this comment.
The idea is to place Filter stages immediately after the judge stage that is producing the relevant column. For example, if judge 1 produces extraction_quality and you filter on it, placing the filter right after stage 1 means rows that fail the filter get dropped before judge 2 ever runs on them, saving that judge's LLM calls on rows you were going to discard anyway.
Although I am not so sure I want to keep all the filtering logic anyway. Like realistically, the user will probably want to inspect the "bad" columns themselves and keep the judging information, rather than just throwing it away. WDYT?
| def _build_language_filter_stage( | ||
| *, | ||
| language: str | None, | ||
| model_path: str | None, | ||
| min_score: float, | ||
| text_field: str, | ||
| ) -> ScoreFilter | None: | ||
| """Build an optional FastText language gate without retaining its score column.""" | ||
| if not language: | ||
| return None | ||
| if not model_path: | ||
| msg = "--fasttext-langid-model-path is required when --language is provided." | ||
| raise ValueError(msg) | ||
| if not 0.0 <= min_score <= 1.0: | ||
| msg = "--min-langid-score must be between 0 and 1." | ||
| raise ValueError(msg) | ||
|
|
||
| # FastText is optional, so import it only for jobs that enable this stage. | ||
| from nemo_curator.stages.text.filters.fasttext import FastTextLangId | ||
|
|
||
| return ScoreFilter( | ||
| filter_obj=FastTextLangId( | ||
| model_path=model_path, | ||
| min_langid_score=min_score, | ||
| lang=language, | ||
| ), | ||
| text_field=text_field, | ||
| verbose=True, | ||
| ).with_(name="fasttext_language_filter") | ||
|
|
There was a problem hiding this comment.
I get the motivation for this but i wonder if we want it as a generic feature here ?
There was a problem hiding this comment.
Can you explain more what your suggestion is? IMO it is nice for doing language-specific testing, although I can understand it isn't applicable to every use case. It is mostly supposed to be a "nice-to-have."
| judge_stages = [] | ||
| for stage, filters_after_stage in zip(configured_stages, stage_filters, strict=True): |
There was a problem hiding this comment.
What is the intuition b/w using 1 stage vs multiple stages , do we expect perf to be better b/w them ?
There was a problem hiding this comment.
Initially I was seeing better perf by using 1 stage, but the advantage of using multiple stages is that it enables resumability between each Data Designer stage.
I can try to do more rigorous benchmarks though. Like if multi stage can get the same perf as single stage, then we should only support multi stage.
There was a problem hiding this comment.
Hmm I ran another comparison benchmark and still found single stage to be faster. So I am thinking for now to just keep both, so that we can still enable resumability with multi stage.
Actually, the single_stage vs multi_stage comparison I was looking at isn't totally valid. single_stage took 67m but only wrote 411 rows; multi_stage took 80m and wrote 1553 rows from the same eligible pool (409 of the 411 overlap). So single_stage "won" by doing ~1/4 the work.
Root cause (from Codex): both judges use the same model, so their calls share one concurrency pool and one 60-second per-request timeout (inference_parameters.timeout defaults to 60s in NDD's factory.py). In single_stage, both judges' tasks (judge1: 3 scored fields with reasoning, much slower; judge2: 1 field, fast) get dispatched concurrently into the same actor's queue since they're independent columns with no dependency between them. Mixing a heavy and light workload in one bounded-concurrency queue inflates tail latency - enough of judge1's calls blew past 60s that the run spiraled into timeouts and never recovered (confirmed: "degraded performance" warnings hit 90–100% and then the run went silent for its last ~6 minutes). multi_stage splits the two judges into separate actor pools, so each queue is homogeneous and never hit this.
So actually, I am thinking I will just support multi_stage.
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Things to consider:
LLMJudgeColumnConfig.multi_modal_context(e.g.,ImageContext)