From d40c4b69b106e02cd97f10665bcf299137f7c05c Mon Sep 17 00:00:00 2001 From: Lawrence Lane Date: Mon, 13 Apr 2026 18:36:22 -0400 Subject: [PATCH] feat: Add agent skills for NeMo Gym code review, debugging, profiling, config, data, and scaffolding Seven spec-compliant agent skills with evals, references, and a deterministic review script. gym-review is the S-tier reference implementation with a standalone Python checker (scripts/review.py), self-contained anti-pattern and fix-pattern references, and portable eval fixtures. Co-Authored-By: Claude Opus 4.6 --- .claude/skills/add-benchmark/SKILL.md | 9 +- .claude/skills/add-benchmark/evals/evals.json | 51 ++ .claude/skills/chains.yaml | 61 +++ .claude/skills/gym-config/SKILL.md | 207 ++++++++ .claude/skills/gym-config/evals/evals.json | 41 ++ .claude/skills/gym-data/SKILL.md | 174 +++++++ .claude/skills/gym-data/evals/evals.json | 42 ++ .claude/skills/gym-debug/SKILL.md | 121 +++++ .claude/skills/gym-debug/evals/evals.json | 41 ++ .claude/skills/gym-profile/SKILL.md | 146 ++++++ .claude/skills/gym-profile/evals/evals.json | 42 ++ .claude/skills/gym-review/SKILL.md | 110 +++++ .claude/skills/gym-review/evals/evals.json | 52 ++ .../evals/files/sample_clean_server.py | 47 ++ .../gym-review/evals/files/sample_config.yaml | 35 ++ .../evals/files/sample_multi_turn_agent.py | 59 +++ .../evals/files/sample_server_with_bugs.py | 47 ++ .../gym-review/references/anti-patterns.md | 163 +++++++ .../gym-review/references/fix-patterns.md | 191 ++++++++ .claude/skills/gym-review/scripts/review.py | 446 ++++++++++++++++++ .claude/skills/gym-scaffold-agent/SKILL.md | 170 +++++++ .../gym-scaffold-agent/evals/evals.json | 46 ++ 22 files changed, 2299 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/add-benchmark/evals/evals.json create mode 100644 .claude/skills/chains.yaml create mode 100644 .claude/skills/gym-config/SKILL.md create mode 100644 .claude/skills/gym-config/evals/evals.json create mode 100644 .claude/skills/gym-data/SKILL.md create mode 100644 .claude/skills/gym-data/evals/evals.json create mode 100644 .claude/skills/gym-debug/SKILL.md create mode 100644 .claude/skills/gym-debug/evals/evals.json create mode 100644 .claude/skills/gym-profile/SKILL.md create mode 100644 .claude/skills/gym-profile/evals/evals.json create mode 100644 .claude/skills/gym-review/SKILL.md create mode 100644 .claude/skills/gym-review/evals/evals.json create mode 100644 .claude/skills/gym-review/evals/files/sample_clean_server.py create mode 100644 .claude/skills/gym-review/evals/files/sample_config.yaml create mode 100644 .claude/skills/gym-review/evals/files/sample_multi_turn_agent.py create mode 100644 .claude/skills/gym-review/evals/files/sample_server_with_bugs.py create mode 100644 .claude/skills/gym-review/references/anti-patterns.md create mode 100644 .claude/skills/gym-review/references/fix-patterns.md create mode 100644 .claude/skills/gym-review/scripts/review.py create mode 100644 .claude/skills/gym-scaffold-agent/SKILL.md create mode 100644 .claude/skills/gym-scaffold-agent/evals/evals.json diff --git a/.claude/skills/add-benchmark/SKILL.md b/.claude/skills/add-benchmark/SKILL.md index 385666e384..80b8bc44f3 100644 --- a/.claude/skills/add-benchmark/SKILL.md +++ b/.claude/skills/add-benchmark/SKILL.md @@ -6,8 +6,13 @@ description: > training environment, or resources server into NeMo-Gym. Also use when wrapping an existing 3rd-party benchmark library. Covers the full workflow: data preparation, resources server implementation, agent wiring, YAML config, testing, and reward - profiling (baselining). Triggered by: "add benchmark", "new resources server", - "integrate benchmark", "wrap benchmark", "add training environment", "add eval". + profiling (baselining). +license: Apache-2.0 +compatibility: Requires Python 3.12+, uv, git. NeMo Gym must be installed. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(python:*) Bash(ng_*) Bash(git:*) Bash(pre-commit:*) Read Write Edit Grep Glob --- # Add Benchmark to NeMo-Gym diff --git a/.claude/skills/add-benchmark/evals/evals.json b/.claude/skills/add-benchmark/evals/evals.json new file mode 100644 index 0000000000..c678502465 --- /dev/null +++ b/.claude/skills/add-benchmark/evals/evals.json @@ -0,0 +1,51 @@ +{ + "skill_name": "add-benchmark", + "evals": [ + { + "id": 1, + "prompt": "Add a new math benchmark to NeMo Gym. The benchmark tests algebra word problems. The verify method should extract the final numerical answer from the model's response and compare it to the expected answer in verifier_metadata.", + "expected_output": "A complete resources server under resources_servers/math_algebra/ with app.py implementing verify(), configs/math_algebra.yaml with proper dataset wiring, data/example.jsonl with 5 entries, tests/test_app.py with >= 95% coverage, and requirements.txt.", + "assertions": [ + "resources_servers/math_algebra/app.py exists and contains a class extending SimpleResourcesServer", + "The verify() method extracts a numerical answer and compares to verifier_metadata", + "Think-block stripping is present before answer extraction", + "data/example.jsonl contains exactly 5 lines of valid JSON", + "Each example line has responses_create_params.input and verifier_metadata", + "configs/math_algebra.yaml defines both resources server and agent instances", + "tests/test_app.py contains tests for verify pass, verify fail, and edge cases", + "requirements.txt contains '-e nemo-gym[dev] @ ../../'", + "The verify method returns reward as 0.0 or 1.0 only" + ] + }, + { + "id": 2, + "prompt": "Wrap the HumanEval benchmark library as an external benchmark in NeMo Gym. The library has its own execution and scoring logic.", + "expected_output": "A custom agent server under responses_api_agents/ that wraps the HumanEval library, with pre/post processing between Gym schema and library format, and a YAML config wiring it together.", + "assertions": [ + "An agent server directory exists under responses_api_agents/", + "The agent's run() endpoint is async", + "Pre-processing converts Gym schema to library input format", + "Post-processing converts library output to BaseVerifyResponse with reward field", + "The agent uses asyncio.Semaphore for concurrency control", + "httpx is not imported anywhere — aiohttp adapter is used if the library needs HTTP", + "YAML config wires the agent to a model server and resources server", + "requirements.txt includes the external library dependency" + ] + }, + { + "id": 3, + "prompt": "Add a code generation benchmark that compiles and runs C++ code. The verify method should compile the model's code, run it against test cases, and compare stdout to expected output.", + "expected_output": "A resources server with subprocess execution via Ray, auto-install for g++ if needed, semaphore-bounded compilation, and proper error handling for compilation failures and runtime errors.", + "assertions": [ + "app.py uses asyncio.Semaphore to bound concurrent subprocess calls", + "Subprocess output is decoded with errors='replace'", + "A setup module with ensure_gpp() or similar auto-install function exists", + "model_post_init calls the auto-install function", + "tests/conftest.py has a pytest_configure hook that calls the auto-install", + "Tests use pytest.mark.skipif for the external tool", + "Compilation and runtime errors return reward 0.0, not exceptions", + "The verify method handles empty or unparseable model output gracefully" + ] + } + ] +} diff --git a/.claude/skills/chains.yaml b/.claude/skills/chains.yaml new file mode 100644 index 0000000000..c7f8294907 --- /dev/null +++ b/.claude/skills/chains.yaml @@ -0,0 +1,61 @@ +chains: + new-benchmark: + name: New Benchmark + description: End-to-end benchmark creation — scaffold, implement, data, config, baseline, review + steps: + - skill: add-benchmark + purpose: Scaffold server, implement verify(), write tests + - skill: gym-data + purpose: Prepare and register datasets + - skill: gym-config + purpose: Validate YAML configuration + - skill: gym-profile + purpose: Baseline against multiple models + - skill: gym-review + purpose: Final review before PR + + validate: + name: Validate Benchmark + description: Check an existing benchmark is correctly configured and producing valid results + steps: + - skill: gym-config + purpose: Verify config is well-formed + - skill: gym-data + purpose: Validate datasets with ng_prepare_data + - skill: gym-profile + purpose: Run rollouts and analyze results + + diagnose: + name: Diagnose Issues + description: Debug a failing benchmark — identify root cause and anti-patterns + steps: + - skill: gym-debug + purpose: Identify the failure point + - skill: gym-review + purpose: Check code for anti-patterns that may cause the failure + + external-integration: + name: External Benchmark Integration + description: Wrap a 3rd-party benchmark library into NeMo Gym + steps: + - skill: gym-scaffold-agent + purpose: Create agent wrapper for external library + - skill: gym-data + purpose: Convert and register datasets + - skill: gym-config + purpose: Wire configuration + - skill: gym-profile + purpose: Compare Gym scores against published numbers + - skill: gym-review + purpose: Check for httpx, concurrency, and propagation issues + + pre-merge: + name: Pre-Merge Check + description: Review and validate before merging a benchmark PR + steps: + - skill: gym-review + purpose: Check for anti-patterns and correctness issues + - skill: gym-config + purpose: Validate configuration + - skill: gym-data + purpose: Validate datasets diff --git a/.claude/skills/gym-config/SKILL.md b/.claude/skills/gym-config/SKILL.md new file mode 100644 index 0000000000..c4fb2debd8 --- /dev/null +++ b/.claude/skills/gym-config/SKILL.md @@ -0,0 +1,207 @@ +--- +name: gym-config +description: > + Compose and validate Hydra YAML configurations for NeMo Gym. Use when setting up + server configs, wiring agent-to-server references, configuring model endpoints, + setting up multi-environment training, or debugging config composition errors. + Covers Hydra/OmegaConf patterns, env.yaml, and ng_dump_config validation. +license: Apache-2.0 +compatibility: Requires Python 3.12+ with NeMo Gym installed. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(ng_*) Read Write Edit Grep Glob +--- + +# NeMo Gym Configuration + +## Config anatomy + +A NeMo Gym config defines server instances as top-level keys, each mapping to a server type + subdirectory: + +```yaml +my_math_server: # Instance name (arbitrary, must be unique) + resources_servers: # Server type directory + math_benchmark: # Server subdirectory name + entrypoint: app.py + domain: math + datasets: + - name: example + type: example + jsonl_fpath: resources_servers/math_benchmark/data/example.jsonl + # ... server-specific config fields +``` + +Agents reference their dependencies by instance name: + +```yaml +my_math_agent: + responses_api_agents: + simple_agent: + entrypoint: app.py + resources_server: + type: resources_servers + name: my_math_server # Must match the instance name above + model_server: + type: responses_api_models + name: policy_model # Must match a model server instance +``` + +## Step 1: Define server instances + +For each component, create a top-level key with: +- A unique instance name +- The server type directory (`resources_servers`, `responses_api_models`, `responses_api_agents`) +- The server subdirectory name +- Server-specific configuration fields + +## Step 2: Wire references + +Verify that every `name` reference in agent configs points to an actual instance: +- `resources_server.name` must match a resources server instance +- `model_server.name` must match a model server instance +- If using multiple agents/servers, each cross-reference must be exact + +## Step 3: Configure model endpoints + +Model endpoint config goes in `env.yaml` at project root: + +```yaml +policy_base_url: http://localhost:8000/v1 +policy_api_key: your-key +policy_model_name: your-model +``` + +For multiple models (e.g. policy + reward model), add separate entries: +```yaml +reward_base_url: http://localhost:8001/v1 +reward_api_key: your-key +reward_model_name: your-reward-model +``` + +## Step 4: Configure datasets + +See the [gym-data](../gym-data/SKILL.md) skill for full dataset preparation. In config: + +```yaml +datasets: +- name: train_dataset + type: train + jsonl_fpath: resources_servers/my_benchmark/data/train.jsonl + gitlab_identifier: + dataset_name: my_benchmark + version: 0.0.1 + artifact_fpath: train.jsonl + license: MIT +- name: example + type: example + jsonl_fpath: resources_servers/my_benchmark/data/example.jsonl +``` + +Rules: +- `train` and `validation` types need both `jsonl_fpath` and `gitlab_identifier` +- `example` type only needs `jsonl_fpath` (committed to git) +- `license` required for `train` and `validation` + +## Step 5: Multi-environment training + +To run multiple environments simultaneously, compose multiple config files: + +```bash +ng_run "+config_paths=[ + resources_servers/math/configs/math.yaml, + resources_servers/code_gen/configs/code_gen.yaml, + responses_api_models/vllm_model/configs/vllm_model.yaml +]" +``` + +Each server gets its own instance name and port. Agents can reference different resources servers. + +## Step 6: Validate + +Always validate the merged config before running: + +```bash +ng_dump_config "+config_paths=[resources_servers/my_benchmark/configs/my_benchmark.yaml,responses_api_models/openai_model/configs/openai_model.yaml]" +``` + +Check: +- All instance names resolve +- No OmegaConf interpolation errors (`${var}` references) +- Dataset paths exist (for example data) or gitlab_identifier is set (for train/validation) +- Port assignments don't conflict +- `verified: false` is present for new servers (pre-commit hook adds this) + +## Server-specific config fields + +Beyond the base fields documented in CLAUDE.md, individual servers define custom config fields. When configuring a server, read its `app.py` Config class to discover these. Common patterns: + +### Concurrency and timeouts +Most servers that run subprocesses or external calls define: +```yaml +num_processes: 8 # asyncio.Semaphore value for parallel execution +max_concurrency: 32 # Alternative name for semaphore bound +unit_test_timeout_secs: 10 # Timeout for subprocess execution +max_execution_time: 10 # Alternative timeout field name +compilation_timeout: 30.0 # Compilation-specific timeout +sql_execution_timeout_s: 30.0 # SQL query timeout +``` +These are NOT inherited from any base class — each server defines its own. Check the server's Config class. + +### LLM-as-Judge configs +Servers using LLM judges (e.g., `equivalence_llm_judge`, `jailbreak_detection`) require a second model server reference: +```yaml +judge_model_server: + type: responses_api_models + name: judge_model # Must match a model server instance +judge_responses_create_params: + input: [] + temperature: 0.0 + max_output_tokens: 1024 +judge_endpoint_max_concurrency: 64 # Rate-limit judge API calls +``` +This means you need TWO model server instances in your config when using judge-based verification. + +### Partial reward configs +Several servers support non-binary rewards for nuanced training signals: +```yaml +# jailbreak_detection +reward_if_safe: 1.0 +reward_if_unsafe: 0.0 +reward_if_unclear: 0.0 +reward_if_quality_high: 1.0 +reward_if_quality_low: 0.3 # Partial credit + +# equivalence_llm_judge +reward_if_swap_fails: 0.0 # Can be -1.0 for penalty +reward_if_full_generation_succeeds: 0.5 # Partial credit on fallback +check_twice_swap: true # Positional bias detection +``` + +### External service connections +Some servers connect to external services: +```yaml +sandbox_host: ${oc.env:SANDBOX_HOST,localhost} # OmegaConf env var injection +sandbox_port: ${oc.env:SANDBOX_PORT,8080} +``` +The `${oc.env:VAR_NAME,default}` pattern injects environment variables at config resolution time. This is the ONE place env vars are acceptable (for infra endpoints that vary per deployment). + +### Agent-specific fields +```yaml +max_steps: 1 # Override default conversation turns +max_correction_turns: 3 # For proof_refinement_agent +include_all_attempts: true # Record all attempts in output +``` + +## Common mistakes + +| Mistake | Fix | +|---------|-----| +| Instance name mismatch between agent and server | Use exact same string in both places | +| Missing `env.yaml` | Create it at project root with model endpoint config | +| YAML indentation in nested `gitlab_identifier` | Use 4-space indent consistently | +| Hydra `+` prefix confusion | `+key=value` adds new keys, `key=value` overrides existing | +| Config path relative vs absolute | Paths in `config_paths` are relative to project root | +| Missing judge model server for judge-based benchmarks | Need TWO model server instances — one for policy, one for judge | +| Using bare env vars instead of `${oc.env:VAR,default}` | OmegaConf interpolation is the approved pattern for deployment-specific values | +| Forgetting `max_steps` in agent config | Defaults vary by agent — set explicitly for multi-turn | diff --git a/.claude/skills/gym-config/evals/evals.json b/.claude/skills/gym-config/evals/evals.json new file mode 100644 index 0000000000..a4482281e0 --- /dev/null +++ b/.claude/skills/gym-config/evals/evals.json @@ -0,0 +1,41 @@ +{ + "skill_name": "gym-config", + "evals": [ + { + "id": 1, + "prompt": "Set up a YAML config for a new math benchmark that uses LLM-as-judge verification. I need a policy model for generation and a separate judge model for grading answers.", + "expected_output": "A config with THREE server instances: resources server, policy model server, AND judge model server. The resources server config should include judge-specific fields like judge_model_server reference, judge_responses_create_params, and judge_endpoint_max_concurrency.", + "assertions": [ + "The config defines two separate model server instances (policy and judge)", + "The resources server has a judge_model_server reference with type and name matching the judge model instance", + "judge_responses_create_params is present with temperature and max_output_tokens", + "judge_endpoint_max_concurrency is set to bound concurrent judge calls", + "The agent references the policy model, not the judge model", + "env.yaml guidance mentions TWO sets of endpoint configs (policy + judge)" + ] + }, + { + "id": 2, + "prompt": "I need to deploy my benchmark on a SLURM cluster where the sandbox host varies per node. How do I inject the sandbox host as an environment variable into the Gym config?", + "expected_output": "Config using OmegaConf env var injection syntax ${oc.env:VAR_NAME,default} for deployment-specific values, with explanation of when this pattern is acceptable vs passing config through YAML.", + "assertions": [ + "The ${oc.env:VAR_NAME,default} syntax is used for the sandbox host", + "A default value is provided in the interpolation", + "The response explains this is the approved pattern for deployment-specific infra values", + "The response distinguishes this from general config (which must go through YAML, not env vars)" + ] + }, + { + "id": 3, + "prompt": "Configure a jailbreak detection benchmark with combined reward. I want safety checking AND quality evaluation, with partial credit for safe-but-low-quality responses.", + "expected_output": "Config with use_combined_reward: true, separate reward values for safety and quality tiers, and explanation of the reward formula (safety_reward * quality_reward).", + "assertions": [ + "use_combined_reward is set to true", + "Separate reward fields exist for safety (reward_if_safe, reward_if_unsafe) and quality (reward_if_quality_high, reward_if_quality_low)", + "The partial credit value for low quality is between 0 and 1 (not 0.0 or 1.0)", + "The response explains the combined reward formula (multiplication)", + "A judge model server is configured for the quality evaluation stage" + ] + } + ] +} diff --git a/.claude/skills/gym-data/SKILL.md b/.claude/skills/gym-data/SKILL.md new file mode 100644 index 0000000000..d62510d971 --- /dev/null +++ b/.claude/skills/gym-data/SKILL.md @@ -0,0 +1,174 @@ +--- +name: gym-data +description: > + Prepare, validate, and register datasets for NeMo Gym benchmarks. Use when converting + source data to Gym JSONL format, generating example.jsonl files, uploading to the + GitLab dataset registry, validating with ng_prepare_data, or wiring gitlab_identifier + into YAML configs. Covers the full data lifecycle from raw source to registered dataset. +license: Apache-2.0 +compatibility: Requires Python 3.12+, uv. GitLab registry operations require MLflow credentials in env.yaml. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(python:*) Bash(ng_*) Read Write Edit Grep Glob +--- + +# NeMo Gym Data Preparation + +## Step 1: Understand the target schema + +Every line in a Gym JSONL file must have this structure: + +```json +{ + "responses_create_params": { + "input": [ + {"role": "system", "content": "System prompt here"}, + {"role": "user", "content": "Problem statement here"} + ] + }, + "verifier_metadata": { + // Task-specific fields used by verify() + } +} +``` + +- `responses_create_params.input` follows OpenAI message format +- `verifier_metadata` is opaque to the framework — define whatever fields your benchmark's `verify()` method needs (test cases, expected answers, task IDs, etc.) + +## Step 2: Convert source data + +If converting from another format: + +1. **Write the conversion script in the source repo**, not in NeMo Gym. Prompt files also belong in the source repo. Exception: when there is no external source repo. +2. Map source fields to `responses_create_params.input` messages and `verifier_metadata` +3. System prompts go in the first message with `role: system` +4. Validate every line is valid JSON and has the required top-level keys + +### Tool definitions in input +For benchmarks involving tool use, `responses_create_params` can include a `tools` array alongside `input`: +```json +{ + "responses_create_params": { + "input": [...], + "tools": [ + { + "type": "function", + "function": { + "name": "get_weather", + "description": "Get current weather", + "parameters": {"type": "object", "properties": {"city": {"type": "string"}}} + } + } + ], + "parallel_tool_calls": true + }, + "verifier_metadata": {...} +} +``` +Tools and `parallel_tool_calls` are passed through to the model server. Fill in tool `description` fields — models perform significantly better with descriptive tool definitions. + +### verifier_metadata patterns by domain +The `verifier_metadata` structure varies by benchmark type. Study the existing server's `verify()` to know which fields it reads: + +| Domain | Common verifier_metadata fields | +|--------|-------------------------------| +| Code generation | `test_cases` [{input, expected_output}], `function_name`, `language` | +| Math | `expected_answer`, `solution_type` (numeric, symbolic, proof) | +| SQL | `db_id`, `gold_sql`, `ignore_order`, `condition_cols` | +| Safety/Jailbreak | `adversarial_prompt`, `attack_type` | +| LLM-as-Judge | `expected_answer`, `template_metadata` {`output_regex`} | +| Search/QA | `ground_truth`, `question` | + +### Data leakage check +Before finalizing data, verify that: +- The expected answer does NOT appear verbatim in the system or user prompt +- The `verifier_metadata` doesn't contain fields that could leak through to the model (only `responses_create_params.input` reaches the model; `verifier_metadata` stays server-side) +- For judge-based benchmarks, the judge prompt template doesn't inadvertently reveal the expected answer format + +## Step 3: Generate example.jsonl + +Create `data/example.jsonl` with exactly 5 entries. These are committed to git and used for smoke testing. + +Selection criteria: +- Pick entries that exercise different code paths in `verify()` +- Include at least one "easy" case (should always get reward 1.0 from a capable model) +- Include at least one edge case (unusual input format, boundary condition) +- Keep entries small — example data should load instantly + +## Step 4: Validate data + +```bash +# Validate example data (required before PR submission) +ng_prepare_data "+config_paths=[resources_servers/my_benchmark/configs/my_benchmark.yaml]" \ + +output_dirpath=/tmp/prepare +mode=example_validation +``` + +Check for: +- Every line parses as valid JSON +- `responses_create_params.input` is a non-empty list of messages +- Each message has `role` and `content` fields +- `verifier_metadata` fields match what `verify()` expects + +## Step 5: Upload to GitLab registry + +Train and validation datasets must NOT be committed to git. Upload them: + +```bash +ng_upload_dataset_to_gitlab \ + +dataset_name=my_benchmark \ + +version=0.0.1 \ + +input_jsonl_fpath=resources_servers/my_benchmark/data/my_dataset.jsonl +``` + +Requires MLflow credentials in `env.yaml`: +```yaml +mlflow_tracking_uri: +mlflow_tracking_token: +``` + +After upload, verify download works: +```bash +ng_prepare_data "+config_paths=[resources_servers/my_benchmark/configs/my_benchmark.yaml]" \ + +output_dirpath=data/my_benchmark +mode=train_preparation +should_download=true +data_source=gitlab +``` + +## Step 6: Wire YAML config + +Add dataset entries to the server's YAML config. Both `jsonl_fpath` and `gitlab_identifier` must coexist for train/validation datasets: + +```yaml +datasets: +- name: my_dataset + type: train + jsonl_fpath: resources_servers/my_benchmark/data/my_dataset.jsonl + gitlab_identifier: + dataset_name: my_benchmark + version: 0.0.1 + artifact_fpath: my_dataset.jsonl + license: MIT +- name: example + type: example + jsonl_fpath: resources_servers/my_benchmark/data/example.jsonl +``` + +- `jsonl_fpath` is the local download destination +- `gitlab_identifier` tells the system where to fetch from +- `example` datasets don't need `gitlab_identifier` — they're committed to git +- `license` is required for train and validation datasets + +## Step 7: Fix .gitignore + +Check `data/.gitignore`. The scaffold generates default patterns: +``` +*train.jsonl +*validation.jsonl +*train_prepare.jsonl +*validation_prepare.jsonl +*example_prepare.jsonl +``` + +If your filename doesn't match (e.g. `my_eval.jsonl`), add a custom pattern. If data was previously tracked: +```bash +git rm --cached +``` diff --git a/.claude/skills/gym-data/evals/evals.json b/.claude/skills/gym-data/evals/evals.json new file mode 100644 index 0000000000..70a47d1c71 --- /dev/null +++ b/.claude/skills/gym-data/evals/evals.json @@ -0,0 +1,42 @@ +{ + "skill_name": "gym-data", + "evals": [ + { + "id": 1, + "prompt": "Generate an example.jsonl with 5 entries for a tool-calling benchmark. The model needs to call a search_web tool and a calculate tool. The verify method checks that the model called the correct tool with valid arguments.", + "expected_output": "JSONL where responses_create_params includes a tools array with function definitions for search_web and calculate, alongside the input messages. Each tool should have non-empty descriptions.", + "assertions": [ + "responses_create_params includes a 'tools' array with at least 2 tool definitions", + "Each tool has type 'function' with name, description, and parameters", + "Tool descriptions are non-empty (not '' or missing)", + "verifier_metadata specifies which tool should be called and with what arguments", + "At least one entry expects search_web and at least one expects calculate", + "The file contains exactly 5 lines of valid JSON" + ] + }, + { + "id": 2, + "prompt": "I'm building an LLM-as-judge math benchmark using equivalence_llm_judge. What should verifier_metadata look like? Some of my expected answers are numeric, some are symbolic expressions.", + "expected_output": "verifier_metadata structure with expected_answer field, plus guidance on output_regex for per-record extraction, and how template_metadata works for the judge.", + "assertions": [ + "verifier_metadata includes an expected_answer field", + "template_metadata with output_regex is mentioned for per-record regex extraction", + "The response distinguishes numeric answers (can use regex) from symbolic (need judge evaluation)", + "The data leakage risk is mentioned (expected_answer must not appear in the prompt)", + "extraction_length_threshold is mentioned for long answers that should skip regex" + ] + }, + { + "id": 3, + "prompt": "Convert a SQL benchmark dataset. Each source row has: question (natural language), db_id (database name), gold_sql (reference SQL). Generate 5 example entries.", + "expected_output": "JSONL with verifier_metadata containing db_id, gold_sql, question, plus additional fields like ignore_order and condition_cols that spider2_lite expects.", + "assertions": [ + "verifier_metadata contains db_id, gold_sql, and question fields", + "ignore_order field is present (boolean for whether row order matters)", + "condition_cols field is present or mentioned (columns to check for equivalence)", + "instance_id or equivalent unique identifier is present per entry", + "The system prompt instructs the model to output SQL" + ] + } + ] +} diff --git a/.claude/skills/gym-debug/SKILL.md b/.claude/skills/gym-debug/SKILL.md new file mode 100644 index 0000000000..750211a1b0 --- /dev/null +++ b/.claude/skills/gym-debug/SKILL.md @@ -0,0 +1,121 @@ +--- +name: gym-debug +description: > + Diagnose NeMo Gym server failures, rollout errors, and infrastructure issues. Use when + servers won't start, rollouts fail or hang, rewards are unexpected, or there are + concurrency/scaling issues. Covers request tracing, log analysis, config validation, + Ray diagnostics, and common failure modes. +license: Apache-2.0 +compatibility: Requires Python 3.12+, access to NeMo Gym server logs. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(python:*) Bash(ng_*) Bash(git:*) Bash(curl:*) Bash(ps:*) Read Grep Glob +--- + +# NeMo Gym Debugging + +## Step 1: Establish the failure mode + +Categorize the problem before investigating: + +| Symptom | Category | Start at | +|---------|----------|----------| +| Server won't start | Startup | Step 2 | +| Requests hang or timeout | Concurrency | Step 3 | +| Rollouts return all 0.0 rewards | Verification | Step 4 | +| Servers crash under load | Scaling | Step 5 | +| Config errors on launch | Configuration | Step 6 | +| Inconsistent results across runs | Nondeterminism | Step 7 | + +## Step 2: Startup failures + +1. Check `ng_status` — are all servers reporting healthy? +2. Read server logs for import errors, missing dependencies, or port conflicts +3. If using auto-installed tools, check that `ensure_()` completed — look for the install directory (e.g. `.lean4/`, `.go/`) +4. Verify `env.yaml` has correct model endpoint config (`policy_base_url`, `policy_api_key`, `policy_model_name`) +5. Check that YAML config composes correctly: `ng_dump_config "+config_paths=[...]"` + +## Step 3: Concurrency issues + +Symptoms: requests hang, timeouts, server becomes unresponsive at scale. + +1. **Missing semaphore**: Check that subprocess calls are bounded by `asyncio.Semaphore`. Unbounded spawning exhausts system resources. +2. **httpx in use**: Any httpx/httpcore usage causes O(n^2) connection pooling hangs at 16k+ requests. Must use aiohttp via `nemo_gym.server_utils.request()`. +3. **`ray.get()` blocking event loop**: Use `await future` for Ray remote tasks. +4. **aiohttp session lifecycle**: The global client is a singleton with connection pooling. Verify it's not being created per-request. +5. **Cookie propagation**: In stateful environments, missing `cookies=request.cookies` on downstream calls causes session loss, leading to repeated initialization or state corruption. + +## Step 4: Verification failures + +All rollouts returning reward 0.0 when they shouldn't: + +1. **Output parsing**: Is the model's response being extracted correctly? Check code extraction regex. Common miss: markdown fences with language tags (` ```python ` vs ` ``` `). +2. **Think blocks**: Thinking models wrap output in ``/`` blocks. These must be stripped before parsing. +3. **Test case format**: Does `verifier_metadata` in the JSONL match what `verify()` expects? Field name mismatches are silent failures. +4. **Subprocess execution**: If verify() runs code, check: is the binary installed and on PATH? Is the working directory correct? Is the timeout sufficient? +5. **Manual test**: Call `/verify` directly with a known-good input to isolate whether the issue is in verification or upstream. + +```bash +curl -X POST http://localhost:/verify \ + -H "Content-Type: application/json" \ + -d '{"response": {"output_text": "known good answer"}, "verifier_metadata": {...}}' +``` + +## Step 5: Scaling failures + +Servers crash or OOM under high concurrency (4k-65k requests): + +1. Check semaphore value — too high exhausts memory, too low bottlenecks throughput +2. Check Ray worker count and memory allocation +3. Look for memory leaks: subprocess output not being released, accumulating results in memory +4. Verify `errors="replace"` on all subprocess decode — non-UTF8 output without this flag can cause exceptions that leak resources + +## Step 6: Configuration issues + +1. Run `ng_dump_config "+config_paths=[...]"` to see the merged config +2. Check instance name consistency — agent must reference exact names of resources and model servers +3. Verify Hydra override syntax: `+key=value` for new keys, `key=value` for existing +4. Check for YAML indentation issues (especially in dataset sections with nested `gitlab_identifier`) +5. OmegaConf interpolation errors: `${var}` references must resolve + +## Step 7: Nondeterminism + +Results vary significantly across identical runs: + +1. **Temperature**: Ensure `temperature: 1.0` (or your chosen value) is being passed correctly +2. **Random seeds**: If verify() uses randomness (shuffled test cases, random sampling), seed it +3. **Stateful environments**: Check that state is being properly reset between requests — leaked state from one request affects the next +4. **Race conditions**: In multi-turn agents, verify that async operations are properly sequenced + +## LLM-as-Judge debugging + +Judge-based benchmarks (equivalence_llm_judge, jailbreak_detection) have additional failure modes: + +1. **Missing judge model server**: Judge configs require a second model server instance. If the config only defines `policy_model` but the server also needs `judge_model`, all judge calls fail silently or return default rewards. + +2. **Judge rate limiting**: `judge_endpoint_max_concurrency` bounds concurrent judge calls. If set too low, rollout collection stalls. If too high, the judge API returns 429s. Check the judge model server logs separately from the resources server logs. + +3. **Two-stage reward issues** (jailbreak_detection with `use_combined_reward: true`): The final reward is `safety_reward * quality_reward`. If safety passes (1.0) but quality fails (0.3), the combined reward is 0.3, not 0.0. This is intentional partial credit, not a bug — but can look like inconsistent rewards if you don't know the formula. + +4. **Positional bias** (equivalence_llm_judge with `check_twice_swap: true`): The judge runs twice with expected/generated answers swapped. If the two runs disagree, `reward_if_swap_fails` applies (default 0.0). High disagreement rates indicate the judge is sensitive to answer ordering, not answer correctness. + +5. **Regex extraction failures**: Judge-based servers often extract answers via regex before judging. Check `question_extract_regex`, `response_extract_regex`, and per-record `template_metadata.output_regex`. When regex fails, the server may fall back to `check_full_generation_on_fail` — giving partial credit (`reward_if_full_generation_succeeds: 0.5`) instead of 0.0. + +## Custom VerifyResponse fields + +Production servers return more than just `reward`. These extra fields are critical for debugging: + +| Server | Extra fields | What they tell you | +|--------|-------------|-------------------| +| code_gen | `extracted_model_code`, `result`, `unit_tests_time_taken`, `reasoning_format_violation_rate` | What code was extracted, what happened when it ran, whether thinking tags were malformed | +| spider2_lite | `extracted_sql`, `execution_match`, `failure_reason` (enum: NO_SQL_EXTRACTED, EXECUTION_ERROR, etc.) | Whether SQL was found, whether it ran, why it failed | +| equivalence_llm_judge | `expected_answer`, `judge_evaluations` [{verdict_label}] | What the judge saw and decided | +| tavily_search | `num_tool_calls`, `metrics` [{function, status, time_taken}] | How many API calls were made and which failed | + +When debugging, always read these extra fields from the rollout JSONL — they tell you exactly where in the pipeline things went wrong. + +## Ray-specific issues + +- **Socket path too long**: On HPC/Lustre with long working directory paths, Ray's AF_UNIX socket exceeds the 107-byte Linux limit. Fix: `export RAY_TMPDIR=/tmp` before running. +- **`ng_test` venv isolation**: `os.environ` changes in Python don't propagate to `ng_test` venvs. Set env vars externally: `RAY_TMPDIR=/tmp ng_test ...` diff --git a/.claude/skills/gym-debug/evals/evals.json b/.claude/skills/gym-debug/evals/evals.json new file mode 100644 index 0000000000..ba2d08941a --- /dev/null +++ b/.claude/skills/gym-debug/evals/evals.json @@ -0,0 +1,41 @@ +{ + "skill_name": "gym-debug", + "evals": [ + { + "id": 1, + "prompt": "My equivalence_llm_judge benchmark gives inconsistent rewards. Some tasks get 0.0 even when the model's answer looks correct, and I'm seeing rewards of 0.5 that I didn't expect. check_twice_swap is enabled.", + "expected_output": "Diagnosis covering judge positional bias (swap disagrees), regex extraction fallback (partial credit 0.5), and how to read judge_evaluations from the rollout JSONL to see what the judge actually decided.", + "assertions": [ + "check_twice_swap positional bias is identified as cause of unexpected 0.0 rewards", + "reward_if_swap_fails is mentioned as the value applied when swap check disagrees", + "The 0.5 rewards are explained as check_full_generation_on_fail fallback (reward_if_full_generation_succeeds)", + "Reading judge_evaluations from rollout JSONL is recommended for debugging", + "Regex extraction failure is identified as a potential upstream cause" + ] + }, + { + "id": 2, + "prompt": "My code_gen benchmark works for instruct models but thinking models score much lower than expected. The answers look correct when I read the output manually.", + "expected_output": "Diagnosis identifying thinking tag interference with code extraction, with specific mention of reasoning_format_violation_rate and think-block stripping.", + "assertions": [ + "Think/thinking block interference with code extraction is identified as the cause", + "reasoning_format_violation_rate field is mentioned as a diagnostic indicator", + "The response explains that thinking models wrap output in / tags", + "Stripping think blocks before code extraction is recommended as the fix", + "The response suggests checking extracted_model_code in the rollout JSONL to confirm" + ] + }, + { + "id": 3, + "prompt": "My jailbreak_detection benchmark with use_combined_reward gives rewards of 0.3 for some entries. I expected only 0.0 or 1.0.", + "expected_output": "Explanation of the two-stage combined reward formula (safety_reward * quality_reward) and how reward_if_quality_low creates the 0.3 value.", + "assertions": [ + "The two-stage reward formula (safety * quality) is explained", + "reward_if_quality_low is identified as the source of the 0.3 value", + "The response clarifies this is intentional partial credit, not a bug", + "The response explains the two judge calls: safety verdict then quality check", + "The math is shown: 1.0 (safe) * 0.3 (low quality) = 0.3" + ] + } + ] +} diff --git a/.claude/skills/gym-profile/SKILL.md b/.claude/skills/gym-profile/SKILL.md new file mode 100644 index 0000000000..6fb8b586bf --- /dev/null +++ b/.claude/skills/gym-profile/SKILL.md @@ -0,0 +1,146 @@ +--- +name: gym-profile +description: > + Analyze rollout results and reward distributions for NeMo Gym benchmarks. Use when + baselining a benchmark, comparing model performance, diagnosing low pass rates, + investigating reward variance, or validating that a benchmark produces expected scores. + Covers rollout collection, reward profiling, aggregate metrics, and failure analysis. +license: Apache-2.0 +compatibility: Requires Python 3.12+, running NeMo Gym servers. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(python:*) Bash(ng_*) Bash(jq:*) Read Grep Glob +--- + +# NeMo Gym Reward Profiling + +## Step 1: Collect rollouts + +```bash +ng_collect_rollouts +agent_name= \ + +input_jsonl_fpath= \ + +output_jsonl_fpath=results/rollouts.jsonl \ + +num_repeats=5 \ + "+responses_create_params={max_output_tokens: 16384, temperature: 1.0}" +``` + +Start with `example.jsonl` for a quick smoke test before running on full datasets. + +## Step 2: Compute per-task pass rates + +```bash +ng_reward_profile \ + +input_jsonl_fpath= \ + +rollouts_jsonl_fpath=results/rollouts.jsonl \ + +output_jsonl_fpath=results/profiled.jsonl \ + +pass_threshold=1.0 +``` + +## Step 3: Aggregate metrics + +```bash +python scripts/print_aggregate_results.py +jsonl_fpath=results/profiled.jsonl +``` + +Key metrics: +- **pass@1** = `avg_reward` — average reward across all rollouts. The primary metric. +- **pass@k** = derived from `max_reward` — whether the model got it right at least once in k attempts. + +## Step 4: Diagnose issues + +### Variance check +Increase `num_repeats` until variance is < 1% across runs on the same model. If variance remains high, the benchmark may be nondeterministic (randomized test cases, environment state, etc.). + +### Suspicious patterns + +| Pattern | Likely cause | +|---------|-------------| +| All rewards 0.0 | verify() is rejecting everything — check code extraction, output parsing, or test case matching | +| All rewards 1.0 | verify() is too lenient — check that wrong answers actually fail | +| Closed-source < open-source | Bug in the benchmark — closed-source models should generally score at or above open-source | +| High variance across repeats | Nondeterministic verification or model sensitivity to prompt | +| Scores don't match published numbers | For external benchmarks, compare against the original repo's results. Score mismatch signals an integration bug | +| Rewards are 0.0, 0.3, 0.5, 1.0 (not binary) | Server uses partial rewards — check judge config fields like `reward_if_quality_low`, `reward_if_full_generation_succeeds`, `reward_if_swap_fails` | +| Thinking model scores lower than instruct | `reasoning_format_violation_rate` may be high — check if thinking tags are being stripped before answer extraction | + +### Inspect failures using custom VerifyResponse fields + +Don't just look at aggregates. Production servers return diagnostic fields beyond `reward`. Read these from the rollout JSONL: + +```python +import json +with open("results/rollouts.jsonl") as f: + for line in f: + entry = json.loads(line) + if entry.get("reward", 0) == 0.0: + # Check server-specific diagnostic fields + print("extracted_code:", entry.get("extracted_model_code")) + print("failure_reason:", entry.get("failure_reason")) + print("extracted_sql:", entry.get("extracted_sql")) + print("judge_evaluations:", entry.get("judge_evaluations")) + print("execution_match:", entry.get("execution_match")) + break +``` + +Key diagnostic fields by server type: +- **code_gen**: `extracted_model_code` (what was extracted), `result` (execution output), `reasoning_format_violation_rate` +- **spider2_lite**: `extracted_sql`, `execution_match`, `failure_reason` (enum: NO_SQL_EXTRACTED, EXECUTION_ERROR, GOLD_EXECUTION_ERROR) +- **equivalence_llm_judge**: `expected_answer`, `judge_evaluations` (list of verdict objects) +- **tavily_search**: `num_tool_calls`, `metrics` (per-call timing and status) + +These fields tell you exactly WHERE in the pipeline the failure occurred — extraction, execution, or judgment. + +### Common failure causes +- Model output wrapped in `` blocks that weren't stripped +- Code extraction regex too narrow (misses markdown fences with language tags) +- Test case expects exact string match when semantic match is needed +- Subprocess timeout too short for complex tasks +- Judge regex fails, falls back to partial credit (`reward_if_full_generation_succeeds: 0.5`) — not a real failure, just the fallback path +- Judge positional bias: `check_twice_swap` causes disagreements that map to `reward_if_swap_fails: 0.0` + +### Per-task difficulty analysis + +After profiling, look for ceiling and floor effects: + +```python +import json +tasks = {} +with open("results/profiled.jsonl") as f: + for line in f: + entry = json.loads(line) + task_id = entry.get("task_index", 0) + tasks[task_id] = entry.get("avg_reward", 0) + +always_pass = [t for t, r in tasks.items() if r >= 0.95] +always_fail = [t for t, r in tasks.items() if r <= 0.05] +print(f"Ceiling tasks (>= 95%): {len(always_pass)}/{len(tasks)}") +print(f"Floor tasks (<= 5%): {len(always_fail)}/{len(tasks)}") +``` + +- **> 30% ceiling tasks**: Benchmark is too easy for the tested models. These tasks add noise, not signal. +- **> 30% floor tasks**: Benchmark is too hard or these tasks have extraction bugs. Inspect a sample. +- **Ideal**: Most tasks between 10-90% pass rate across model tiers, with clear separation between stronger and weaker models. + +## Step 5: Multi-model comparison + +For baselining, run against at least: +- Your policy model of interest +- One open-source instruct model (e.g. Qwen 3 30B A3B Instruct) +- One open-source thinking model (e.g. Qwen 3 30B A3B Thinking) +- One closed-source model (e.g. GPT-5 Nano or GPT-5) + +Use `openai_model` for endpoints supporting `/v1/responses`, `vllm_model` for `/v1/chat/completions`. + +Compare results in a table: + +``` +| Model | pass@1 | pass@5 | num_repeats | +|--------------------------|--------|--------|-------------| +| Policy (your model) | 0.XX | 0.XX | 5 | +| Qwen 3 30B A3B Instruct | 0.XX | 0.XX | 5 | +| Qwen 3 30B A3B Thinking | 0.XX | 0.XX | 5 | +| GPT-5 Nano | 0.XX | 0.XX | 5 | +``` + +Include this table and W&B links in your PR description. Set `verified: true` in the YAML config after successful baselining. diff --git a/.claude/skills/gym-profile/evals/evals.json b/.claude/skills/gym-profile/evals/evals.json new file mode 100644 index 0000000000..57767d5ee7 --- /dev/null +++ b/.claude/skills/gym-profile/evals/evals.json @@ -0,0 +1,42 @@ +{ + "skill_name": "gym-profile", + "evals": [ + { + "id": 1, + "prompt": "My code_gen benchmark results show that a thinking model (Qwen 3 Thinking) scores 15% lower than the instruct variant (Qwen 3 Instruct) on the same tasks. That seems wrong. Help me analyze.", + "expected_output": "Analysis identifying reasoning_format_violation_rate as the likely cause, with instructions to check extracted_model_code and think-block stripping, plus per-task difficulty breakdown.", + "assertions": [ + "reasoning_format_violation_rate is mentioned as a diagnostic field to check", + "Think-block stripping failure is identified as the likely cause", + "The response recommends checking extracted_model_code in rollout JSONL to see what was actually extracted", + "Per-task comparison between the two models is recommended to find which tasks diverge", + "The response does NOT dismiss the result as expected behavior" + ] + }, + { + "id": 2, + "prompt": "I profiled my spider2_lite SQL benchmark. 40% of tasks have 0% pass rate across all models, and 35% have 100% pass rate. The middle is thin. Is this benchmark useful for training?", + "expected_output": "Analysis of ceiling/floor effects with concrete thresholds, recommendation to inspect failure_reason field for the 0% tasks, and guidance on whether to trim the dataset.", + "assertions": [ + "Ceiling effect (35% always-pass tasks) is flagged as adding noise not signal", + "Floor effect (40% always-fail tasks) is flagged — could be bugs or genuinely too hard", + "failure_reason field (NO_SQL_EXTRACTED, EXECUTION_ERROR, etc.) is recommended for diagnosing the 0% tasks", + "The response recommends checking if floor tasks fail due to extraction bugs vs genuine difficulty", + "Trimming always-pass and always-fail tasks is suggested to improve training signal", + "The ideal distribution (10-90% per task with model separation) is mentioned" + ] + }, + { + "id": 3, + "prompt": "My equivalence_llm_judge results show pass@1 of 0.72 but I'm seeing reward values of 0.0, 0.5, and 1.0 in the rollouts. I thought rewards should be binary. How do I interpret this for pass@k calculation?", + "expected_output": "Explanation of partial rewards from judge fallback paths, how they affect pass@1 (avg_reward) vs pass@k (threshold-based), and which config fields control the partial reward values.", + "assertions": [ + "The 0.5 value is explained as reward_if_full_generation_succeeds from the fallback path", + "The distinction between pass@1 (avg_reward, includes partials) and pass@k (threshold-based) is explained", + "pass_threshold parameter in ng_reward_profile is mentioned for controlling how partials count toward pass@k", + "The config fields controlling partial rewards are named (check_full_generation_on_fail, reward_if_full_generation_succeeds, reward_if_swap_fails)", + "check_twice_swap is mentioned as another source of non-binary rewards" + ] + } + ] +} diff --git a/.claude/skills/gym-review/SKILL.md b/.claude/skills/gym-review/SKILL.md new file mode 100644 index 0000000000..e18cd81150 --- /dev/null +++ b/.claude/skills/gym-review/SKILL.md @@ -0,0 +1,110 @@ +--- +name: gym-review +description: > + Review code changes for NeMo Gym anti-patterns and correctness issues. Use when + reviewing a PR, auditing a benchmark implementation, or checking a resources server, + agent, or config before merge. Catches: httpx usage (must use aiohttp), ray.get() in + async context, missing semaphores, non-binary rewards, missing think-block stripping, + env vars instead of YAML config, test coverage gaps, and cookie propagation issues. +license: Apache-2.0 +compatibility: Requires Python 3.10+. Works standalone or inside the NeMo Gym repo. +metadata: + author: nvidia-nemo-gym + version: "2.0" +allowed-tools: Bash Read Grep Glob +--- + +# NeMo Gym Code Review + +Review code for anti-patterns that cause production failures in NeMo Gym's async, high-concurrency microservice architecture (4k-65k concurrent requests). + +This skill is **script-first**: run the deterministic checker, then apply judgment for context the script can't catch. + +## Step 1: Run the automated checker + +Run `scripts/review.py` against the target path. It checks 11 Python rules and 1 YAML rule. + +```bash +# Scan a directory (most common — scan the whole server) +python scripts/review.py + +# Scan with JSON output (for programmatic use) +python scripts/review.py --json + +# Only BLOCK-level findings +python scripts/review.py --severity BLOCK +``` + +The script exits 1 if any BLOCK-level findings exist, 0 otherwise. + +> **Note**: `scripts/review.py` is self-contained — no dependencies beyond the Python standard library. It works outside the NeMo Gym repo. + +## Step 2: Interpret the results + +The script reports findings at two severity levels: + +### BLOCK (must fix before merge) + +| Rule | What it catches | +|------|----------------| +| `httpx-usage` | httpx/httpcore imports — O(n^2) connection pooling hangs at 16k+ requests | +| `ray-get-async` | `ray.get()` in async context — blocks the event loop | +| `missing-semaphore` | Subprocess calls without `asyncio.Semaphore` — unbounded at scale | +| `missing-errors-replace` | `.decode()` without `errors="replace"` — crashes on non-UTF8 | +| `env-var-config` | `os.environ`/`os.getenv` for config — must use YAML/Hydra | +| `wrong-client` | litellm/anthropic imports — must use `nemo_gym/openai_utils.py` | +| `missing-cookies` | Agent `server_client.post()` without `cookies=` — breaks stateful sessions | +| `missing-token-ids` | Multi-turn agent without token ID accumulation — breaks RL training | +| `non-binary-reward` | Reward values other than 0.0/1.0 without documentation | + +### WARN (should fix) + +| Rule | What it catches | +|------|----------------| +| `missing-think-strip` | Parses model output without stripping `` blocks | +| `sync-endpoint` | `def verify`/`def run` instead of `async def` | +| `verified-true` | Config has `verified: true` — confirm baselining was done | +| `missing-gitlab-id` | Train/validation dataset without `gitlab_identifier` | +| `missing-license` | Train/validation dataset without `license` field | + +For each finding, the script provides the file, line number, rule name, description, and fix suggestion. + +## Step 3: Apply judgment (what the script can't catch) + +The script handles pattern matching. These require human/agent judgment: + +1. **Test coverage completeness**: Does the server have tests for verify pass, verify fail (wrong output), verify fail (no extraction), verify fail (compilation error if applicable), and verify timeout? Target >= 95% coverage. + +2. **`pytest.mark.skipif` for external tools**: Tests requiring tools not in the standard library should use `skipif(shutil.which("tool") is None, ...)`. + +3. **Unguarded optional fields**: Access patterns like `body.field.get("key")` should use `(body.field or {}).get("key", default)`. + +4. **YAML instance name consistency**: Agent configs reference resources/model servers by name — verify these match actual instance names in the config. + +5. **Intentional partial rewards**: If the script flags `non-binary-reward`, check whether the partial credit is documented and intentional (e.g., judge-based servers with `check_twice_swap`). + +## Step 4: Report + +Structure the review as: + +``` +## Review: [server/agent name] + +### Automated findings + + +### Manual checks +- Test coverage: [pass/fail/not applicable] +- Optional field guards: [pass/fail] +- YAML consistency: [pass/fail] + +### Summary +X BLOCK, Y WARN — [merge/do not merge] +``` + +## References + +Full context for each anti-pattern and its fix: + +- `references/anti-patterns.md` — Why each pattern fails in production, with architecture context +- `references/fix-patterns.md` — Production code patterns: aiohttp adapter, cookie chain, token accumulation, semaphore-subprocess, think-block stripping variants diff --git a/.claude/skills/gym-review/evals/evals.json b/.claude/skills/gym-review/evals/evals.json new file mode 100644 index 0000000000..75b41ca8ac --- /dev/null +++ b/.claude/skills/gym-review/evals/evals.json @@ -0,0 +1,52 @@ +{ + "skill_name": "gym-review", + "evals": [ + { + "id": 1, + "prompt": "Review the file evals/files/sample_server_with_bugs.py for NeMo Gym anti-patterns.", + "expected_output": "A review identifying all 7 BLOCK findings: httpx import, ray.get() in async, missing semaphore, missing errors='replace' (x2), env var config, and non-binary reward.", + "files": ["evals/files/sample_server_with_bugs.py"], + "assertions": [ + "The agent runs scripts/review.py against the file", + "httpx-usage BLOCK finding is reported", + "ray-get-async BLOCK finding is reported", + "missing-semaphore BLOCK finding is reported", + "missing-errors-replace BLOCK finding is reported (stdout and stderr)", + "env-var-config BLOCK finding is reported for MY_API_KEY", + "non-binary-reward BLOCK finding is reported for the 0.5 value", + "The report recommends NOT merging due to BLOCK findings", + "Each finding includes file path and line number" + ] + }, + { + "id": 2, + "prompt": "Review the multi-turn agent at evals/files/sample_multi_turn_agent.py before I submit a PR.", + "expected_output": "A review identifying 2 BLOCK findings (missing cookies, missing token IDs) and 1 WARN (missing think-strip), with fix suggestions referencing the propagation patterns.", + "files": ["evals/files/sample_multi_turn_agent.py"], + "assertions": [ + "The agent runs scripts/review.py against the file", + "missing-cookies BLOCK finding is reported", + "missing-token-ids BLOCK finding is reported", + "missing-think-strip WARN finding is reported", + "The fix for cookies references passing cookies=request.cookies and updating from response", + "The fix for token IDs mentions accumulating prompt_token_ids and generation_token_ids across turns", + "The report recommends NOT merging due to BLOCK findings" + ] + }, + { + "id": 3, + "prompt": "Review evals/files/sample_config.yaml and evals/files/sample_clean_server.py together — this is a new benchmark submission.", + "expected_output": "YAML review catches 3 WARN findings (verified:true, missing gitlab_identifier, missing license). Clean server gets zero findings. Overall assessment acknowledges YAML needs fixes but no BLOCK issues.", + "files": ["evals/files/sample_config.yaml", "evals/files/sample_clean_server.py"], + "assertions": [ + "The agent runs scripts/review.py against both files", + "verified-true WARN is reported for the YAML config", + "missing-gitlab-id WARN is reported for the train dataset", + "missing-license WARN is reported for the train dataset", + "The clean server is reported as having no issues", + "The report notes no BLOCK findings — merge is possible after fixing WARNs", + "The report mentions that verified should be false for new unbaselined servers" + ] + } + ] +} diff --git a/.claude/skills/gym-review/evals/files/sample_clean_server.py b/.claude/skills/gym-review/evals/files/sample_clean_server.py new file mode 100644 index 0000000000..948849e0cb --- /dev/null +++ b/.claude/skills/gym-review/evals/files/sample_clean_server.py @@ -0,0 +1,47 @@ +"""Sample clean resources server — no anti-patterns. + +review.py should report zero findings on this file. +""" + +import asyncio + +from nemo_gym.server_utils import request, raise_for_status +from nemo_gym.servers.resources_server import SimpleResourcesServer + + +class CleanServerConfig: + timeout: int = 30 + num_processes: int = 4 + + +class CleanServer(SimpleResourcesServer): + config: CleanServerConfig + + def model_post_init(self, __context): + super().model_post_init(__context) + self.semaphore = asyncio.Semaphore(self.config.num_processes) + + async def verify(self, body): + code = body.get("code", "") + if not code: + return {"reward": 0.0} + + async with self.semaphore: + proc = await asyncio.create_subprocess_exec( + "python", "-c", code, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for( + proc.communicate(), timeout=self.config.timeout + ) + output = stdout.decode(errors="replace") + errors = stderr.decode(errors="replace") + + expected = body.get("expected_output", "") + if output.strip() == expected.strip(): + reward = 1.0 + else: + reward = 0.0 + + return {"reward": reward, "output": output, "errors": errors} diff --git a/.claude/skills/gym-review/evals/files/sample_config.yaml b/.claude/skills/gym-review/evals/files/sample_config.yaml new file mode 100644 index 0000000000..91283f8885 --- /dev/null +++ b/.claude/skills/gym-review/evals/files/sample_config.yaml @@ -0,0 +1,35 @@ +# Sample YAML config with intentional issues for eval testing. +my_benchmark: + resources_servers: + my_benchmark: + entrypoint: app.py + domain: coding + verified: true + timeout: 30 + num_processes: 4 + datasets: + - name: my_example + type: example + jsonl_fpath: resources_servers/my_benchmark/data/example.jsonl + - name: my_train + type: train + jsonl_fpath: resources_servers/my_benchmark/data/train.jsonl + - name: my_validation + type: validation + jsonl_fpath: resources_servers/my_benchmark/data/validation.jsonl + gitlab_identifier: + dataset_name: my_benchmark + version: 0.0.1 + artifact_fpath: validation.jsonl + license: Apache-2.0 + +my_agent: + responses_api_agents: + simple_agent: + entrypoint: app.py + resources_server: + type: resources_servers + name: my_benchmark + model_server: + type: responses_api_models + name: policy_model diff --git a/.claude/skills/gym-review/evals/files/sample_multi_turn_agent.py b/.claude/skills/gym-review/evals/files/sample_multi_turn_agent.py new file mode 100644 index 0000000000..af00d0bcf6 --- /dev/null +++ b/.claude/skills/gym-review/evals/files/sample_multi_turn_agent.py @@ -0,0 +1,59 @@ +"""Sample multi-turn agent with intentional anti-patterns for eval testing.""" + +import asyncio + +from pydantic import BaseModel +from starlette.requests import Request + +from nemo_gym.servers.responses_api_agent import SimpleResponsesAPIAgent + + +class MultiTurnAgentConfig(BaseModel): + max_turns: int = 3 + resources_server: dict = {} + model_server: dict = {} + name: str = "multi_turn_agent" + + +class MultiTurnAgent(SimpleResponsesAPIAgent): + config: MultiTurnAgentConfig + + async def run(self, request: Request, body): + current_input = body.model_dump() + + for turn in range(self.config.max_turns): + # Model call - not forwarding session state + gen_resp = await self.server_client.post( + server_name=self.config.name, + url_path="/v1/responses", + json=current_input, + ) + + model_response = await gen_resp.json() + output_text = model_response.get("output_text", "") + + # Parsing output without stripping think blocks + if "```" in output_text: + code = output_text.split("```")[1] + else: + code = output_text + + # Verify call - not forwarding session state + verify_resp = await self.server_client.post( + server_name=self.config.resources_server.get("name", ""), + url_path="/verify", + json={"code": code, "verifier_metadata": body.get("verifier_metadata", {})}, + ) + + verify_data = await verify_resp.json() + if verify_data.get("reward", 0.0) == 1.0: + break + + # Build next turn input (no token ID accumulation) + current_input = { + "input": [ + {"role": "user", "content": f"Your code was wrong. Error: {verify_data.get('errors', '')}. Try again."} + ] + } + + return verify_data diff --git a/.claude/skills/gym-review/evals/files/sample_server_with_bugs.py b/.claude/skills/gym-review/evals/files/sample_server_with_bugs.py new file mode 100644 index 0000000000..7ef2420598 --- /dev/null +++ b/.claude/skills/gym-review/evals/files/sample_server_with_bugs.py @@ -0,0 +1,47 @@ +"""Sample resources server with intentional anti-patterns for eval testing.""" + +import asyncio +import os + +import httpx +from nemo_gym.server_utils import raise_for_status +from nemo_gym.servers.resources_server import SimpleResourcesServer + +API_KEY = os.getenv("MY_API_KEY") + + +class BuggyServer(SimpleResourcesServer): + config: dict + + def model_post_init(self, __context): + super().model_post_init(__context) + self.client = httpx.AsyncClient(base_url="http://localhost:8000") + + async def verify(self, body): + import ray + + future = ray.remote(lambda: 42).remote() + result = ray.get(future) + + code = body.get("code", "") + if not code: + return {"reward": 0.0} + + proc = await asyncio.create_subprocess_exec( + "python", "-c", code, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for( + proc.communicate(), timeout=30 + ) + output = stdout.decode() + errors = stderr.decode() + + expected = body.get("expected_output", "") + if output.strip() == expected.strip(): + reward = 1.0 + else: + reward = 0.5 + + return {"reward": reward, "output": output, "errors": errors} diff --git a/.claude/skills/gym-review/references/anti-patterns.md b/.claude/skills/gym-review/references/anti-patterns.md new file mode 100644 index 0000000000..cfe29e359c --- /dev/null +++ b/.claude/skills/gym-review/references/anti-patterns.md @@ -0,0 +1,163 @@ +# NeMo Gym Anti-Patterns Reference + +## Architecture context + +NeMo Gym is a microservice architecture with three FastAPI server types (resources, model, agent) communicating over async HTTP. Servers handle 4k-65k concurrent requests. Anti-patterns in this list cause production failures at scale. + +--- + +## BLOCK-level anti-patterns + +### 1. httpx-usage + +**What**: Any import of `httpx` or `httpcore`. + +**Why**: httpx/httpcore has O(n^2) connection pooling. At 16k+ concurrent requests, the connection pool scan becomes the bottleneck and servers hang. This was discovered in production and documented in `docs/infrastructure/engineering-notes/aiohttp-vs-httpx.md`. + +**Fix**: All async HTTP must go through `nemo_gym.server_utils.request()`, which uses aiohttp with a singleton connection pool. When wrapping external libraries that use httpx internally, replace their HTTP transport with an aiohttp adapter (see fix-patterns.md § aiohttp-adapter). + +--- + +### 2. ray-get-async + +**What**: Calling `ray.get()` in an async function. + +**Why**: `ray.get()` is a blocking call. In an async context, it blocks the entire event loop, preventing all other coroutines from running. One blocked `ray.get()` in a verify handler stops the server from processing any other requests. + +**Fix**: Ray futures are directly awaitable: `result = await future`. If you must use `ray.get()` (e.g., in a callback), wrap it in `loop.run_in_executor(None, ray.get, future)`. + +--- + +### 3. missing-semaphore + +**What**: `asyncio.create_subprocess_exec` or subprocess calls without a bounding `asyncio.Semaphore`. + +**Why**: Without concurrency control, every incoming request spawns a subprocess. At 65k concurrent requests, this exhausts file descriptors, memory, and CPU. The server crashes or the OS kills processes. + +**Fix**: Initialize a semaphore in `model_post_init()`: +```python +self.semaphore = asyncio.Semaphore(self.config.num_processes) +``` +Wrap all subprocess calls: +```python +async with self.semaphore: + proc = await asyncio.create_subprocess_exec(...) +``` + +--- + +### 4. non-binary-reward + +**What**: `verify()` returning reward values other than 0.0 or 1.0 without explicit documentation. + +**Why**: RL training frameworks assume binary rewards unless configured otherwise. Non-binary rewards silently change training dynamics. Partial credit IS used in some servers (e.g., jailbreak_detection's combined reward, equivalence_llm_judge's fallback), but it must be intentional and documented. + +**Fix**: Return exactly 0.0 or 1.0. If partial credit is intentional, add a comment explaining the reward structure and ensure the YAML config exposes the partial reward values (e.g., `reward_if_quality_low: 0.3`). + +--- + +### 5. missing-errors-replace + +**What**: `subprocess.stdout.decode()` or `.stderr.decode()` without `errors="replace"`. + +**Why**: Model-generated code can produce non-UTF8 output (binary data, corrupted strings). Without `errors="replace"`, the decode raises `UnicodeDecodeError`, which either crashes the request or leaks resources if the exception isn't caught properly. + +**Fix**: Always use `.decode(errors="replace")`. + +--- + +### 6. env-var-config + +**What**: Using `os.environ` or `os.getenv()` for configuration. + +**Why**: NeMo Gym uses Hydra/OmegaConf for all configuration. Environment variables bypass the config system, making deployments non-reproducible and configs non-composable. The ONE exception is `${oc.env:VAR,default}` in YAML for deployment-specific infrastructure values (sandbox hosts, etc.). + +**Allowed env vars**: `RAY_TMPDIR`, `PATH`, `LD_LIBRARY_PATH`, `HOME`, `USER`, `TMPDIR`, `CUDA_VISIBLE_DEVICES`. + +--- + +### 7. wrong-client + +**What**: Imports of `litellm`, `anthropic`, or OpenAI clients other than NeMo Gym's wrapper. + +**Why**: NeMo Gym pins `openai<=2.6.1` for schema compatibility. Other clients have incompatible message formats, don't integrate with the config system, and don't go through the aiohttp transport. + +**Fix**: Use `nemo_gym/openai_utils.py` for all LLM calls. + +--- + +### 8. missing-cookies + +**What**: Agent server makes `server_client.post()` calls without passing `cookies=request.cookies`. + +**Why**: Stateful environments (e.g., multi-turn proof refinement) use cookies to track session state on the resources server. Missing cookies mean the resources server can't associate requests with the correct session, causing state loss or corruption. + +**Fix**: Capture cookies from the incoming request and propagate through every downstream call: +```python +cookies = request.cookies +response = await self.server_client.post(..., cookies=cookies) +cookies = response.cookies # Update for next call +``` + +--- + +### 9. missing-token-ids + +**What**: Multi-turn agents that don't propagate `prompt_token_ids`, `generation_token_ids`, `generation_log_probs` across turns. + +**Why**: RL training requires token-level information to compute policy gradients. If multi-turn agents don't accumulate token IDs from each model call, the training framework can't attribute rewards to specific generation decisions. + +**Fix**: Extract from each model response and accumulate: +```python +all_prompt_token_ids.extend(response.get("prompt_token_ids", [])) +all_generation_token_ids.extend(response.get("generation_token_ids", [])) +all_generation_log_probs.extend(response.get("generation_log_probs", [])) +``` + +--- + +## WARN-level anti-patterns + +### 10. missing-think-strip + +**What**: Code that parses model output without stripping ``/`` blocks. + +**Why**: Thinking models (Qwen 3 Thinking, DeepSeek-R1) emit reasoning in `...` tags. If these aren't stripped, code extraction picks up code from the reasoning trace, answer extraction matches intermediate reasoning, and `reasoning_format_violation_rate` increases. + +**Fix**: Strip before parsing: +```python +if "" in text: + text = text.split("")[-1].strip() +``` + +--- + +### 11. sync-endpoint + +**What**: `/run` or `/verify` defined as `def` instead of `async def`. + +**Why**: Synchronous handlers block the FastAPI event loop. Under concurrent load, this serializes all requests. + +--- + +### 12. test-coverage + +**What**: New servers with insufficient test coverage (< 95%). + +**Required test cases**: verify pass, verify fail (wrong output), verify fail (no code/answer extracted), verify fail (compilation error if applicable), verify timeout. + +--- + +### 13. missing-skipif + +**What**: Tests requiring external tools without `pytest.mark.skipif(shutil.which("tool") is None, ...)`. + +**Why**: Tests must pass in CI environments where the tool may not be installed. If the server auto-installs the tool, add a `pytest_configure` hook in `conftest.py` to run the install before test collection — `skipif` evaluates at import time, before fixtures. + +--- + +### 14. unguarded-optional-fields + +**What**: Accessing `body.field.get("key")` without guarding against None. + +**Fix**: Use `(body.field or {}).get("key", default)`. diff --git a/.claude/skills/gym-review/references/fix-patterns.md b/.claude/skills/gym-review/references/fix-patterns.md new file mode 100644 index 0000000000..0a088d2219 --- /dev/null +++ b/.claude/skills/gym-review/references/fix-patterns.md @@ -0,0 +1,191 @@ +# NeMo Gym Fix Patterns + +Correct implementations for each anti-pattern. These are production code patterns — use them directly. + +--- + +## aiohttp-adapter + +When wrapping an external library that uses httpx internally, replace its HTTP transport with an aiohttp-compatible adapter: + +```python +from pydantic import BaseModel +from nemo_gym.server_utils import request, raise_for_status + +class AIOHTTPClientResponse(BaseModel): + """Drop-in replacement for httpx.Response.""" + status_code: int + data: dict + + def json(self): + return self.data + + +class AIOHTTPClient(BaseModel): + """Drop-in replacement for httpx.AsyncClient. + + Wraps aiohttp (via nemo_gym.server_utils.request) to avoid + httpx's O(n^2) connection pooling at high concurrency. + """ + headers: dict + base_url: str + + async def post(self, endpoint: str, content: str, timeout: float) -> AIOHTTPClientResponse: + response = await request( + method="POST", + headers=self.headers, + url=f"{self.base_url}{endpoint}", + data=content, + ) + return AIOHTTPClientResponse( + status_code=response.status, + data=await response.json(), + ) + + @classmethod + def from_httpx_client(cls, client, **kwargs): + """Convert an existing httpx.AsyncClient to this adapter.""" + return cls( + headers=dict(client.headers), + base_url=str(client.base_url), + **kwargs, + ) +``` + +Usage in `model_post_init()`: +```python +def model_post_init(self, __context): + super().model_post_init(__context) + # Replace the library's httpx client with aiohttp adapter + self.library._client = AIOHTTPClient.from_httpx_client(self.library._client) +``` + +--- + +## cookie-propagation + +Full cookie chain for a multi-turn agent: + +```python +async def run(self, request: Request, body: RunRequest) -> VerifyResponse: + cookies = request.cookies + + # Seed session + seed_resp = await self.server_client.post( + server_name=self.config.resources_server.name, + url_path="/seed_session", + json=body.model_dump(), + cookies=cookies, + ) + await raise_for_status(seed_resp) + cookies = seed_resp.cookies # Update cookies from response + + for turn in range(self.config.max_turns): + # Model call + gen_resp = await self.server_client.post( + server_name=self.config.name, + url_path="/v1/responses", + json=current_input, + cookies=cookies, # Forward cookies + ) + await raise_for_status(gen_resp) + cookies = gen_resp.cookies # Update + + # Verify call + verify_resp = await self.server_client.post( + server_name=self.config.resources_server.name, + url_path="/verify", + json=verify_data, + cookies=cookies, # Forward cookies + ) + await raise_for_status(verify_resp) + cookies = verify_resp.cookies # Update +``` + +--- + +## token-id-propagation + +Accumulate token IDs across all turns in a multi-turn agent: + +```python +all_prompt_token_ids = [] +all_generation_token_ids = [] +all_generation_log_probs = [] + +for turn in range(max_turns): + model_response = await get_response_json(gen_resp) + + # Accumulate from each model call + all_prompt_token_ids.extend(model_response.get("prompt_token_ids", [])) + all_generation_token_ids.extend(model_response.get("generation_token_ids", [])) + all_generation_log_probs.extend(model_response.get("generation_log_probs", [])) + + # ... verify, check reward, build next turn ... + +# Attach to final response +final_response.prompt_token_ids = all_prompt_token_ids +final_response.generation_token_ids = all_generation_token_ids +final_response.generation_log_probs = all_generation_log_probs +``` + +--- + +## semaphore-subprocess + +Bound concurrent subprocess execution: + +```python +class MyServer(SimpleResourcesServer): + config: MyConfig + + def model_post_init(self, __context): + super().model_post_init(__context) + self.semaphore = asyncio.Semaphore(self.config.num_processes) + + async def verify(self, body): + async with self.semaphore: + proc = await asyncio.create_subprocess_exec( + "python", "-c", code, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for( + proc.communicate(), timeout=self.config.timeout + ) + output = stdout.decode(errors="replace") + errors = stderr.decode(errors="replace") +``` + +--- + +## think-block-stripping + +Three patterns depending on context: + +**Simple strip (most common):** +```python +if "" in text: + text = text.split("")[-1].strip() +``` + +**Violation detection (for RL penalty):** +```python +def has_reasoning_format_violation(response) -> bool: + final_answer = response.output_text or "" + if "" in final_answer or "" in final_answer: + return True + # Check reasoning content for duplicate tags + reasoning = extract_reasoning_text(response) + if reasoning.count("") > 1 or reasoning.count("") > 1: + return True + return False +``` + +**Structured parsing (for multi-section output):** +```python +response = response.split("")[-1].strip() +if SOLUTION_HEADER not in response: + return None, "missing_solution_header" +proof, self_eval = response.split(SELF_EVAL_HEADER, 1) +``` diff --git a/.claude/skills/gym-review/scripts/review.py b/.claude/skills/gym-review/scripts/review.py new file mode 100644 index 0000000000..4a008beba2 --- /dev/null +++ b/.claude/skills/gym-review/scripts/review.py @@ -0,0 +1,446 @@ +#!/usr/bin/env python3 +"""Deterministic NeMo Gym anti-pattern checker. + +Scans Python and YAML files for known anti-patterns that cause production +failures in NeMo Gym's async, high-concurrency microservice architecture. + +Usage: + python review.py # Scan a directory or file + python review.py --json # Output as JSON + python review.py --severity BLOCK # Only BLOCK-level findings + +Exit codes: + 0 — no BLOCK findings + 1 — BLOCK findings present + 2 — error +""" + +import argparse +import json +import re +import sys +from dataclasses import asdict, dataclass, field +from pathlib import Path +from typing import List + + +@dataclass +class Finding: + file: str + line: int + severity: str # BLOCK or WARN + rule: str + message: str + fix: str + + +@dataclass +class ReviewResult: + findings: List[Finding] = field(default_factory=list) + files_scanned: int = 0 + ok_checks: List[str] = field(default_factory=list) + + @property + def blocks(self): + return [f for f in self.findings if f.severity == "BLOCK"] + + @property + def warns(self): + return [f for f in self.findings if f.severity == "WARN"] + + +# --------------------------------------------------------------------------- +# Rules +# --------------------------------------------------------------------------- + +def check_httpx_usage(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: httpx/httpcore imports — O(n^2) connection pooling hangs at 16k+ requests.""" + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + if re.search(r"\bimport\s+httpx\b|\bfrom\s+httpx\b|\bimport\s+httpcore\b|\bfrom\s+httpcore\b", stripped): + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="httpx-usage", + message=f"httpx/httpcore import: `{stripped.strip()}`", + fix="Use aiohttp via nemo_gym.server_utils.request(). See references/fix-patterns.md § aiohttp-adapter.", + )) + + +def check_ray_get(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: ray.get() blocks the event loop in async context.""" + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + if re.search(r"\bray\.get\s*\(", stripped): + # Check if it's inside run_in_executor (acceptable pattern) + context_start = max(0, i - 5) + context = "\n".join(lines[context_start:i]) + if "run_in_executor" in context: + continue + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="ray-get-async", + message=f"ray.get() in potentially async context: `{stripped.strip()}`", + fix="Use `result = await future` — Ray futures are directly awaitable. Or wrap in run_in_executor if synchronous context is required.", + )) + + +def check_missing_semaphore(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: subprocess calls without asyncio.Semaphore.""" + has_subprocess = False + has_semaphore = False + subprocess_line = 0 + full_text = "\n".join(lines) + + for i, line in enumerate(lines, 1): + if "create_subprocess" in line or "asyncio.subprocess" in line: + has_subprocess = True + if subprocess_line == 0: + subprocess_line = i + if "Semaphore" in line: + has_semaphore = True + + if has_subprocess and not has_semaphore: + findings.append(Finding( + file=str(path), line=subprocess_line, severity="BLOCK", rule="missing-semaphore", + message="Subprocess calls without asyncio.Semaphore for concurrency control.", + fix="Add `self.semaphore = asyncio.Semaphore(N)` in model_post_init() and wrap subprocess calls with `async with self.semaphore:`.", + )) + + +def check_decode_errors_replace(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: subprocess decode without errors='replace'.""" + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + # Match .decode() calls that don't have errors="replace" + if re.search(r"\.decode\s*\(\s*\)", stripped): + # Check surrounding context for subprocess + context_start = max(0, i - 10) + context = "\n".join(lines[context_start:i + 3]) + if "subprocess" in context or "stdout" in context or "stderr" in context or "process" in context: + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="missing-errors-replace", + message=f"Subprocess output decoded without errors='replace': `{stripped.strip()}`", + fix='Use `.decode(errors="replace")` to handle non-UTF8 output.', + )) + + +def check_env_vars(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: config via environment variables instead of YAML.""" + allowed_env_vars = {"RAY_TMPDIR", "PATH", "LD_LIBRARY_PATH", "HOME", "USER", "TMPDIR", "CUDA_VISIBLE_DEVICES"} + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + match = re.search(r'os\.(?:environ|getenv)\s*[\[\(]\s*["\'](\w+)["\']', stripped) + if match: + var_name = match.group(1) + if var_name not in allowed_env_vars: + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="env-var-config", + message=f"Config via environment variable `{var_name}`. Must use YAML config.", + fix="Pass this value through Hydra/OmegaConf YAML config. Use ${oc.env:VAR,default} only for deployment-specific infra values.", + )) + + +def check_wrong_client(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: non-Gym HTTP/LLM clients.""" + bad_imports = { + "litellm": "LiteLLM", + "anthropic": "Anthropic SDK", + } + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + for module, name in bad_imports.items(): + if re.search(rf"\bimport\s+{module}\b|\bfrom\s+{module}\b", stripped): + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="wrong-client", + message=f"{name} import: `{stripped.strip()}`", + fix="Use nemo_gym/openai_utils.py (openai<=2.6.1) for all LLM calls.", + )) + + +def check_cookie_propagation(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: multi-turn agents missing cookie propagation.""" + full_text = "\n".join(lines) + # Only check agent files + if "SimpleResponsesAPIAgent" not in full_text and "responses_api_agent" not in str(path): + return + + has_server_post = "server_client.post" in full_text + has_cookies_param = "cookies=" in full_text + + if has_server_post and not has_cookies_param: + findings.append(Finding( + file=str(path), line=1, severity="BLOCK", rule="missing-cookies", + message="Agent makes server_client.post() calls without passing cookies.", + fix="Pass `cookies=request.cookies` on every downstream call. Update cookies from each response: `cookies = response.cookies`.", + )) + + +def check_token_propagation(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: multi-turn agents missing token ID propagation.""" + full_text = "\n".join(lines) + if "SimpleResponsesAPIAgent" not in full_text and "responses_api_agent" not in str(path): + return + + # Only flag if it's a multi-turn agent (has a loop or multiple model calls) + is_multi_turn = ("while " in full_text or "for " in full_text) and "server_client.post" in full_text + if not is_multi_turn: + return + + has_token_ids = "prompt_token_ids" in full_text or "generation_token_ids" in full_text + if not has_token_ids: + findings.append(Finding( + file=str(path), line=1, severity="BLOCK", rule="missing-token-ids", + message="Multi-turn agent does not propagate token IDs (prompt_token_ids, generation_token_ids, generation_log_probs).", + fix="Extract token IDs from each model response and accumulate across turns. Include in final response for RL training.", + )) + + +def check_think_block_stripping(path: Path, lines: list[str], findings: list[Finding]): + """WARN: code parsing model output without stripping think blocks.""" + full_text = "\n".join(lines) + # Only relevant for servers that parse model output + parses_output = any(p in full_text for p in ["output_text", "extract_code", "extract_answer", "model_out"]) + strips_think = any(p in full_text for p in ["", "", "thinking", "reasoning_format"]) + + if parses_output and not strips_think: + findings.append(Finding( + file=str(path), line=1, severity="WARN", rule="missing-think-strip", + message="Parses model output but does not strip / blocks.", + fix="Strip think blocks before extraction: `text = text.split('')[-1].strip()` or check reasoning_format_violation.", + )) + + +def check_sync_endpoints(path: Path, lines: list[str], findings: list[Finding]): + """WARN: synchronous verify/run endpoints.""" + for i, line in enumerate(lines, 1): + stripped = line.strip() + # Match def verify or def run that are NOT async + if re.match(r"def\s+(verify|run)\s*\(", stripped): + # Check if async is on the same line or the previous line + prev_line = lines[i - 2].strip() if i >= 2 else "" + if "async" not in stripped and "async" not in prev_line: + findings.append(Finding( + file=str(path), line=i, severity="WARN", rule="sync-endpoint", + message=f"Synchronous endpoint: `{stripped.strip()}`", + fix="Change to `async def`.", + )) + + +def check_non_binary_rewards(path: Path, lines: list[str], findings: list[Finding]): + """BLOCK: verify returning non-binary rewards without documentation.""" + full_text = "\n".join(lines) + if "verify" not in full_text or "reward" not in full_text: + return + + # Look for reward assignments with values other than 0.0 or 1.0 + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + match = re.search(r"reward\s*[=:]\s*(-?[\d.]+)", stripped) + if match: + val = float(match.group(1)) + if val not in (0.0, 1.0): + # Check for documentation (comment on same line or previous) + prev_line = lines[i - 2].strip() if i >= 2 else "" + has_doc = "#" in stripped or "partial" in stripped.lower() or "partial" in prev_line.lower() + if not has_doc: + findings.append(Finding( + file=str(path), line=i, severity="BLOCK", rule="non-binary-reward", + message=f"Non-binary reward value: {val}. Must be 0.0 or 1.0 unless explicitly documented as intentional partial credit.", + fix="Use 0.0 or 1.0, or add a comment explaining why partial credit is intentional.", + )) + + +def check_yaml_config(path: Path, lines: list[str], findings: list[Finding]): + """Check YAML configs for common issues.""" + full_text = "\n".join(lines) + + # Check verified flag + if "verified: true" in full_text: + # Only flag if it looks like a new/unbaselined server + if "verified:" in full_text: + for i, line in enumerate(lines, 1): + if "verified: true" in line: + findings.append(Finding( + file=str(path), line=i, severity="WARN", rule="verified-true", + message="verified: true — confirm this server has been baselined with reward profiling.", + fix="Set to `verified: false` for new servers. Only set `true` after successful baselining.", + )) + + # Check for train/validation datasets missing gitlab_identifier + in_dataset = False + dataset_type = None + has_gitlab_id = False + has_license = False + dataset_start_line = 0 + + for i, line in enumerate(lines, 1): + stripped = line.strip() + if stripped.startswith("- name:"): + # Flush previous dataset + if in_dataset and dataset_type in ("train", "validation"): + if not has_gitlab_id: + findings.append(Finding( + file=str(path), line=dataset_start_line, severity="WARN", rule="missing-gitlab-id", + message=f"{dataset_type} dataset missing gitlab_identifier.", + fix="Add gitlab_identifier with dataset_name, version, and artifact_fpath.", + )) + if not has_license: + findings.append(Finding( + file=str(path), line=dataset_start_line, severity="WARN", rule="missing-license", + message=f"{dataset_type} dataset missing license field.", + fix="Add `license: ` to the dataset entry.", + )) + in_dataset = True + dataset_type = None + has_gitlab_id = False + has_license = False + dataset_start_line = i + elif in_dataset: + if "type:" in stripped: + dataset_type = stripped.split("type:")[-1].strip() + if "gitlab_identifier" in stripped: + has_gitlab_id = True + if "license:" in stripped: + has_license = True + + # Flush last dataset + if in_dataset and dataset_type in ("train", "validation"): + if not has_gitlab_id: + findings.append(Finding( + file=str(path), line=dataset_start_line, severity="WARN", rule="missing-gitlab-id", + message=f"{dataset_type} dataset missing gitlab_identifier.", + fix="Add gitlab_identifier with dataset_name, version, and artifact_fpath.", + )) + if not has_license: + findings.append(Finding( + file=str(path), line=dataset_start_line, severity="WARN", rule="missing-license", + message=f"{dataset_type} dataset missing license field.", + fix="Add `license: ` to the dataset entry.", + )) + + +# --------------------------------------------------------------------------- +# Runner +# --------------------------------------------------------------------------- + +PY_CHECKS = [ + check_httpx_usage, + check_ray_get, + check_missing_semaphore, + check_decode_errors_replace, + check_env_vars, + check_wrong_client, + check_cookie_propagation, + check_token_propagation, + check_think_block_stripping, + check_sync_endpoints, + check_non_binary_rewards, +] + +YAML_CHECKS = [ + check_yaml_config, +] + + +def scan_file(path: Path, result: ReviewResult): + try: + text = path.read_text(encoding="utf-8", errors="replace") + except Exception: + return + lines = text.splitlines() + result.files_scanned += 1 + + if path.suffix == ".py": + for check in PY_CHECKS: + check(path, lines, result.findings) + elif path.suffix in (".yaml", ".yml"): + for check in YAML_CHECKS: + check(path, lines, result.findings) + + +def scan_path(target: Path, result: ReviewResult): + if target.is_file(): + scan_file(target, result) + elif target.is_dir(): + for ext in ("*.py", "*.yaml", "*.yml"): + for f in sorted(target.rglob(ext)): + # Skip common non-source dirs + if any(p in f.parts for p in ("__pycache__", ".venv", "node_modules", ".git")): + continue + scan_file(f, result) + + +def format_text(result: ReviewResult) -> str: + lines = [] + lines.append(f"Scanned {result.files_scanned} files\n") + + if not result.findings: + lines.append("No issues found.\n") + return "\n".join(lines) + + blocks = result.blocks + warns = result.warns + + if blocks: + lines.append(f"### BLOCK ({len(blocks)})\n") + for f in blocks: + lines.append(f"- `{f.file}:{f.line}` [{f.rule}] — {f.message}") + lines.append(f" Fix: {f.fix}\n") + + if warns: + lines.append(f"### WARN ({len(warns)})\n") + for f in warns: + lines.append(f"- `{f.file}:{f.line}` [{f.rule}] — {f.message}") + lines.append(f" Fix: {f.fix}\n") + + lines.append(f"\nSummary: {len(blocks)} BLOCK, {len(warns)} WARN") + return "\n".join(lines) + + +def main(): + parser = argparse.ArgumentParser(description="NeMo Gym anti-pattern reviewer") + parser.add_argument("path", help="File or directory to scan") + parser.add_argument("--json", action="store_true", help="Output as JSON") + parser.add_argument("--severity", choices=["BLOCK", "WARN"], help="Filter by severity") + args = parser.parse_args() + + target = Path(args.path) + if not target.exists(): + print(f"Error: {target} does not exist", file=sys.stderr) + sys.exit(2) + + result = ReviewResult() + scan_path(target, result) + + if args.severity: + result.findings = [f for f in result.findings if f.severity == args.severity] + + if args.json: + output = { + "files_scanned": result.files_scanned, + "findings": [asdict(f) for f in result.findings], + "summary": { + "block": len(result.blocks), + "warn": len(result.warns), + "total": len(result.findings), + }, + } + print(json.dumps(output, indent=2)) + else: + print(format_text(result)) + + sys.exit(1 if result.blocks else 0) + + +if __name__ == "__main__": + main() diff --git a/.claude/skills/gym-scaffold-agent/SKILL.md b/.claude/skills/gym-scaffold-agent/SKILL.md new file mode 100644 index 0000000000..c55a5998a5 --- /dev/null +++ b/.claude/skills/gym-scaffold-agent/SKILL.md @@ -0,0 +1,170 @@ +--- +name: gym-scaffold-agent +description: > + Create a custom agent server for NeMo Gym. Use when the default simple_agent is + insufficient — for multi-turn interaction, external library wrapping, custom tool + orchestration, or non-standard interaction patterns (model assimilation). Covers + agent server scaffolding, cookie/token propagation, httpx replacement, and async + patterns for high-concurrency operation. +license: Apache-2.0 +compatibility: Requires Python 3.12+, NeMo Gym installed. +metadata: + author: nvidia-nemo-gym + version: "1.0" +allowed-tools: Bash(python:*) Bash(ng_*) Bash(git:*) Read Write Edit Grep Glob +--- + +# Scaffold a Custom Agent Server + +## When you need a custom agent + +The built-in agents cover most cases: +- **`simple_agent`** — single-turn: sends prompt to model, gets response, calls verify. Works for most benchmarks. +- **`proof_refinement_agent`** — multi-turn correction: model gets error feedback and retries. + +Build a custom agent when: +- The interaction pattern doesn't fit single-turn or simple correction loops +- You're wrapping an external library that has its own orchestration +- The benchmark requires custom tool-call sequencing or state management +- You need to teach the model a specific interaction protocol (assimilation) + +## Step 1: Create the directory + +``` +responses_api_agents/my_agent/ +├── app.py # Server class extending SimpleResponsesAPIAgent +├── configs/my_agent.yaml +├── tests/__init__.py +├── tests/test_app.py +└── requirements.txt # just: -e nemo-gym[dev] @ ../../ +``` + +## Step 2: Implement the agent + +Your agent extends `SimpleResponsesAPIAgent` and implements `responses()` and `run()`. + +```python +from nemo_gym.server import SimpleResponsesAPIAgent + +class MyAgent(SimpleResponsesAPIAgent): + async def responses(self, request): + # Single response from model + ... + + async def run(self, request): + # Full orchestration loop + ... +``` + +### The `/run` endpoint + +This is where orchestration happens. The general pattern: + +1. Receive the input (system prompt + user message + verifier_metadata) +2. Call the model server (`/v1/responses` or `/v1/chat/completions`) +3. If the model returns tool calls, execute them against the resources server +4. Optionally loop (multi-turn) +5. Call `/verify` on the resources server +6. Return the verify response (includes reward) + +The `/run` endpoint **must be async**. + +## Step 3: Cookie propagation (critical for stateful environments) + +Every downstream request must forward cookies from the incoming request: + +```python +async def run(self, request): + cookies = request.cookies # Capture from incoming request + + # Every downstream call passes cookies + model_response = await self.server_client.post( + model_url, json=payload, cookies=cookies + ) + verify_response = await self.server_client.post( + verify_url, json=payload, cookies=cookies + ) +``` + +Missing cookies break stateful environments where the resources server tracks session state. + +## Step 4: Token ID propagation (critical for RL training) + +Multi-turn agents must propagate token IDs from model responses into subsequent turns: + +```python +# After receiving model response +prompt_token_ids = model_response.get("prompt_token_ids", []) +generation_token_ids = model_response.get("generation_token_ids", []) +generation_log_probs = model_response.get("generation_log_probs", []) + +# Accumulate across turns and include in final response +``` + +Without these, the RL training framework can't compute policy gradients for multi-turn interactions. + +## Step 5: Wrapping external libraries + +When integrating a 3rd-party benchmark library: + +1. **Replace httpx transport**: If the library uses httpx internally, replace its HTTP transport with an aiohttp adapter. See `resources_servers/tavily_search/app.py` (`TavilySearchAIOHTTPClient`) for the pattern. + +2. **Pre-process input**: Convert from Gym schema (`responses_create_params.input` + `verifier_metadata`) to the library's expected input format. + +3. **Post-process output**: Convert the library's results back to `BaseVerifyResponse` (must include `reward` field). + +4. **Reproduce published numbers**: Run the original library standalone first and record scores. Then run through your Gym wrapper and verify scores match. + +```python +async def run(self, request): + # Pre-process: Gym schema -> library input + lib_input = self.convert_to_library_format(request) + + # Run library (may need asyncio.Semaphore for concurrency control) + async with self.semaphore: + lib_result = await self.run_library(lib_input) + + # Post-process: library output -> Gym response + return self.convert_to_gym_response(lib_result) +``` + +## Step 6: Concurrency control + +The agent must handle 4k-65k concurrent requests. Use `asyncio.Semaphore` for any blocking or resource-intensive operations: + +```python +class MyAgent(SimpleResponsesAPIAgent): + def model_post_init(self, __context): + super().model_post_init(__context) + self.semaphore = asyncio.Semaphore(self.max_concurrent) +``` + +## Step 7: Wire YAML config + +```yaml +my_agent_instance: + responses_api_agents: + my_agent: + entrypoint: app.py + resources_server: + type: resources_servers + name: my_resources_server # Must match resources server instance name + model_server: + type: responses_api_models + name: policy_model # Must match model server instance name + datasets: + - name: example + type: example + jsonl_fpath: resources_servers/my_benchmark/data/example.jsonl +``` + +## Step 8: Test + +Write tests covering: +- Happy path (model produces correct output, gets reward 1.0) +- Model failure (bad output, gets reward 0.0) +- Multi-turn logic (if applicable — verify correct number of turns, proper accumulation) +- Cookie propagation (verify cookies are forwarded) +- Concurrency (verify semaphore bounds are respected) + +Coverage must be >= 95%. diff --git a/.claude/skills/gym-scaffold-agent/evals/evals.json b/.claude/skills/gym-scaffold-agent/evals/evals.json new file mode 100644 index 0000000000..54439ebf55 --- /dev/null +++ b/.claude/skills/gym-scaffold-agent/evals/evals.json @@ -0,0 +1,46 @@ +{ + "skill_name": "gym-scaffold-agent", + "evals": [ + { + "id": 1, + "prompt": "Create a multi-turn agent that gives the model 3 attempts to solve a math problem. After each wrong attempt, it sends the error back to the model for correction.", + "expected_output": "A custom agent server with a retry loop, proper cookie and token ID propagation, async /run endpoint, and YAML config.", + "assertions": [ + "The agent extends SimpleResponsesAPIAgent", + "The run() method implements a loop with max 3 iterations", + "Each iteration calls the model server then the resources server /verify", + "Error feedback from verify is sent back to the model on failure", + "cookies=request.cookies is passed on every downstream call", + "Token IDs (prompt_token_ids, generation_token_ids, generation_log_probs) are accumulated across turns", + "The /run endpoint is async", + "asyncio.Semaphore is used for concurrency control" + ] + }, + { + "id": 2, + "prompt": "Wrap the SWE-bench library as an agent in NeMo Gym. The library has its own task execution logic and uses httpx for API calls.", + "expected_output": "An agent server that wraps SWE-bench, replaces httpx transport with aiohttp, pre/post-processes between Gym and library schemas.", + "assertions": [ + "httpx is NOT imported — an aiohttp adapter replaces the transport", + "Pre-processing converts from Gym schema to SWE-bench input", + "Post-processing converts SWE-bench results to BaseVerifyResponse with reward field", + "The approach notes to reproduce published SWE-bench numbers first", + "asyncio.Semaphore bounds concurrent library calls", + "requirements.txt includes the SWE-bench dependency" + ] + }, + { + "id": 3, + "prompt": "I need an agent that handles tool calls in a loop — the model can call tools multiple times before giving a final answer.", + "expected_output": "An agent with a tool-call loop that executes tool calls against the resources server and feeds results back to the model until it produces a final non-tool-call response.", + "assertions": [ + "The agent implements a loop that continues while the model returns tool calls", + "Tool call results are sent back to the model as tool response messages", + "The loop has a maximum iteration bound to prevent infinite loops", + "Cookie propagation is present on all downstream calls", + "Token IDs are accumulated across all turns in the loop", + "The final response is passed to /verify for reward computation" + ] + } + ] +}