Repository navigation
chore(configs)!: drop vlm attr-path fields and the unread orchestrator.model.vlm - #3852
Closed
faresobeid wants to merge 1 commit into
Closed
faresobeid wants to merge 1 commit into
faresobeid wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #3861 (
refactor/trainer-vlm: fold multimodal into trainer/vlm.py; one VLM registry). That PR removes the last readers of these fields. Merge it first.Summary
Breaking config change.
[model.vlm]losesvision_encoder_attrandlanguage_model_attr. Onlyfreeze_vision_encoderis left. The trainer now finds the modules by convention (model.visual, andmodel.language_modelwith a fallback tomodel.model). The only trainable VLM families (Qwen3.5 dense and MoE) already used exactly these paths.orchestrator.model.vlmis removed. Nothing in the orchestrator read it, because images are detected from message content parts.vlmmoves fromBaseModelConfigto the trainerModelConfig. The shared RL[model.vlm]now propagates only totrainer.model.vlm.examples/extra/vlm/{sft,sft-moe}.tomlandconfigs/ci/nightly/multimodal_color_codeword.toml: the color-codeword config now has a bare[model.vlm].docs/advanced.md: the family table loses the attr columns, and the wrong "prefix derived fromlanguage_model_attr" sentence is gone.tests/unit/test_configs.py.test_orchestrator_vlm_requires_renderer.test_orchestrator_renderer_none_rejectedalready covers the renderer=None rejection, and the VLM part no longer exists.Migration: delete
vision_encoder_attr/language_model_attrlines and any[orchestrator.model.vlm]block. Old keys now fail with "Extra inputs are not permitted". No compatibility shim, by design.Behavior change
Config-schema only, as described above. Resolved configs were dumped with
model_dump(mode="json")for every TOML underconfigs/,examples/,k8s/(91 entries, with overlays composed on sibling bases). Every difference is a removed key:orchestrator.model.vlm(alwaysnull)No value changed.
Checks
pytest tests/unit/test_configs.py tests/unit/orchestrator tests/unit/train/test_multimodal.py -m "not gpu": 292 passedruff check,ruff format --check: clean[orchestrator.model.vlm]are rejected at parse time (checked by hand).GPU checks (not run): the nightly
configs/ci/nightly/multimodal_color_codeword.tomlRL run, andexamples/extra/vlm/sft.tomlfor a few steps. These are the same model paths that the base PR already validates on GPU.Conflicts
Text-only conflicts:
utils/validation.py: themodel.vlmpropagation)configs/shared.py)shared.py,trainer.py,test_configs.py)trainer.py,validation.py,test_configs.py)trainer.py)No logical conflicts.