Repository navigation
Add critpt_custom_grader: a sandboxed, locally executed grader for CritPt-style problems - #3430
willtong-aq wants to merge 3 commits into
Conversation
ff4d14e to
6147b4d
Compare
…d core
Rebuild resources_servers/critpt_custom_grader so the change touches only that
directory and one README index row, on top of current main. Drop every prior
edit to shared core (nemo_gym, responses_api_agents/simple_agent,
responses_api_models/openai_model, tests). The grader now runs against
unmodified core.
Dropped shared symbols, replaced by grader-local equivalents:
- nemo_gym.server_utils.{ServerRequestPolicy, ServerRequestPrivacy,
private_error_response} -> server_support.py. The two config models are
declarative operator intent. The grader enforces its own request privacy at
its HTTP boundary through its private route class, its private ingress, and
its overridden telemetry and exception hooks, which unmodified core still
calls.
- DaytonaProvider(category_only_diagnostics=...) and the missing-exit-code
guard -> provider_support.py. A local CategoryOnlyDaytonaProvider subclass
reads a missing or non-integer exit code as a typed sandbox failure, never as
success, and keeps the provider's own exec text and log lines to fixed
categories. Only _exec is overridden.
Config: the agent block points at the stock responses_api_agents/simple_agent
and carries no private-mode keys. The launch command drops the two model-server
overrides that unmodified core ignores. The grader server block keeps its
request-policy and request-privacy config, backed by the local models.
Tests: rewrote the two tests that exercised dropped features, added tests for
the local provider subclass and the local config models, kept every other test.
ng_test passes and validates the example data.
Grading semantics are unchanged. The comparator, codec, symbolic, task_data,
and runner modules are the lean commit's, byte for byte.
Review fixes, grader directory only:
- comparator.py: a test pins the unordered_undecided verdict, where a one-to-one
matching exists over undecided member pairs but not over certain ones.
- symbolic.py: the polynomial fast path is guarded, so an expression the
is_polynomial or Poly check cannot handle becomes the numeric fallback instead
of an unhandled exception.
- app.py: a latched-gate verify shares one queue-wait budget across its reconcile
and its execute, so it cannot wait the full queue timeout twice. The single-job
limit is unchanged.
Astra review fixes, grader directory only:
- symbolic.py (finding 1): an undecided difference that carries a branch
function (Abs, sign, floor, ceiling, Heaviside, Piecewise, Max, Min, re, im,
arg) returns None instead of accepting on fixed samples, since the samples can
all fall on one side of the branch point. Non-branch expressions keep the
sampling path. This guard sits after the polynomial guard above.
- comparator.py (finding 2): the accuracy kind read (abs/rel) is bounded to the
keyword's own sentence, the same bounds name attribution uses, and each number
is scored under its nearest keyword, so a number a wide window pulls across a
sentence break is not double-counted under two kinds.
- comparator.py (finding 4): a cross-pair that raises RepresentationError in the
unordered match is treated as no match, so a valid one-to-one matching still
passes.
- runner.py (finding 5): materialize_input has a SetValue branch that returns a
Python set of materialized members, so task code that uses the set API works.
- comparator.py (finding 6): a symbolic-carrier expectation of oo or zoo is
rejected as nonfinite_expected, matching the README rule. A plain categorical
string is unaffected.
- README.md (findings 9, 10): an emptied harvested promise reads as the strict
floor where no author spoke; a waiter completes only while its cumulative wait
stays within queue_timeout_s, not for as many jobs as the queue holds.
Astra review, second batch, grader directory only:
- app.py: a response that ends on an assistant message whose own status is not
completed now needs regeneration, through a _final_message_incomplete helper
feeding _needs_regeneration. An incomplete final message inside a completed
envelope is no longer read as a finished answer.
- execution.py: shutdown scans the journal's unresolved records the way reconcile
does, so a disk-recovered record whose first delete failed gets another attempt
at shutdown instead of waiting for the next restart. A corrupt record is
reported as corrupt_record, never resolved.
Signed-off-by: William <william@afterquery.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H1ap1cmdJDPbW1dj3KAovc
Rename the failure classes to follow nemo_gym/failure_kinds.py: verifier_unavailable -> provider_unavailable, reference_failed -> verifier_error, needs_regeneration -> critpt_custom_grader:response_incomplete. The first two are registered core names that execution.py already produced, so app.py no longer translates them. failure_kind now carries the same value as _ng_failure_class. Routing, retry, and terminal behavior do not change. Correct the README recovery section. Core has no recover_failure_classes option, and --judge-failed-only selects only judge_failed rows. Select provider_unavailable rows from the failures file with jq, then run plain gym eval reverify on them. Document the reverify model overrides and the aggregate quoting that a live run showed are required. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCKgzMtDMkANTG7Xu9W1ef Signed-off-by: William <william@afterquery.com>
428a35d to
602b9cf
Compare
yuhezhang-ai
left a comment
There was a problem hiding this comment.
Thanks for adding this grading option. Being able to run CritPt with operator-supplied expectations and individual task scores would be useful. I reproduced several issues in the supplied Gym integration and answer comparison that should be addressed before merging; the inline comments include concrete examples and suggested fixes. The comments on masking unscorable responses and preserving error categories are smaller, non-blocking improvements.
Could you add regression coverage for these cases and validate the corrected configuration with a representative end-to-end model rollout on Daytona? My validation covered local tests and focused reproductions; live execution isolation still needs review by someone familiar with the sandbox runtime.
| # job at a time and re-grades an idempotent request without any double-scoring. Cache resume | ||
| # may redispatch a nonterminal row. Recovery must not regenerate a saved candidate. | ||
|
|
||
| critpt_custom_grader_simple_agent: |
There was a problem hiding this comment.
[P1] Add the required environment-server routing block
Please add the environment-server block that routes collection to this agent. With all four execution inputs supplied and a dummy model composed, the shipped YAML raises AgentWithoutEnvironmentServerError in GlobalConfigDictParser.parse(..., offline=True). Adding the usual legacy_agent environment server pointing to critpt_custom_grader_simple_agent makes the same parse succeed. The documented setup therefore cannot load on this revision. Please exercise the actual config parser in a regression test.
| def public_params(cls, value: dict) -> dict: | ||
| bounded_json(value, max_bytes=MAX_RESPONSE_BYTES) | ||
| NeMoGymResponseCreateParamsNonStreaming.model_validate(value) | ||
| if value.get("tools") or value.get("tool_choice") not in (None, "none"): |
There was a problem hiding this comment.
[P1] Accept the stock agent default when no tools are configured
Please allow tool_choice=auto when the tools list is empty. SimpleAgentRunRequest.model_validate(first_example_row).model_dump() fills this default, and simple_agent sends that full dump to seed_session and verify. This validator rejects it with tools are not supported, even though no tool can be called. Explicit none works, but the committed examples omit the field. Please test a request serialized by the real agent model so the default workflow is covered.
|
|
||
| def _task(body: CritPtRunRequest) -> TaskData: | ||
| row = body.model_dump(exclude={"response", "capture_rollout_id"}) | ||
| fields, conflicts = normalize_task_fields(row) |
There was a problem hiding this comment.
[P1] Exclude framework bookkeeping from task validation
Please strip framework bookkeeping before validating the task schema. normalize_task_fields does not remove _ng_attempt_index or _ng_environment_server at this base, and TaskData rejects extras. Adding either key to an otherwise valid committed example reproduces extra_forbidden in _task. Collection stamps the attempt key on resumed rows and the environment key for legacy routing, so those requests fail before grading. Preserve strict rejection of task-data typos while excluding framework-owned keys, and add resumed-request coverage.
| if bool(abs(left_value - right_value) > rtol_value * max(abs(left_value), abs(right_value)) + atol_value): | ||
| return False | ||
| decided += 1 | ||
| return True if decided >= required else None |
There was a problem hiding this comment.
[P1] Do not certify symbolic equality from fixed samples
Please keep unproven symbolic equality undecided instead of accepting agreement at fixed real samples. Production compare_request returns equal for x + sin(840pix) versus x, although the difference is exactly 1 at x=1/1680. It also accepts conjugate(x) versus assumption-free x, which differ at x=i. Both were reproduced with SymPy 1.14.0. Equal verdicts can become reward 1. Changing the fixed points or expanding the branch blacklist alone does not address the root problem; require an appropriate equivalence proof and add these regressions.
| and name.lower() not in _PROMISE_VOCABULARY | ||
| and not _PROMISE_KEYWORD.fullmatch(name) | ||
| ) | ||
| blanket = max(honoured, key=lambda clause: clause["value"]) if honoured else None |
There was a problem hiding this comment.
[P1] Keep unrelated percentages out of answer tolerances
Please avoid promoting an unrelated numerical statement to the output tolerance. With 'The trap frequency is stable to within 2% during the measurement. Report the ground-state energy with relative error below 1e-6.', expected 1, candidate 1.015, and authored rtol=1e-9, compare_request returns equal because the 2% clause becomes the blanket. Removing the first sentence makes the same answer mismatch. Only an accuracy requirement tied to the output should override its tolerance; ambiguous or incidental quantities must not widen it. Please add this mixed-prose regression.
|
|
||
| def _expression_shaped(text: str) -> bool: | ||
| stripped = text.strip() | ||
| return stripped.lstrip("+-") in _MATH_ATOMS or any(char in stripped for char in "*/()^+") or " - " in stripped |
There was a problem hiding this comment.
[P2] Recognize simple mathematical legacy answers
Please handle mathematical legacy outputs such as -x, x, and unspaced x-1 consistently with their symbolic reference values. The current classifier leaves those legacy expectations as categorical strings, while the same SymPy output is parsed as an expression. Production reference-role comparisons for all three return invalid_reference, stopping a valid task before candidate execution. Preserve or correctly recognize the expected mathematical kind, retain explicit categorical behavior, and add reference-preflight regressions.
| response=body.response, | ||
| problem_id=task.problem_id if task else None, | ||
| reward=reward, | ||
| scored=scored, |
There was a problem hiding this comment.
[P2] Mask unscorable responses for shared metric consumers (non-blocking)
Please set the inherited mask_sample flag for unscorable responses, consistently with scored=False. comparator_uncertain currently returns reward=0 and mask_sample=False; Gym's shared select_measured therefore retains it as a measurement. One success plus this unmeasured attempt implies mean reward 0.5 in that consumer. Normal collection's failure routing and this server's custom aggregation already filter it correctly, so this is a standard-consumer interoperability fix, not a claim that normal collection is corrupted.
| and result.terminal is True | ||
| and result.category in {"task_invalid", "reference_mismatch"} | ||
| ): | ||
| return self._response(body, task, "reference_invalid", terminal=True, cleanup=cleanup) |
There was a problem hiding this comment.
[P3] Preserve the invalid-task error category (non-blocking)
Please retain task_invalid when the controller reports an invalid expected value or comparison policy, and map only reference_mismatch to reference_invalid. A task_invalid GradeResult currently projects to reference_invalid here, which directs operators to the reference solution when the defect is in the stored expectation or policy. This can be corrected without changing reward or retry behavior.
|
Thank you for this grader, and for the careful review it received. Its grading pipeline (reference preflight, candidate run, comparator) and its grading core carry over into the replacement, and this PR's commits keep their authorship there. The replacement, #4629, takes that pipeline onto Gym's provider-neutral sandbox API, so the grader runs on any Gym sandbox provider without a backend of its own, and it addresses every finding of the review here, each with a regression test. Could a maintainer close this PR in its favour? Thanks again. |
Closes #3342. This PR implements the proposal in that issue.
What this adds
resources_servers/critpt_custom_grader: a resources server that grades a candidate response for a CritPt-style problem. It executes the reference preflight and the candidate code inside a Daytona sandbox. It then compares the results against stored expectations. It is added besideresources_servers/critpt, which stays unchanged. It does not call the Artificial Analysis grading service and it does not claim parity with the hidden CritPt grader.How it works
/verifyreceives an already generated response. The grader never generates candidates.eval, no unrestrictedsympify.provider_unavailable) is recovered by re-verifying the saved candidate without a new model call. A lost outer/runresponse is a documented at-least-once limitation (see README)._ng_failure_classandfailure_kindto one of three names.provider_unavailableandverifier_errorcome from core'sFAILURE_KINDSregistry.critpt_custom_grader:response_incompleteuses the server prefix that the registry asks for.Execution limits
The README documents how the derived bounds follow from the suite limit.
Core changes
None. The grader is self-contained against unmodified core. Two small local modules replace what earlier drafts patched into core: a Daytona provider subclass that reads a missing or non-integer exit code as a sandbox failure and keeps its diagnostics category-only, and a module that holds the grader's request policy and privacy settings and its private error responses.
Tests and evidence
NG_CRITPT_RUNNER_REMOTE=1.uv.lockpins. One pre-existing failure (test_slurm_launcher_submits_one_dependent_cpu_cleanup_job) fails the same way on the base commit.data/example_rollouts.jsonl, collected throughsimple_agent.example_metrics.jsonreports 5 examples.Public set: the 70 answer rows, 73 alternates on their own test cases, 73 alternates against the primary test cases, and 70 canaries.
Private set, reward agreement with the historical grader: above 99.7% of rows. The 20 other rows are triaged in the acceptance report:
Re-runs on the frozen build: the 7 candidate timeouts reproduced at 1823 to 1844 s. Of the 4 rows above, 3 reproduced. 1 submission passed on the first pass and timed out at the 1800 s suite limit on the second pass. Its verdict is timing-bound.
A further 54 rows differ only in the rejection label. Both graders give reward 0.
Public 70: all 70 primary solutions pass. All 70 canaries reject. 71 of 73 alternates pass on their own test cases. The 2 others are reference faults (public set question #2, alternate 0, and public set question #62, alternate 0). 71 of 73 alternates reject against the primary test cases. Public set question #68, alternate 0, passes and public set question #12, alternate 0, is uncertain.
Review fixes
Two independent model reviews were run and each finding was verified against the code before a fix. Fixed: the symbolic sampling fallback no longer accepts two expressions that differ by a branch function, an accuracy kind (absolute or relative) no longer bleeds across a sentence break, an unparseable cross pair no longer defeats a valid unordered matching, set inputs reach task code as Python sets, a symbolic infinity expectation is unscorable, a polynomial check that raises is treated as "not a polynomial", so the comparison goes on to the branch-function guard and then the bounded numeric fallback, a queue wait is bounded once, shutdown retries journal records recovered from disk, and a completed envelope with an incomplete final message is flagged for regeneration instead of scored zero.
Update 2026-09-23
main. The only conflict was the rootREADME.mdtable, where theDeepsearchqarow landed next to this one. Both rows are kept.nemo_gym/failure_kinds.py:verifier_unavailable→provider_unavailable,reference_failed→verifier_error,needs_regeneration→critpt_custom_grader:response_incomplete.failure_kindcarries the same value. Routing, retry, and terminal behavior do not change.recover_failure_classesoption that core does not have. Core's--judge-failed-onlyselects onlyjudge_failedrows. The README now selectsprovider_unavailablerows from the failures file withjqand runs plaingym eval reverifyon them, with no core change. The README also documents thepolicy_*overrides thatgym eval reverifyneeds (it has no--modelflag) and the quoting thatgym eval aggregateneeds for a comma list of files.Scope notes
SandboxBackendseam so other providers can follow.