Super v3 math tool calling slurm test

[gwarmstrong]

Summary by CodeRabbit

• Tests
  • Added automated test suite for validating tool-calling functionality on nano_30B model using mathematical benchmarks (AIME24 and AIME25).
  • Performs comprehensive validation of tool usage metrics, execution timeouts, and mathematical problem-solving accuracy.
  • Integrates cluster-based testing infrastructure for streamlined validation.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces two new SLURM-based test scripts for evaluating MCP tool-calling functionality with a Nano 30B model on clusters. The orchestration script prepares data and launches evaluation jobs for aime24/aime25 benchmarks, while the validation script checks tool usage frequency, timeout counts, and math metric correctness against defined ranges.

Changes

Cohort / File(s)

Summary

MCP Tool-Calling Evaluation Tests tests/slurm-tests/nano_30b_tool_calling/run_test.py, tests/slurm-tests/nano_30b_tool_calling/check_results.py

Introduces test orchestration and validation scripts. run_test.py defines MODEL and SERVER_ARGS constants, implements eval_math_tool_calling() to schedule evaluation jobs on SLURM, and main() to prepare data and schedule result-checking. check_results.py implements three validators: check_tool_usage() counting tool calls from JSONL output, check_timeouts() detecting timeout indicators via regex, and check_math_tool_calling() validating symbolic_correct percentages against metric ranges. Uses soft assertions to aggregate validation failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title references 'Super v3' and 'math tool calling slurm test', which partially aligns with the changeset that introduces SLURM-based test scripts for math tool calling, but 'Super v3' is vague and doesn't clearly convey what the changes accomplish.	Consider a more descriptive title that clearly explains the primary change, such as 'Add SLURM test suite for nano-30B math tool calling evaluation' or similar.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch georgea/super-tool-calling-slurm-test

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can generate a title for your PR based on the changes with custom instructions.

Set the reviews.auto_title_instructions setting to generate a title for your PR based on the changes in the PR with custom instructions.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

tests/slurm-tests/super_120b_tool_calling/check_results.py (3)

136-146: Consider using Path consistently for path operations.

Line 139 uses os.path.join while the other check functions use Path (lines 73, 110). Using Path consistently would improve readability.

Suggested change

 def check_math_tool_calling(eval_dir: str):
     """Check accuracy metrics for math benchmarks with tool calling."""
     for benchmark in MATH_BENCHMARKS:
-        f = os.path.join(eval_dir, "eval-results", benchmark, "metrics.json")
+        f = Path(eval_dir) / "eval-results" / benchmark / "metrics.json"
         data = load_json(f)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/super_120b_tool_calling/check_results.py` around lines 136
- 146, The function check_math_tool_calling builds file paths with os.path.join;
change it to use pathlib.Path consistently (e.g., create p = Path(eval_dir) and
compute p / "eval-results" / benchmark / "metrics.json") to match other checks,
then pass either the Path or str(p) into load_json as used elsewhere; update the
reference in load_json call and anywhere the file path is used so
MATH_BENCHMARKS, eval_dir, check_math_tool_calling, and soft_assert remain
unchanged.

---

67-97: Consider using direct access for num_tool_calls since this test exercises the tool-calling path.

Line 84 uses .get("num_tool_calls", 0). Since this test specifically uses the tool-calling wrapper (per run_test.py), the num_tool_calls field should always be present in output. Using direct access row["num_tool_calls"] would surface unexpected issues where the field is missing.

Suggested change

-                    if row.get("num_tool_calls", 0) > 0:
+                    if row["num_tool_calls"] > 0:

As per coding guidelines: "Don't use .get() for accessing dictionary keys if the code expects them to be present; use direct access data[key_name] to fail with a clear error instead of silently corrupting data."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/super_120b_tool_calling/check_results.py` around lines 67 -
97, In check_tool_usage, replace the permissive lookup row.get("num_tool_calls",
0) with direct indexing row["num_tool_calls"] so missing fields raise an error
and surface unexpected output shape; update any local logic that relied on the
default (e.g., the if that checks > 0 remains the same) and let the test fail
with a KeyError if the tool-calling wrapper omitted the field.

---

148-163: Consider updating type hints to str | Path for consistency.

The function signatures declare eval_dir: str, but main() passes Path objects (e.g., eval_root / "math_tool_calling"). The code works because the functions either wrap with Path() or use os.path.join(), but the type hints don't reflect this.

Suggested change for function signatures

+from typing import Union
+
-def check_tool_usage(eval_dir: str):
+def check_tool_usage(eval_dir: Union[str, Path]):

