Skip to content

fix: un-blocklist deepseek_v3 in transformers tokenizer class set - #3116

Merged
yuki-97 merged 9 commits into
mainfrom
fix/moonlight-tokenizer-offline-2764
Jul 9, 2026
Merged

fix: un-blocklist deepseek_v3 in transformers tokenizer class set#3116
yuki-97 merged 9 commits into
mainfrom
fix/moonlight-tokenizer-offline-2764

Conversation

@ZhiyuLi-Nvidia

@ZhiyuLi-Nvidia ZhiyuLi-Nvidia commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

#2764
Two coordinated fixes so Moonlight-16B-A3B (and any future deepseek_v3 model) can load its tokenizer offline.

Bug 1 — transformers blocks the tokenizer

transformers 5.4–5.11 lists "deepseek_v3" in two internal registries that together force the fast tokenizer backend and suppress trust_remote_code:

Registry Effect
MODELS_WITH_INCORRECT_HUB_TOKENIZER_CLASS (set) Suppresses the model's tokenizer_class / auto_map
TOKENIZER_MAPPING_NAMES (dict) Pins deepseek_v3 to TokenizersBackend (fast)

Under HF_HUB_OFFLINE=1 the fast backend can't enumerate the repo to find tiktoken.model and fails. Moonlight ships no tokenizer.json, only a slow TikTokenTokenizer reachable via auto_map — so the online fallback also can't help.

Fix: nemo_rl/__init__.py removes both entries at import time. discard / pop-with-default are no-ops if the entries are absent, so the patch is safe on any transformers version.

Bug 2 — dead code in Megatron tokenizer setup

nemo_rl/models/megatron/setup.py:setup_model_and_optimizer used to mutate hf_tokenizer_kwargs["trust_remote_code"] = True on the Megatron TokenizerConfig. This was dead code:

Field Snapshot at __post_init__ (empty dict) Original code's intent Actual runtime This PR
trust_remote_code dict.get("trust_remote_code", False) = False True (dead) False — dead True (attribute set directly)
tokenizer_hf_no_use_fast not dict.get("use_fast", True) = False (i.e. use_fast=True) True (dead) use_fast=True via default Unchanged, use_fast=True

Root cause of the deadness: TokenizerConfig.__post_init__ (see Megatron-Bridge/src/megatron/bridge/training/tokenizers/config.py) snapshots hf_tokenizer_kwargs into plain attributes once at construction time; from then on build_tokenizer reads the attribute, not the dict.

Fix: assign the attribute directly — megatron_cfg.tokenizer.trust_remote_code = True. This is the API Megatron-Bridge's own deprecation notice recommends.

Blast radius of the trust_remote_code=True flip

Historically every model on this Megatron path ran with trust_remote_code=False (the dead-code effect above). Making it actually True:

  • Standard-tokenizer models (Llama / Qwen / GLM / gpt-oss / …) — no behavior change; transformers only consults the flag when the model has auto_map / tokenizer_class pointing at remote code.
  • Remote-code tokenizer models (Moonlight-16B-A3B, DeepSeek-V3, …) — bug fixed.

Test plan

  • llm_grpo_moonlight_16b_automodel_1n8g_ep8 (1n8g automodel) — 10+ training steps offline, KL 0.0003, no NaN, no backend-tokenizer errors on driver + 8 workers.
  • llm_grpo_moonlight_16ba3b_4n8g_megatron (4n8g Megatron) — 17+ training steps offline, KL 0.0003 stable across steps, zero tokenizer errors on driver + 32 workers.
  • Sanity probe: import nemo_rl; "deepseek_v3" not in MODELS_WITH_INCORRECT_HUB_TOKENIZER_CLASS and "deepseek_v3" not in TOKENIZER_MAPPING_NAMES → True.
  • Sanity probe: offline AutoTokenizer.from_pretrained("moonshotai/Moonlight-16B-A3B-Instruct", trust_remote_code=True) returns TikTokenTokenizer with byte-identical input_ids to the worker-side NeMoAutoTokenizerWithBosEosEnforced under add_special_tokens=False.
  • llm_grpo_moonlight_16ba3b_4n8g_megatron_tq_simple and _fp8_e2e — variants of the base 4n8g recipe; expected to pass since the fix is at tokenizer construction (pre-quantization).

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia requested a review from a team as a code owner July 8, 2026 05:57
@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia requested a review from a team as a code owner July 8, 2026 07:53
@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/moonlight-tokenizer-offline-2764 branch 2 times, most recently from 1b10a01 to 02e7e91 Compare July 8, 2026 17:52
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Jul 8, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 02e7e91

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia requested review from ahmadki and yuki-97 July 8, 2026 18:00
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to c82d9a0

@yuki-97 yuki-97 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.

