Skip to content

fix(examples): keep completion tokenizer in sync with model overrides - #670

Open
Tianyuan Ding (likecake) wants to merge 2 commits into
microsoft:mainfrom
likecake:codex/fix-completion-tokenizer-model
Open

Tianyuan Ding (likecake) wants to merge 2 commits into
microsoft:mainfrom
likecake:codex/fix-completion-tokenizer-model

Conversation

@likecake

@likecake Tianyuan Ding (likecake) commented Oct 31, 2026 •

Copy link
Copy Markdown

Fixes #669.

In completion mode, overriding actor_rollout_ref.model.path changes the training model but previously left the GSM8K and Search-R1 agents using the tokenizer copied before the override was merged. For example, Search-R1 could train Qwen/Qwen2.5-3B-Instruct while its agent still encoded requests with the default Llama tokenizer.

Use ${actor_rollout_ref.model.path} for both tokenizer defaults so they resolve against the final configuration. Explicit tokenizer environment overrides retain precedence. Document this behavior in both examples.

Validation

On Windows with Python 3.12.10:

.venv\Scripts\ruff.exe check .
.venv\Scripts\ruff.exe format --check .
.venv\Scripts\python.exe scripts/check_headers.py
git diff --check
  • Before their removal at maintainer request, the example regression tests showed 4 failed, 6 passed before the fix and 10 passed after the fix.
  • Ruff, formatting, copyright headers, and diff whitespace checks passed again after removing the example test file. The earlier focused Pyright check also passed before removal.
  • For the earlier regression run, SSLKEYLOGFILE was cleared only for the test process because the inherited local value points to an inaccessible log file.
  • Full test suite, full-project type checking, documentation build, and real model inference/training were not run locally.

Copilot AI balanced review requested due to automatic review settings October 31, 2026 12:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@likecake

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@hzy46

Copy link
Copy Markdown
Contributor

We do not need tests for examples. Please remove the tests and I will merge this PR.

@02anicol-byte

02anicol-byte commented Nov 2, 2026 via email

Copy link
Copy Markdown

@likecake

Copy link
Copy Markdown
Author

Zhiyuan He (@hzy46) Thanks for the review! I've removed the example tests as requested and pushed the update in d154c7a.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Completion tokenizer does not follow model config overrides in GSM8K and Search-R1

4 participants