-def check_timeouts(eval_dir: str):
+def check_timeouts(eval_dir: Union[str, Path]):

-def check_math_tool_calling(eval_dir: str):
+def check_math_tool_calling(eval_dir: Union[str, Path]):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/super_120b_tool_calling/check_results.py` around lines 148
- 163, The type hints for the functions that accept evaluation directories
(check_tool_usage, check_timeouts, check_math_tool_calling) are misleading
because main() passes Path objects; update the function signatures from
eval_dir: str to eval_dir: str | Path (or Path | str) and import typing as
needed so these functions accept both str and Path, and adjust any internal
Path() wrapping if present to avoid double-wrapping.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/slurm-tests/super_120b_tool_calling/run_test.py`:
- Around line 44-63: The eval(...) call in eval_math_tool_calling passes
wandb_project and wandb_name but log_samples defaults to False so those params
are ignored; update the eval call in eval_math_tool_calling to enable logging by
adding log_samples=True (or alternatively remove wandb_project and wandb_name if
sample logging is not desired) so wandb_project/wandb_name are actually used;
look for the eval invocation inside eval_math_tool_calling and modify its
arguments (eval, ctx=wrap_arguments(COMMON_PARAMS), model=MODEL, etc.) to
include log_samples=True or drop the wandb_* args.

---

Nitpick comments:
In `@tests/slurm-tests/super_120b_tool_calling/check_results.py`:
- Around line 136-146: The function check_math_tool_calling builds file paths
with os.path.join; change it to use pathlib.Path consistently (e.g., create p =
Path(eval_dir) and compute p / "eval-results" / benchmark / "metrics.json") to
match other checks, then pass either the Path or str(p) into load_json as used
elsewhere; update the reference in load_json call and anywhere the file path is
used so MATH_BENCHMARKS, eval_dir, check_math_tool_calling, and soft_assert
remain unchanged.
- Around line 67-97: In check_tool_usage, replace the permissive lookup
row.get("num_tool_calls", 0) with direct indexing row["num_tool_calls"] so
missing fields raise an error and surface unexpected output shape; update any
local logic that relied on the default (e.g., the if that checks > 0 remains the
same) and let the test fail with a KeyError if the tool-calling wrapper omitted
the field.
- Around line 148-163: The type hints for the functions that accept evaluation
directories (check_tool_usage, check_timeouts, check_math_tool_calling) are
misleading because main() passes Path objects; update the function signatures
from eval_dir: str to eval_dir: str | Path (or Path | str) and import typing as
needed so these functions accept both str and Path, and adjust any internal
Path() wrapping if present to avoid double-wrapping.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4ab0c42c-bed4-4778-845c-29d85d0e73af

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0a84d and 86301b1.

📒 Files selected for processing (2)

• tests/slurm-tests/super_120b_tool_calling/check_results.py
• tests/slurm-tests/super_120b_tool_calling/run_test.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if other slurm tests pass wandb parameters with or without log_samples
rg -n "wandb_project|wandb_name|log_samples" tests/slurm-tests/ --type py -C 2

Repository: NVIDIA-NeMo/Skills

Length of output: 19619

---

🏁 Script executed:

fd -t f "*.py" | xargs rg -l "def eval\(" | head -5

Repository: NVIDIA-NeMo/Skills

Length of output: 431

---

🏁 Script executed:

rg -n "def eval\(" nemo_skills/pipeline/eval.py -A 30

Repository: NVIDIA-NeMo/Skills

Length of output: 1879

---

🏁 Script executed:

rg -n "def eval\(" nemo_skills/pipeline/eval.py -A 80 | head -100

Repository: NVIDIA-NeMo/Skills

Length of output: 5050

---

🏁 Script executed:

rg -n "wandb_project|wandb_name|log_samples" nemo_skills/pipeline/eval.py -B 2 -A 2

Repository: NVIDIA-NeMo/Skills

Length of output: 1914

---

Add log_samples=True or remove unused wandb parameters.