@ZhiyuLi-Nvidia thanks for the fix! left some minor comments.

Comment thread nemo_rl/__init__.py Outdated
Comment thread nemo_rl/__init__.py Outdated
Comment thread nemo_rl/models/megatron/setup.py Outdated
Fixes #2764.

transformers 5.4-5.11 lists "deepseek_v3" in two internal registries --
MODELS_WITH_INCORRECT_HUB_TOKENIZER_CLASS (a set) and TOKENIZER_MAPPING_NAMES
(a dict pinning it to "TokenizersBackend"). Together they force the fast
tokenizer backend and suppress trust_remote_code, so AutoTokenizer can only
load via a local tokenizer.json. Models like Moonlight-16B-A3B ship no
tokenizer.json (only tiktoken.model + a remote-code TikTokenTokenizer),
so offline loading fails with "Couldn't instantiate the backend tokenizer".

Two coordinated patches:

1. nemo_rl/__init__.py: discard "deepseek_v3" from both registries at
   nemo_rl import time. Adds a version assertion that fires if transformers
   is bumped past 5.12 (upstream fix), noting Megatron-Bridge is the current
   blocker for that bump.

2. nemo_rl/models/megatron/setup.py: set megatron_cfg.tokenizer.trust_remote_code
   attribute directly. The pre-existing hf_tokenizer_kwargs["trust_remote_code"]
   dict mutation was dead code because Megatron-Bridge's TokenizerConfig
   snapshots hf_tokenizer_kwargs into plain attributes at __post_init__ and
   never re-reads the dict afterwards. Both patches are held together by the
   same MBridge-caps-transformers-below-5.12 dependency chain and can be
   dropped together once that pin is relaxed.

Verified end-to-end: llm_grpo_moonlight_16b_automodel_1n8g_ep8 trains 10+
steps offline; llm_grpo_moonlight_16ba3b_4n8g_megatron trains 17+ steps
offline with KL 0.0003 stable, no NaN, zero backend-tokenizer errors on
driver or 32 workers.

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/moonlight-tokenizer-offline-2764 branch from f421f4b to a74e966 Compare July 9, 2026 08:57
…sert

String comparison "5.5.0" < "5.12.0" is False in Python (lexicographic
'5' > '1' at the minor position), so the assertion would fire and raise
AssertionError on every import for every currently-pinned transformers
version (>=5.5.0,<5.9.0). Switch to packaging.version.Version.

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
…op packaging dep)

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
…y canonical pattern

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test ca6a042

The lint CI's editable install pipeline (uv sync + setuptools) imports
nemo_rl to statically read __version__ before deps are guaranteed to be
present. Without a guard, 'import transformers' at nemo_rl load time
crashes the build with 'nemo_rl has no attribute __version__'.

Wrap the top-level 'import transformers' in try/except so the patch
becomes a no-op under build isolation. Runtime venv always has the
package, so the assertion + discards still fire in real use.

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
…ead of try/except

Same rationale as the existing _check_container_fingerprint guard on
line 99: setuptools' uv-based build isolation imports nemo_rl to read
__version__, but transformers isn't in the isolated build env.

Match the existing pattern -- skip the patch at the call site under
build isolation, keep the function body clean (no exception handling
for control flow). Runtime venv is never a build isolation, so the
patch still fires in real use.

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
…emo_rl.models.policy.__init__

The patch is only needed by policy code paths that eventually call
AutoTokenizer.from_pretrained -- driver's get_tokenizer(), Megatron
worker's build_tokenizer, DTensor worker's tokenizer reconstruction.
All three share nemo_rl.models.policy as an import ancestor.

Placing it in nemo_rl.models.policy.__init__.py fires exactly once
per process when policy code is first imported. Non-policy consumers
(tests, utilities, environments) no longer mutate the global
transformers registries as a side effect of importing nemo_rl.

Bonus: setuptools build isolation reads nemo_rl.__version__ from
nemo_rl.__init__.py but never cascades into models/policy, so the
_is_build_isolation() guard is no longer needed at the call site.

Verified locally with 'from nemo_rl.models.policy import PolicyConfig':
deepseek_v3 removed from MODELS_WITH_INCORRECT_HUB_TOKENIZER_CLASS
and TOKENIZER_MAPPING_NAMES as expected.

Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia requested a review from a team as a code owner July 9, 2026 10:17
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 0405109

@yuki-97 yuki-97 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.

LGTM, thanks @ZhiyuLi-Nvidia @ahmadki

@yuki-97
yuki-97 enabled auto-merge (squash) July 9, 2026 11:04
@yuki-97
yuki-97 merged commit f18ca21 into main Jul 9, 2026
80 checks passed
@yuki-97
yuki-97 deleted the fix/moonlight-tokenizer-offline-2764 branch July 9, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) r0.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants