Repository navigation
fix(pinchbench): startup validation, non-clean exit handling, FEP-1322 image check - #2440
Merged
Merged
Conversation
Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
- Reject unknown web_search_provider values and provider/key mismatches at startup - Validate sandbox_work_base is absolute and max_tokens <= context_window at startup - Call get_provider_class eagerly in model_post_init to catch provider typos before first task - Log warning and record non_clean_exit_rc in raw_rollout when apptainer exits non-zero but archive exists - Warn when multiple result JSON files are found in _parse_result Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
- Trim module docstring; remove PR refs, scores, and version-specific prose - Strip comments that restate field names, section dividers, and duplicate rationale - Import NG_* constants from nemo_gym.rollout_collection instead of redefining - web_search_provider: str → Literal["brave", "tavily"]; drop manual membership check - Extract _SEARCH_KEY_MAP module-level constant; rename validator _validate_api_key_present - Move get_provider_class validation into _validate_sandbox_config (config-level, not agent init) - Remove dead absolute-path guard in _run_in_apptainer_direct (covered by config validator) - Simplify non_clean_exit_rc to ternary return; build raw_rollout dict with explicit if - Extract tarfile from original archive instead of the just-copied duplicate Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
… exit Covers three seams: - PinchBenchAgentConfig: web_search_provider/key mismatch, unknown provider, relative sandbox_work_base, max_tokens > context_window - _parse_result: warning logged when multiple result JSON files present - _run_in_apptainer_direct: non-clean exit returns rc without raising; clean exit returns None with no warning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
- Remove ghost assertion for PINCHBENCH_FORCE_LOCAL (key never existed in app.py) - Strengthen OPENCLAW_GATEWAY_TOKEN check from truthiness to exact value - Split compound `and` assertions in parse_result tests - Parametrize three identical-structure routing sentinel tests into one - Parametrize test_classify_task_failure_mapping into per-case tests - Narrow contextlib.suppress(Exception) to suppress(RuntimeError) in sandbox isolation helper - Drop tautological openclaw_mode == "gateway" assertion (field is Literal["gateway"]) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
- _task_env: tavily key injection, brave key omitted when None, provider timeout conditional - run(): missing task_id raises ValueError; non_clean_exit_rc flows into raw_rollout - _run_in_apptainer_direct: signal-kill detection (rc=-15, 137, 143) raises SandboxKilledError - Transcript parsing: event dedup by id, toolResult details fallback, malformed JSON toleration - _tool_call_arguments: dict branch and absent-arguments branch - _collect_transcript: empty return when transcript dir absent - _parse_result: empty runs list defaults grading_type to unknown - Sandbox isolation: direct_exec_args as string is split into argv Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
…(FEP-1322) A missing .sif file previously only surfaced as a per-task RuntimeError inside the sandbox, causing every task to score 0 with no startup signal. With chained Slurm jobs this silently burned GPU-hours across the entire chain. Add an existence check in _validate_sandbox_config: if sandbox_spec.image is a local path (no :// URI scheme), verify it exists at server startup and raise a clear ValueError if not. URI references (docker://, apptainer://, etc.) skip the check since they are resolved remotely. Update tests to use docker://test for subprocess-mocking tests that don't need a real file, and tmp_path for tests that assert on the image path itself. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
laszkiewiczp
force-pushed
the
plaszkiewicz/pinchbench-fixes
branch
3 times, most recently
from
August 10, 2026 11:32
2cce958 to
e0fd5e2
Compare
laszkiewiczp
force-pushed
the
plaszkiewicz/pinchbench-fixes
branch
from
August 10, 2026 11:35
e0fd5e2 to
72ca335
Compare
tvly-* strings pattern-matched the secrets detector. Replaced with test-tavily-key throughout. Also applied ruff-format to the two files the CI formatter flagged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
laszkiewiczp
force-pushed
the
plaszkiewicz/pinchbench-fixes
branch
from
August 10, 2026 11:40
72ca335 to
96fe458
Compare
Contributor
Author
|
/ok to test 96fe458 |
marta-sd
reviewed
Aug 10, 2026
next(iter(sandbox_provider)) silently picks the first key and ignores any others, so a two-entry dict with one valid and one typo'd provider would pass startup validation. Add an explicit length check so the validator catches this before the first task runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: plaszkiewicz <plaszkiewicz@nvidia.com>
marta-sd
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds startup validation to
PinchBenchAgentConfigand hardens the agent's error handling and test coverage.Changes
app.pyweb_search_provideris nowLiteral["brave", "tavily"]— invalid values rejected at startup_validate_api_key_present)sandbox_work_baseis an absolute path at startup (was only checked inside_run_in_apptainer_direct)max_tokens <= context_windowsandbox_providername is known viaget_provider_class(catches typos before the first task)sandbox_spec.imageexists on disk when it is a local path — fails fast at startup instead of silently scoring 0 across every task in a chained Slurm jobnon_clean_exit_rcinraw_rolloutinstead of silently continuing_parse_result: logs a warning when multiple result JSON files are foundNG_*constants fromnemo_gym.rollout_collectioninstead of redefining thembenchmarks/pinchbench/config.yamlTests (
test_app.py,test_sandbox_isolation.py,test_validation.py— new file)_classify_task_failurePINCHBENCH_FORCE_LOCAL, strengthenedOPENCLAW_GATEWAY_TOKENchecknon_clean_exit_rcend-to-end, missingtask_id, optional env var injection,toolResultdetails fallback, transcript dedup, malformed JSON toleration, image existence check