The eval() function only uses wandb_project and wandb_name when log_samples=True. Since log_samples defaults to False, these parameters are ignored. Either enable logging with log_samples=True, or remove the unused parameters.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/super_120b_tool_calling/run_test.py` around lines 44 - 63,
The eval(...) call in eval_math_tool_calling passes wandb_project and wandb_name
but log_samples defaults to False so those params are ignored; update the eval
call in eval_math_tool_calling to enable logging by adding log_samples=True (or
alternatively remove wandb_project and wandb_name if sample logging is not
desired) so wandb_project/wandb_name are actually used; look for the eval
invocation inside eval_math_tool_calling and modify its arguments (eval,
ctx=wrap_arguments(COMMON_PARAMS), model=MODEL, etc.) to include
log_samples=True or drop the wandb_* args.

[gwarmstrong]

the other slurm tests have this same pattern

[coderabbitai]

@gwarmstrong, understood — thanks for the clarification! I'll note this pattern for future reviews.

(∪ ･ ω ･)∪

---

🧠 Learnings used

Learnt from: CR
Repo: NVIDIA-NeMo/Skills PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-02-26T02:00:21.772Z
Learning: When enabling new modality or adding complicated evaluation/metrics logic in benchmarks, consider adding the dataset into slurm tests for comprehensive evaluation

[coderabbitai]

🧹 Nitpick comments (2)

tests/slurm-tests/nano_30b_tool_calling/check_results.py (1)

140-149: Minor path handling inconsistency.

This function uses os.path.join while check_tool_usage and check_timeouts use Path(). Not a functional issue, but could be unified for consistency.

♻️ Suggestion to use Path() consistently

 def check_math_tool_calling(eval_dir: str):
     """Check accuracy metrics for math benchmarks with tool calling."""
     for benchmark in MATH_BENCHMARKS:
-        f = os.path.join(eval_dir, "eval-results", benchmark, "metrics.json")
+        f = Path(eval_dir) / "eval-results" / benchmark / "metrics.json"
         data = load_json(f)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/nano_30b_tool_calling/check_results.py` around lines 140 -
149, Unify path handling in check_math_tool_calling by replacing os.path.join
usage with pathlib.Path like the other helpers; build the metrics path with
Path(eval_dir) / "eval-results" / benchmark / "metrics.json" (and call load_json
with either the Path or str(path) depending on load_json's expected type) so
check_math_tool_calling uses the same Path-based approach as check_tool_usage
and check_timeouts.

tests/slurm-tests/nano_30b_tool_calling/run_test.py (1)

27-29: Acknowledge: eval import shadows Python builtin.

The static analysis tool flagged that importing eval from nemo_skills.pipeline.cli shadows the Python builtin. This appears intentional since eval is the CLI command name, and the builtin eval() is not used in this script. Consider using an alias if this causes confusion in the future.

♻️ Optional: Use alias to avoid shadowing

-from nemo_skills.pipeline.cli import eval, prepare_data, run_cmd, wrap_arguments
+from nemo_skills.pipeline.cli import eval as run_eval, prepare_data, run_cmd, wrap_arguments

Then update the call site accordingly. However, this is optional if the pattern is consistent with other test scripts in this directory.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/slurm-tests/nano_30b_tool_calling/run_test.py` around lines 27 - 29,
The import of eval from nemo_skills.pipeline.cli shadows the Python builtin;
change the import to alias the CLI command (e.g., import eval as cli_eval) and
update all call sites in this file that currently call eval(...) to use the new
alias (cli_eval(...)); specifically modify the import line that references eval
and any subsequent uses of eval in run_test.py (other imported symbols:
prepare_data, run_cmd, wrap_arguments remain unchanged).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/slurm-tests/nano_30b_tool_calling/check_results.py`:
- Around line 140-149: Unify path handling in check_math_tool_calling by
replacing os.path.join usage with pathlib.Path like the other helpers; build the
metrics path with Path(eval_dir) / "eval-results" / benchmark / "metrics.json"
(and call load_json with either the Path or str(path) depending on load_json's
expected type) so check_math_tool_calling uses the same Path-based approach as
check_tool_usage and check_timeouts.

In `@tests/slurm-tests/nano_30b_tool_calling/run_test.py`:
- Around line 27-29: The import of eval from nemo_skills.pipeline.cli shadows
the Python builtin; change the import to alias the CLI command (e.g., import
eval as cli_eval) and update all call sites in this file that currently call
eval(...) to use the new alias (cli_eval(...)); specifically modify the import
line that references eval and any subsequent uses of eval in run_test.py (other
imported symbols: prepare_data, run_cmd, wrap_arguments remain unchanged).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 99a2634a-ba1c-4fc9-a1ca-1393646b9362

📥 Commits

Reviewing files that changed from the base of the PR and between 86301b1 and be08678.

📒 Files selected for processing (2)

• tests/slurm-tests/nano_30b_tool_calling/check_results.py
• tests/slurm-tests/nano_30b_tool_calling/run_test.py