Repository navigation
Conversation
Documentation Changes Detected📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Drafted by Codex.
Reviewed commit 4614f9ffa32b70cd7048cb0a8c271d29ab1c0f6e.
No blocking findings or significant correctness regressions found.
Reviewed the exact base-to-head diff, source adaptation, rubric/target binding validation, shared grading gates, judge failure propagation, and effects on existing judge profiles. No previous Codex change requests required reassessment.
Validation: all changed Python files compile; standalone checks of the head’s parser, reward tiers, empty-reply handling, and pinned rubric hash passed. Diff whitespace checks passed. Pytest and Ruff are unavailable in this sandbox. Live judge calibration and GPU training remain unverified, as disclosed in the PR; no GPU validation run was claimed.
Documentation Changes Detected📄
|
There was a problem hiding this comment.
Drafted by Claude.
Reviewed commit 4614f9ffa32b70cd7048cb0a8c271d29ab1c0f6e.
Merge recommendation: Ready to merge.
This is a well-scoped port. The adapter, profile/target binding checks, the explicit-IDK shortcut, and the docs fit the existing named-judge design. I found no blocking correctness or security problem. The first comment below changes behaviour in a way the author probably didn't intend, so I'd recommend addressing it before or soon after merge.
1. Forcing policy = "raise" makes evaluation fail on a judge outage (worth fixing)
open_instruct/miles/rewards/general_judge.py:326-328
The comment says the goal is that a failed judge is "never recorded as an incorrect answer". The existing pipeline already handles that without the override:
- Training (
rewards.py:658,omit_failures=True): under the defaultzeropolicy,general_judge_scorereturns0.0withstatus="judge_error"._scoresees that status atrewards.py:570-574and raises, so the response or group is left out. It is never trained on as a 0. With the override, the raise happens earlier, but the result is the same omission. - Evaluation (
rewards.py:658,omit_failures=False, failure_policy="zero"): on base, a named judge that fails after three retries returns ajudge_errorzero. The eval run logs it and continues; the diagnostics show it was a judge failure, not a grade. With this PR, the abstention judge raises instead._scorere-raises atrewards.py:593-594, so one transient judge outage during periodic eval stops the whole run. Other named judges in the same mixture don't behave this way. registered_reward(..., failure_policy="zero")callers are affected the same way.
Possible fix: drop the override and let _score's judge_error handling keep failures ungraded, as it does for reference_judge. If you want abstention to be stricter than the global setting, consider making it explicit only for training, for example a per-rubric failure policy. If crashing eval is intended, say so in docs/miles/abstention.md. Right now the doc only says failures "remain errors".
Related nit: the failure-diagnostics block at general_judge.py:355-367 still records "kind": "general_judge" for abstention failures. Success paths are relabelled to abstention.KIND (lines 403-406). Setting the kind on failure too would make it easier to filter abstention failures in run records.
2. The probe's third check never contacts the judge (optional)
open_instruct/miles/rewards/judge_registry.py:121-142
\boxed{[IDK]} takes the is_abstention shortcut, so scores[2] == 0.5 is true no matter what the judge does. So the check against [1.0, 0.0, 0.5] really only tests the A and B tiers. It never shows that the selected judge returns C (NOT_ATTEMPTED) for a hedged, unboxed abstention, and that is the case most likely to be miscalibrated. Consider adding a fourth control such as "I'm not sure; I don't know the capital of France." and expecting 0.5 with judge_called=True. The doc's wording, "explicit-abstention controls", is accurate today, but a reader could take it to mean the judge's C path is covered.
3. Adapter robustness (minor)
open_instruct/miles/datasets/source_preparation.py:363,365
raw.get("responses_create_params", {}).get("tools")raisesAttributeErrorif the key is present butnull.prepare()only catchesValueError/KeyError/TypeError/SchemaError(line 428), so one such row would abort preparation instead of being omitted.messages()already usesraw.get("responses_create_params") or {}(line 84); the same pattern would work here.raw.get("domain", "HotpotQA")puts any row without a domain into the HotpotQA stratum, which would quietly skew per-domain counts. The docs call for qualification "across all four domains". Requiringdomain, or falling back to something like"unknown", would keep the strata honest.
4. Small clarity nits (optional)
abstention.build_prompt(abstention.py:325-331) hashes the template twice. Comparing againstRUBRIC_SHA256once and returning that constant would be simpler. The drift check only catches in-process changes; an edit to the file has to updateRUBRIC_SHA256anyway. That is fine as a guard, but a one-line comment saying so would help.- In
resolve_request,extract_answeris silently ignored for the abstention profile. That is correct, since the profile does its own think-stripping and box extraction. A short comment would stop a later reader from "fixing" it. Note that with final-answer grading on,rewards.py:530-532already passes the response-only view, so stripping thinking again is harmless. test_miles_abstention.py:1mentions "C033", which looks like an internal tracker ID with no meaning in the repo.
What I checked
- Read the full diff.
- Traced
general_judge_scorethroughrewards._score,rollout_rewardandregistered_rewardto compare the failure-policy behaviour on base and head. - Checked that
NamedJudgeVerifierhas noscore_floor, so gated or truncated responses get 0 and never 0.5, as the doc says. - Confirmed
judging.PROFILESis only consumed injudge_registry, so the new non-open-instruct/profile doesn't affect other code paths. - Confirmed
validate_datarejects a dict target under a generic rubric and a string target under the abstention rubric.
Verification limits
This was a static review; I ran no tests. I didn't compare the ported rubric or parser byte-for-byte against NeMo Gym at cf7f71a5. The SHA pin protects against later drift but doesn't prove the original port was faithful. Live judge calibration and GPU training are, as the PR says, still to be done.
Drafted by Codex.
A new Codex review is starting; withdrawing the previous approval.
There was a problem hiding this comment.
Drafted by Codex.
Reviewed commit c7493a4d6ac1ccdbb12b47f827f42ac783c7d2bb.
No blocking findings or non-blocking correctness concerns found.
Reviewed the exact BASE...HEAD diff and head sources, including source adaptation, target/profile validation, native extraction and scoring, shared judge dispatch, grading gates, and training/evaluation failure handling. Existing generic judge behavior is preserved. There are no previous Codex findings to reassess.
Exact-head parser and rubric smoke checks passed. Ruff and pytest are unavailable in this sandbox, so I could not independently rerun the reported checks. Live judge calibration and GPU training remain unverified, as disclosed in the PR; these limits do not block approval of this scoped change.
There was a problem hiding this comment.
Drafted by Claude.
Reviewed commit c7493a4d6ac1ccdbb12b47f827f42ac783c7d2bb.
Merge recommendation: Ready to merge.
The follow-up commits address the substantive points from the earlier review. I found no blocking correctness or security problem. One new side effect of the diagnostics relabelling is worth fixing (item 1). The rest are minor.
Earlier review: what changed
- Judge failure policy override: resolved.
general_judge_scoreno longer forcesraisefor abstention. A judge failure now follows the global policy (general_judge.py:325-374). Training still excludesjudge_errorresponses throughrewards.py:570-574, and evaluation records a fallback zero and keeps running.test_bridge_excludes_failed_training_grades_and_continues_evaltests both paths, and the doc now describes this accurately. - Failure diagnostics
kind: resolved (general_judge.py:361). This change causes item 1 below. - Probe covering the judge's C path: resolved. A fourth, unboxed hedged control is added (
judge_registry.py:124). The test asserts 3 judge calls andjudge_called=Trueon the last result. - Adapter robustness: resolved. It now uses
responses_create_params or {}, and the domain fallback is limited to the 24-hex HotpotQA IDs, with"Unknown"for anything else (source_preparation.py:91-96). resolve_requestcomment: resolved (judge_registry.py:45-46).- Still open, both minor:
build_prompthashes the template on every call (abstention.py:330), andtest_miles_abstention.py:1still says "C033".
1. Abstention judge calls and errors no longer appear in rollout/general_judge/* metrics (worth fixing)
open_instruct/miles/rewards/general_judge.py:315-317, 361, 400-403
rollout_metrics.general_judge_metrics (miles/rollout/rollout_metrics.py:88-102) only counts records where record.get("kind") == "general_judge". Abstention diagnostics now use kind = "nemo_abstention_v1" in all three cases: the explicit-IDK shortcut, failures and successes. So abstention samples never show up in rollout/general_judge/samples, errors or error_fraction.
This matters for training. If the abstention judge goes down, its judge_error responses are excluded as intended, but the judge error-rate metric won't move. The only remaining signal is the generic rollout/verification/ungraded_* counter. If abstention is the only judged source in a run, the general_judge metrics are missing entirely.
Reward exclusion is not affected: rewards.py:570 checks status, not kind. This is metrics only.
Possible fixes:
- Keep
kind="general_judge"and add a separate field such asprofile="nemo_abstention_v1"orrubric_kind=...for filtering. - Or widen the filter in
general_judge_metricsto includeabstention.KIND. Consider whether explicit-IDK records (judge_called=False) should count toward "samples". I'd lean towards counting only judge calls, or adding a separateabstention/explicitcounter. That way, a policy that learns to output[IDK]doesn't dilute the error fraction.
Either way, a small test in test_miles_abstention.py that runs general_judge_metrics on an abstention judge_error sample would lock this in. I raised the failure-path relabelling in the previous review without noticing this consumer, so this one is partly on me.
2. Explicit-IDK shortcut skips the failure-policy validation (nit)
general_judge.py:314-327
The early return 0.5 comes before the policy not in {"zero", "raise"} check. A misconfigured OI_MILES_JUDGE_FAILURE_POLICY therefore goes unreported until the first response that isn't [IDK]. That is harmless in practice. If you want config errors to surface on the first call regardless of the response, move the policy validation above the shortcut.
3. prompt_sha256 and rubric_sha256 are identical for this profile (question)
judge_registry.py:64, general_judge.py:392
resolve_request returns digest in both positions, so the success diagnostics record the rubric template hash as both prompt_sha256 and rubric_sha256. The per-sample rendered prompt is still distinguished by rendered_prompt_sha256. This is not new (the generic profiles already do the same), so it's only worth a comment if downstream evidence consumers treat the two fields as different things.
4. CHANGELOG placement (nit)
CHANGELOG.md:10: the entry adds a new adapter and profile but sits under "Changed". If the file uses an "Added" section in this release block, that would be the more natural place. Otherwise, ignore.
What I checked
- Read the full diff and compared it with the prior review.
- Traced
general_judge_scorethroughrewards._score(rewards.py:565-610) for the training-omission and evaluation zero-fallback paths. - Grepped every consumer of diagnostics
kindandjudge_error, which turned up item 1. - Verified that the probe's fourth control doesn't normalize to
[IDK], so it reaches the judge. - Confirmed that
prepare()catches the adapter'sValueError/KeyErrorpaths, so a bad row is omitted rather than aborting preparation. - Checked that
validate_dataenforces the profile/target pairing in both directions.
Verification limits
This was a static review; I ran no tests. I did not compare the ported rubric and parser byte-for-byte against NeMo Gym cf7f71a5. Live judge calibration and GPU training are still to be done, as the PR says.
Documentation Changes Detected📄
|
Drafted by Codex.
A new Codex review is starting; withdrawing the previous approval.
Documentation Changes Detected📄
|
There was a problem hiding this comment.
Drafted by Codex.
Reviewed commit ebdca5379bdad50fe3b5edf7f3306b0475e835df.
No blocking findings or other actionable concerns found.
Checked the exact BASE...HEAD diff, source adaptation, target/profile validation, native answer extraction and reward mapping, service probing, and shared grading/failure handling. Existing judge profiles retain their prior behavior; no optimizer, distributed coordination, or checkpoint paths changed. There were no previous change requests to reassess.
Validation: changed Python sources passed syntax checks; head-source abstention smoke checks and diff whitespace checks passed. Pytest and Ruff are unavailable in this sandbox. Live judge calibration and GPU training remain unverified, as disclosed in the PR.
There was a problem hiding this comment.
Drafted by Claude.
Reviewed commit ebdca5379bdad50fe3b5edf7f3306b0475e835df.
Merge recommendation: Ready to merge.
The latest commit (ebdca53) fixes the one substantive point from the previous review, the missing judge metrics. I found no blocking correctness or security problem. What's left is minor and optional.
Prior review: status
- 1. Abstention judge calls missing from
rollout/general_judge/*metrics: resolved. Success and failure diagnostics usekind="general_judge"again (open_instruct/miles/rewards/general_judge.py:362, 384). The profile is now carried asrubric_profile/judge_called(:363, :403-405), sogeneral_judge_metrics(open_instruct/miles/rollout/rollout_metrics.py:88-102) counts abstention judge calls and judge errors again. The explicit-[IDK]shortcut keepskind=abstention.KIND(general_judge.py:319). As a result, an[IDK]that never reached the judge doesn't dilute the judge error fraction. I think that is the right split.test_native_judge_rewardsandtest_empty_judge_is_not_an_incorrect_gradenow assertkind == "general_judge". - 2. Explicit-IDK shortcut skipped policy validation: resolved. The
policy not in {"zero", "raise"}check (general_judge.py:314-316) now runs before the shortcut (:317). - 3.
prompt_sha256andrubric_sha256are the same value: unchanged. This was already true before this PR and is only a question. Nothing new here. - 4. CHANGELOG entry under "Changed": unchanged (
CHANGELOG.md:10). This is a nit. The entry adds a new adapter and profile, so "Added" may fit better if this release block uses that heading. - Earlier nits, still open (optional):
abstention.build_promptre-hashes the 5 KB template on every judged sample (open_instruct/miles/rewards/abstention.py:327). Checking it once at import, or caching the result, would keep the drift guard without per-call cost. The test module docstring still says "for C033" (open_instruct/test_miles_abstention.py:1), which is an internal tracking ID with no meaning in the repo.
New observations (all minor)
-
The explicit-IDK record has a different shape from the judged records (optional).
general_judge.py:318-327gives the shortcut recordkind,status,verdict,score,judge_called,binding,rubric_sha256andrubric_profile. It has nomodel,seed,temperatureorrendered_prompt_sha256. I found no consumer that requires those fields (a grep forjudge_calledandrendered_prompt_sha256in non-test code only turns up this file), so this is fine today. If Workbench evidence tooling later groups diagnostics byrubric_profile, it will need to treat bothkindvalues as one profile. A one-line comment at:317would make that contract explicit, for example: "explicit abstention is not a judge call, so it is deliberately excluded from general_judge metrics". -
reasoningandverdicthold the same value on the judged path (nit). For abstention,abstention.gradereturns("correct"|"incorrect"|"abstain", score), soreasoning(:395) and theverdictadded at:404are always equal. That's harmless. For other parsers,reasoningholds the judge's free text, so readers may expect it to differ here. You could leavereasoningout for this parser, or leave it as is. -
The test name
test_unavailable_judge_is_unknownoverstates its coverage (nit).test_miles_abstention.pyonly exercisesraisemode. The zero-policy path for transport failures is covered indirectly by the empty-reply test and bytest_bridge_excludes_failed_training_grades_and_continues_eval, so coverage is fine. A name liketest_unavailable_judge_raises_in_strict_modewould match what the test actually does. -
The adapter's role check assumes plain-string content (question).
source_preparation.py:358comparesms[-1]["content"] != target["question"]. Rows with list-shaped content parts would be rejected and omitted rather than adapted. That fails closed and probably doesn't happen in this dataset, so I'm only flagging it in case the published rows ever use the content-parts form.
What I checked
- Read the full diff and compared it with the head
general_judge.pyand the base behaviour, including the generic-profile paths throughresolve_request. They are unchanged apart from computingparserlocally (judge_registry.py:41-68). - Re-traced the diagnostics
kindconsumers inrollout_metrics.py(code_service_metrics,math_verifier_metrics,general_judge_metrics) to confirm the metrics fix. - Confirmed the probe's fourth control is unboxed hedging text that doesn't normalize to
[IDK], so it reaches the judge. The test asserts 3 calls andjudge_called=True. - Confirmed that
validate_dataenforces the profile/target pairing in both directions, and thatresolve_requestrejects abstention targets bound to generic rubrics.
Verification limits
This was a static review; I ran no tests. I did not compare the ported rubric and parser byte-for-byte against NeMo Gym cf7f71a5. Live judge calibration and GPU training are still to be done, as the PR description says.
Nemotron-RL-QA-Abstention-v1 asks a policy to answer factoid questions or abstain when uncertain. Its factual references alone are insufficient for training: ordinary answer matching gives the same score to abstention and an incorrect answer. This change adds its native source adapter and NVIDIA NeMo Gym's three-tier judged reward so RL Data Workbench can prepare the source for a future dataset release.
The
nvidia/qa-abstention-v1named rubric preserves the official OMNISCIENCE_GRADER template, thinking stripping, nested/last-box extraction, and A/B/C parsing from Gymcf7f71a57dde0a69b14a2e919b60a01b17cc7abd. It fixes the native default reward scale at correct=1, abstain=0.5, incorrect=0. Explicit normalized IDK avoids a judge request; service failures and empty replies remain ungraded. Profile/target bindings are validated before use, and the service probe checks both explicit and judge-recognized abstention as well as correct and incorrect answers.The adapter preserves the original system/user prompt and emits question/reference targets, keeping reward code in Open Instruct. Documentation records the native parser's permissive fallback and the need for independent judge calibration. No generic-quality rubric or exact-match reward is substituted.
Validation: 219 CPU tests passed, 5 optional tests skipped across abstention, source preparation, named judging, judge failure handling and shared grading. Repository-wide Ruff lint/format and diff whitespace checks pass. Live model judging and GPU training have not run; they remain release qualification work. Workbench also needs to resolve the dataset card/row license discrepancy and run combined protected-source hygiene.
Review follow-ups preserve the existing judge failure policy: training excludes judge-error responses, while evaluation can record a diagnostic fallback zero and continue. Explicit bridge controls cover both paths.
The documented default reuses the existing
judges.generalQwen/Qwen3-32B service, adding a rubric/binding without allocating another model instance. Judge-call diagnostics retain the existinggeneral_judgediscriminator for rollout error metrics; explicit IDK shortcuts are separate and do not dilute the request error rate. Live calibration is deferred per Robert.