Repository navigation
tokenizers simplification - #6622
Conversation
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
|
/claude review |
There was a problem hiding this comment.
Light review — the core change is good, and it fixes a real latent bug: the old lookup read metadata['tokenizer_class'] / metadata['tokenizer_class_path'], keys write_metadata never wrote, so a custom tokenizer_class was silently ignored. test_own_metadata_class is a good regression test for that. A few leftovers:
1. Stale model_type docstrings in two files the PR missed
text_tokenizer.py had its model_type config line removed, but the same line survives in two other docstrings:
megatron/core/tokenizers/base_tokenizer.py:17megatron/core/tokenizers/vision/vision_tokenizer.py:27
class_path (str): path to tokenizer class.
- model_type (str): type of the model to be used with tokenizer.
chat_template (str): tokenizer chat template.2. Dead code left behind in megatron_tokenizer.py
- Line 7:
from collections import OrderedDict—TOKENIZER_MAPPING_NAMESwas its only user, so this import is now unused. - Line 127:
tokenizer_type = 'text' if tokenizer_library in TEXT_LIBRARIES else 'vision'inwrite_metadatais now an unused local; thedefault-{tokenizer_type}line that consumed it was removed._get_tokenizer_model_classcomputes its own copy.
Neither is caught by CI — ruff only selects S506, and F841 is in the ignore list.
3. Test writes an unimportable class_path into shared /opt/data
test_write_metadata_hf writes metadata with tokenizer_class=CustomTokenizerClass to the default path (/opt/data/tokenizers/huggingface/tokenizer_metadata.json), so class_path gets recorded as the pytest module name (test_tokenizer).
Before this PR that was harmless, because the class-restore branch never matched (wrong keys). Now _get_tokenizer_model_class calls importlib.import_module(metadata['class_path']) on it, so any later from_pretrained("/opt/data/tokenizers/huggingface") without an explicit metadata_path will try to import test_tokenizer — which only resolves inside the same pytest process that put that directory on sys.path, and /opt/data is a shared mount across test buckets.
Suggest pointing that call at a scratch metadata_path like the other cases do, so the default metadata file is not clobbered:
MegatronTokenizer.write_metadata(
tokenizer_path=tokenizer_path,
metadata_path=f"{tokenizer_path}/test_metadata.json",
tokenizer_library=tokenizer_library,
tokenizer_class=CustomTokenizerClass,
overwrite=True,
)4. Minor coverage gap
test_write_metadata_sp asserts the text default resolves to MegatronTokenizerText, but nothing covers the vision branch ('vision'.capitalize() -> MegatronTokenizerVision). A short test alongside the existing multimodal fixture would close it:
def test_write_metadata_vision():
tokenizer_path = "/opt/data/tokenizers/multimodal"
metadata_path = f"{tokenizer_path}/test_metadata.json"
MegatronTokenizer.write_metadata(
tokenizer_path=tokenizer_path,
metadata_path=metadata_path,
tokenizer_library="multimodal",
overwrite=True,
)
with open(metadata_path, "r") as f:
assert json.load(f)["class_name"] == "MegatronTokenizerVision"5. Heads-up (not blocking)
The removed Default* / GPTTokenizer subclasses used to inject class_name / class_path into config, so tokenizers built from an inline metadata dict ended up with .class_name / .class_path attributes set. They no longer do. Nothing in this repo reads those attributes, but downstream consumers (NeMo, Megatron-Bridge) might — worth a quick check before merge.
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
|
/ok to test 87802f5 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/32302898146 |
Signed-off-by: dimapihtar <dpykhtar@nvidia.com> Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com> Signed-off-by: Kezhi Kong <kezhik@kezhik-mlt.client.nvidia.com>
Signed-off-by: dimapihtar <dpykhtar@nvidia.com> Signed-off-by: Dmytro Pykhtar <dpykhtar@nvidia.com>
What does this PR do?
Simplifies tokenizers system by removing "models" implementations.
Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.