Skip to content

fix(gdpval): make Office→PDF preconvert actually work in comparison mode - #1228

Merged
bxyu-nvidia merged 4 commits into
mainfrom
agronskiy/gdpval-preconvert-via-apptainer
May 5, 2026
Merged

fix(gdpval): make Office→PDF preconvert actually work in comparison mode#1228
bxyu-nvidia merged 4 commits into
mainfrom
agronskiy/gdpval-preconvert-via-apptainer

Conversation

@agronskiy

@agronskiy agronskiy commented May 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

The most recent GDPVal run on nemotron-ultra-v3-rl-ea1-apr13-step108 showed that 96.6% of Office deliverables (.docx / .xlsx / .pptx) reach the multimodal judge as filename-only stubs because the gym evaluation container doesn't ship libreoffice and resources_servers/gdpval/preconvert.py swallowed every failure silently. This PR removes every stage of the silent failure:

  1. Surface preconvert failures — preconvert_dir now returns (ok, fail, error_messages); both call sites log a sample at WARNING. (commit 1)
  2. Auto-install libreoffice — setup_libreoffice.ensure_libreoffice() apt-installs at GDPValResourcesServer.model_post_init if the binary is missing. (commit 2)
  3. Isolate libreoffice profile per call — each convert_to_pdf runs with its own -env:UserInstallation=file://<tempdir>, so concurrent invocations don't race on $HOME/.config/libreoffice (the reason the previous default preconvert_max_concurrent was 1). Default bumped to 4. (commit 3)
  4. Fail fast in comparison mode — when preconvert_office_to_pdf=True, reward_mode="comparison", and ensure_libreoffice() returns False, raise at startup. Comparison mode without working preconvert silently feeds Office deliverables to the judge as filename-only stubs and biases the win rate; loud failure beats degraded judging. (commit 3)

Why

(Investigation context: gym gdpval-thinking-summary.md Chapter 2.) The deployment container's pre_cmd apt install -y git-lfs rpm2cpio cpio squashfuse fuse3 does not include libreoffice. gdpval/preconvert.py:101 discarded the per-file error messages and app.py:319-321 ignored the (success, fail) tuple. Combined: a 100%-failed preconvert produced zero log lines and the run continued to a degraded judge.

Quantified impact on the most recent run:

                                  agronskiy (eval)        Kimi (reference)
total Office files                       357                     165
  with sibling .pdf                       12 (3.4%)              165 (100%)
  WITHOUT sibling .pdf                   345 (96.6%)               0 (0%)

build_file_section dispatch:
  files reaching judge as filename-only  363 / 768 (47.3%)         3 / 524 (0.6%)

The 12 sibling pairs on the eval side all had mtime_pdf - mtime_office == 0.0s (model wrote both formats simultaneously in one code_exec call); zero traced back to libreoffice. With this PR, the same setup either ships every Office file with a sibling PDF (libreoffice present or installable) or aborts at startup (comparison mode, libreoffice unavailable) instead of silently judging stubs.

Commits

  • 6bd8c21b fix(gdpval): surface silent failures in defensive preconvert
  • f962ef62 feat(gdpval): auto-install libreoffice for Office→PDF preconvert
  • 2273013d fix(gdpval): isolate libreoffice profile per call; fail fast in comparison mode

Test plan

  • pytest resources_servers/gdpval/tests/ — 39 passed (13 new + 26 existing). New tests cover: profile-flag presence, error-list propagation, ensure_libreoffice apt-install branches (already-present, non-Linux, no apt-get, apt-get update fail, apt-get install fail, install reported success but binary missing, full success, timeout, --no-install-recommends enforcement), comparison-mode fail-fast, rubric-mode tolerance, comparison-mode passes when libreoffice available.
  • ruff check + ruff format --check clean.
  • pre-commit run --files <changed> clean.
  • Smoke run on a small GDPVal subset to verify Office files now ship with sibling PDFs and the comparison judge sees PDF blocks.
  • Full GDPVal run on nemotron-ultra-v3-rl-ea1-apr13-step108 to validate the win-rate change vs the silent-failure baseline.

Backward compatibility

  • preconvert_dir return signature changed from (ok, fail) to (ok, fail, errors). Internal API; only one in-repo caller.
  • preconvert_office_to_pdf default stays True — existing behavior preserved when libreoffice is already on PATH (shutil.which early-return; no apt cost).
  • Comparison-mode runs that previously silently degraded will now fail loudly until libreoffice is reachable. To keep the old (broken) behavior, set preconvert_office_to_pdf=false.
  • preconvert_max_concurrent default raised from 1 → 4. Safe because of per-call profile isolation.

🤖 Generated with Claude Code

@agronskiy
agronskiy force-pushed the agronskiy/gdpval-preconvert-via-apptainer branch from 4684053 to edbd762 Compare May 5, 2026 08:38
agronskiy and others added 3 commits May 5, 2026 11:56
@agronskiy @claude
fix(gdpval): surface silent failures in defensive preconvert
6bd8c21 
The host-side preconvert in resources_servers/gdpval is retained as a
fallback for the new agent-side path (e.g. when the agent ran with a
non-Apptainer provider, or for the reference deliverables tree). It
used to discard per-file error messages inside the ThreadPoolExecutor
loop and ignore the (success, fail) tuple at the call site, which is
why a 100%-failed conversion in the production run produced zero log
lines.

Changes:
- ``preconvert_dir`` now returns ``(num_success, num_failed,
  error_messages)``; ``preconvert_dir_async`` propagates the tuple.
- ``convert_to_pdf`` returns specific error strings for FileNotFoundError
  ("libreoffice not found on host PATH"), TimeoutExpired, missing-PDF
  after ran, and generic exceptions.
- New ``GDPValResourcesServer._preconvert_and_log`` helper logs
  ``ok=N fail=M`` at INFO and the first 5 error messages at WARNING.
  Both call sites (eval dir + each ref dir) go through it.

Tests cover: errors-list propagation, libreoffice-not-found message,
libreoffice-ran-but-no-pdf message, empty dir no-op, success path,
async wrapper.

Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
@agronskiy @claude
feat(gdpval): auto-install libreoffice for Office→PDF preconvert
f962ef6 
The deployment container (where the gdpval resources server runs)
ships without libreoffice, which silently no-ops the comparison-mode
preconvert and ~96.6% of Office deliverables reach the multimodal
judge as filename-only stubs.

Add ``setup_libreoffice.ensure_libreoffice`` and call it from
``GDPValResourcesServer.model_post_init`` whenever
``preconvert_office_to_pdf`` is enabled. It checks
``shutil.which("libreoffice")`` first (early-return), then
apt-get-installs ``libreoffice`` and ``fonts-liberation`` on Linux
hosts that have ``apt-get``. Failures (non-Linux, missing apt-get,
network, etc.) log at WARNING and return False so the server still
boots; per-file conversion errors are then surfaced via the existing
preconvert-failure logging.

Pattern follows the existing ``ensure_*()`` modules
(ifbench, newton_bench, ether0, bird_sql, spider2_lite); this is the
first one to apt-install a system package.

Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
@agronskiy @claude
fix(gdpval): isolate libreoffice profile per call; fail fast in compa…
2273013 
…rison mode

Two related preconvert hardening changes:

1. ``convert_to_pdf`` now creates a fresh ``UserInstallation`` profile
   directory per call (``-env:UserInstallation=file://<tempdir>``).
   Without this, concurrent libreoffice subprocesses race on the
   shared ``$HOME/.config/libreoffice`` lock, which is why the prior
   ``preconvert_max_concurrent`` default was 1. Default bumped to 4
   now that profiles are isolated.

2. ``GDPValResourcesServer.model_post_init`` raises ``RuntimeError``
   when ``preconvert_office_to_pdf=True``, ``reward_mode="comparison"``,
   and ``ensure_libreoffice()`` returns False. Comparison mode without
   working preconvert silently feeds Office deliverables to the
   multimodal judge as filename-only stubs, biasing the win rate;
   fail loudly at startup so the operator can install libreoffice
   or opt out via ``preconvert_office_to_pdf=false``. Rubric mode
   keeps booting (its content-block builder has a text fallback).

Test plan:
- New ``test_passes_user_installation_flag`` asserts the flag is
  present and uniquely scoped.
- New ``test_comparison_fails_fast_when_libreoffice_unavailable``,
  ``test_rubric_does_not_fail_when_libreoffice_unavailable``,
  ``test_comparison_passes_when_libreoffice_available`` cover the
  startup gate.
- Test helper ``_server(...)`` now defaults
  ``preconvert_office_to_pdf=False`` so existing tests don't trip
  the gate; tests exercising preconvert opt back in.

Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
@agronskiy
agronskiy force-pushed the agronskiy/gdpval-preconvert-via-apptainer branch from edbd762 to 2273013 Compare May 5, 2026 10:16
@agronskiy agronskiy changed the title Preconvert Office deliverables via Apptainer + surface silent failures in defensive layer fix(gdpval): make Office→PDF preconvert actually work in comparison mode May 5, 2026
agronskiy added a commit that referenced this pull request May 5, 2026
@agronskiy
Merge PR #1228 (agronskiy/gdpval-preconvert-via-apptainer) into pr-train
d16962d
@agronskiy @claude
fix(gdpval): pull default-jre-headless alongside libreoffice
39ed19f 
Empirical: on the 2026-05-05 b8d417be75c122bb GDPVal run, libreoffice
auto-installed via this module worked, but every Office doc that
needed Java for chart/formula/embedded-object rendering hit:

  Warning: failed to launch javaldx - java may not function correctly

…and libreoffice silently exited rc=0 without producing the PDF
(observed: `Compensation Model Ideas.pdf` for `task_fd3ad420` on
both eval and ref repeats — the doc has charts).

PR #1228's defensive `did not produce X.pdf` check correctly catches
this so the rollout doesn't proceed with a missing PDF, but the
underlying preconvert still loses signal — the judge falls back to a
plain-text view of the doc and misses the visual rendering. Headless
JRE is the standard libreoffice dependency for this; no GUI needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
agronskiy added a commit that referenced this pull request May 5, 2026
@agronskiy
Merge PR #1228 (agronskiy/gdpval-preconvert-via-apptainer) into pr-train
f08ec34
@bxyu-nvidia
bxyu-nvidia merged commit b13b143 into main May 5, 2026
5 checks passed
@bxyu-nvidia
bxyu-nvidia deleted the agronskiy/gdpval-preconvert-via-apptainer branch May 5, 2026 17:07
@Kh4L Kh4L mentioned this pull request May 5, 2026
Merged
1 of 2 tasks
@agronskiy agronskiy mentioned this pull request May 8, 2026
Merged
1 of 2 tasks
bxyu-nvidia pushed a commit that referenced this pull request May 11, 2026
@agronskiy @claude
fix(gdpval): also apt-install JRE when libreoffice is pre-baked into …
bf0a179 
…image (#1268)

## Summary

PR #1228 added `default-jre-headless` to the apt package list in
`setup_libreoffice.py` because libreoffice silently exits `rc=0` with
`Warning: failed to launch javaldx` for any
chart/formula/embedded-object/pivot-table doc when no JRE is on `PATH`,
producing **filename-only stubs in comparison-mode preconvert**.

But `ensure_libreoffice()` short-circuits at
`shutil.which(\"libreoffice\")` *before* running apt — so when the
deployment image already bakes libreoffice in (without a JRE), the
apt-install never runs and JRE is never installed. The package-list
change in #1228 is unreachable on that code path.

## Why this matters

Observed in two GDPVal pipeclean runs on `ultra-v3` (`b8d417be`,
`078ab5e1`) — **~25–30% of office docs reach the multimodal judge as
filename-only stubs** with `libreoffice rc=0 did not produce X.pdf:
Warning: failed to launch javaldx` for every chart/formula doc.

This biases comparison-mode win rates: if Kimi reference docs convert
without Java but eval candidate's docs don't (or vice-versa), the judge
votes against the side delivering only a filename.

## Fix

- Gate the early-exit on **both** `libreoffice` AND `java` being on
`PATH`.
- When libreoffice is present but java is missing, still run `apt-get
install` — idempotent on libreoffice (apt skips already-installed), and
gives us the missing JRE.
- Add a post-install `which(\"java\")` check that warns and fails if
apt-install reports success but java still isn't on `PATH`.

## Test plan

- [x] `pytest resources_servers/gdpval/tests/test_setup_libreoffice.py`
— 11/11 pass (existing tests refactored to use a dict-based `which`
mock; 2 new tests cover the libreoffice-present-but-java-missing case)
- [ ] Validate end-to-end on a GDPVal pipeclean run — fold this PR into
the pr-train and re-pin EFB (next step).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: Alex Gronskiy <agronskiy@nvidia.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
marta-sd added a commit that referenced this pull request Jul 10, 2026
@Kh4L @marta-sd
[GDPVal] Align rubric judge max_tokens default with comparison mode (#…
2b1c0dd 
…1234)

## Summary

Two related changes to harmonize judge-side knobs between rubric and
comparison modes:

1. **Rubric default max_tokens 8192 → 65535**
(`scoring.py:score_with_rubric`, `score_with_rubric_visual`). Matches
the default already in place for comparison mode
(`comparison.send_judge_request`) and structured rubric
(`score_with_rubric_structured`). Prevents `finish_reason: length`
truncations on rubrics with many criteria when callers don't override.

2. **`create_overrides` plumbing for comparison-mode judge calls** —
adds the same override mechanism to `comparison.send_judge_request` and
threads it through `run_trials` and `_verify_comparison`. Mirrors the
rubric path where `judge_responses_create_params_overrides` already gets
merged into `chat.completions.create`. Also switches
`_verify_comparison` from `overrides.get` → `overrides.pop` so
`model`/`api_key` are consumed and the rest pass through (same pattern
`_verify_rubric` uses post #1225).

After this lands, callers can override max_tokens / temperature / top_p
/ etc. for the comparison-mode judge from Hydra config exactly the same
way they already can for rubric mode.

## Replaces #1215

Supersedes #1215 (same change, rebased onto current main). The original
branch was DIRTY against main due to overlap with #1225 (raw_responses
kwarg on `run_trials`), #1228 (preconvert + `clean_up_list`), and #1229
(`JUDGE_REQUEST_TIMEOUT_SECONDS`). All three are kept; this PR adds
`create_overrides` alongside.

## Test plan

- [x] Existing
`test_app.py::test_verify_rubric_passes_create_overrides_through`
continues to verify the rubric path.
- [ ] Ad-hoc smoke: run a 5-task comparison-mode rollout with
`++gdpval_judge_model.responses_api_models.openai_model.max_tokens=8192`
to verify the override actually reaches the comparison judge call.

Signed-off-by: Serge Panev <spanev@nvidia.com>
Co-authored-by: Marta Stepniewska-Dziubinska <marta-sd@users.noreply.github.com>
OlegSudakov pushed a commit to OlegSudakov/Gym that referenced this pull request Jul 15, 2026
@Kh4L @marta-sd @OlegSudakov
[GDPVal] Align rubric judge max_tokens default with comparison mode (N…
b9a0acd 
…VIDIA-NeMo#1234)

## Summary

Two related changes to harmonize judge-side knobs between rubric and
comparison modes:

1. **Rubric default max_tokens 8192 → 65535**
(`scoring.py:score_with_rubric`, `score_with_rubric_visual`). Matches
the default already in place for comparison mode
(`comparison.send_judge_request`) and structured rubric
(`score_with_rubric_structured`). Prevents `finish_reason: length`
truncations on rubrics with many criteria when callers don't override.

2. **`create_overrides` plumbing for comparison-mode judge calls** —
adds the same override mechanism to `comparison.send_judge_request` and
threads it through `run_trials` and `_verify_comparison`. Mirrors the
rubric path where `judge_responses_create_params_overrides` already gets
merged into `chat.completions.create`. Also switches
`_verify_comparison` from `overrides.get` → `overrides.pop` so
`model`/`api_key` are consumed and the rest pass through (same pattern
`_verify_rubric` uses post NVIDIA-NeMo#1225).

After this lands, callers can override max_tokens / temperature / top_p
/ etc. for the comparison-mode judge from Hydra config exactly the same
way they already can for rubric mode.

## Replaces NVIDIA-NeMo#1215

Supersedes NVIDIA-NeMo#1215 (same change, rebased onto current main). The original
branch was DIRTY against main due to overlap with NVIDIA-NeMo#1225 (raw_responses
kwarg on `run_trials`), NVIDIA-NeMo#1228 (preconvert + `clean_up_list`), and NVIDIA-NeMo#1229
(`JUDGE_REQUEST_TIMEOUT_SECONDS`). All three are kept; this PR adds
`create_overrides` alongside.

## Test plan

- [x] Existing
`test_app.py::test_verify_rubric_passes_create_overrides_through`
continues to verify the rubric path.
- [ ] Ad-hoc smoke: run a 5-task comparison-mode rollout with
`++gdpval_judge_model.responses_api_models.openai_model.max_tokens=8192`
to verify the override actually reaches the comparison judge call.

Signed-off-by: Serge Panev <spanev@nvidia.com>
Co-authored-by: Marta Stepniewska-Dziubinska <marta-sd@users.noreply.github.com>
Signed-off-by: Oleg Sudakov <oleg.sudakov@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kh4L Kh4L Kh4L approved these changes
@bxyu-nvidia bxyu-nvidia bxyu-nvidia approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@agronskiy @Kh4L @bxyu-nvidia