Skip to content

Fix convert to original state dict for VLMs #38385

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 27, 2025

Conversation

hiyouga
Copy link
Contributor

@hiyouga hiyouga commented May 26, 2025

What does this PR do?

#37033 introduces the base models for all VLMs. The model weights will be converted by mapping the original keys according to

_checkpoint_conversion_mapping = {
"^visual": "model.visual",
r"^model(?!\.(language_model|visual))": "model.language_model",
}

However, the previous implementation of Transformers could not properly convert the weights back due to existing bugs (replacement = re.sub(r"\(.*?\)", "", pattern) -> replacement = re.sub(r"\(.*?\)", "", replacement)) and the lack of support for nested parentheses

if any(allowed_name in self.__class__.__name__.lower() for allowed_name in VLMS):
reverse_key_mapping = {v: k for k, v in self._checkpoint_conversion_mapping.items()}
original_state_dict = {}
for key, value in state_dict.items():
for pattern, replacement in reverse_key_mapping.items():
replacement = replacement.lstrip("^") # strip off un-needed chars and patterns
replacement = re.sub(r"\(.*?\)", "", pattern)
key, n_replace = re.subn(pattern, replacement, key)
# Early exit of the loop
if n_replace > 0:
break
original_state_dict[key] = value
state_dict = original_state_dict

We want to provide a more accurate weight conversion implementation to prevent issues with third-party apps.
hiyouga/LLaMA-Factory#8147

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@ArthurZucker @zucchini-nlp

@hiyouga hiyouga changed the title Fix convert to original state dict Fix convert to original state dict for VLMs May 26, 2025
Copy link
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

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

thanks

@zucchini-nlp zucchini-nlp enabled auto-merge (squash) May 27, 2025 10:15
@zucchini-nlp zucchini-nlp added the for patch Tag issues / labels that should be included in the next patch label May 27, 2025
@zucchini-nlp zucchini-nlp merged commit 008e0d8 into huggingface:main May 27, 2025
20 checks passed
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

ArthurZucker pushed a commit that referenced this pull request May 27, 2025
* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py
ArthurZucker pushed a commit that referenced this pull request May 27, 2025
* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py
ArthurZucker pushed a commit that referenced this pull request May 27, 2025
* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py
ArthurZucker pushed a commit that referenced this pull request May 27, 2025
* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py
ArthurZucker pushed a commit that referenced this pull request May 28, 2025
* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py
ArthurZucker added a commit that referenced this pull request May 30, 2025
* make it go brrrr

* date time

* update

* fix

* up

* uppp

* up

* no number i

* udpate

* fix

* [paligemma] fix processor with suffix (#38365)

fix pg processor

* [video utils] group and reorder by number of frames (#38374)

fix

* Fix convert to original state dict for VLMs (#38385)

* fix convert to original state dict

* fix

* lint

* Update modeling_utils.py

* update

* warn

* no verbose

* fginal

* ouft

* style

---------

Co-authored-by: Raushan Turganbay <[email protected]>
Co-authored-by: hoshi-hiyouga <[email protected]>
@hiyouga
Copy link
Contributor Author

hiyouga commented May 31, 2025

@zucchini-nlp Thanks for the merging. Could we have a unittest for this function?

@zucchini-nlp
Copy link
Member

I don't think we need a test, since it's fixed already and we probably won't touch that part anymore. We have a test for loading the ckpt and for creating a new dummy model, loading-saving-loading-saving it back. That is enough imo

@hiyouga
Copy link
Contributor Author

hiyouga commented Jun 3, 2025

@zucchini-nlp Okay. By the way, do you think we should override the state_dict and named_parameters method to ensure consistency between the output and the original model weights? For example, OpenR1 uses the named_parameters method to synchronize the model weights with the inference engine.

https://github.com/huggingface/trl/blob/fef915e36f12f759b384e4ab6f650208130aa232/trl/trainer/grpo_trainer.py#L872-L875

vermouth1992 pushed a commit to volcengine/verl that referenced this pull request Jun 7, 2025
### What does this PR do?

Fixes #1710


![image](https://github.com/user-attachments/assets/185d37b6-a4fe-4e89-8eed-72f4477937e8)

1. vLLM 0.9.0 does not support `limit_mm_per_prompt=None`; this
parameter must be a `dict`.
2. Transformers 4.52.* changes the weight keys in the model state dict,
causing mismatches with vLLM's weight loader.

See also:
huggingface/transformers#38385
vllm-project/vllm#19054
vllm-project/vllm#19151

### Test

run `bash examples/grpo_trainer/run_qwen2_5_vl-7b.sh`


![image](https://github.com/user-attachments/assets/b8137c87-f250-40d0-b9c3-c3f44f1a40a1)

### Checklist Before Submitting

- [x] Read the [Contribute
Guide](https://github.com/volcengine/verl?tab=readme-ov-file#contribution-guide).
- [x] Apply [pre-commit
checks](https://github.com/volcengine/verl?tab=readme-ov-file#code-linting-and-formatting).
- [ ] Add `[BREAKING]` to the PR title if it breaks any API.
- [ ] Update the documentation about your changes in the
[docs](https://github.com/volcengine/verl/tree/main/docs).
- [ ] New CI unit test(s) are added to cover the code path.
- [ ] Rely on existing unit tests on CI that covers the code path.
yellowbee686 pushed a commit to yellowbee686/verl that referenced this pull request Jun 10, 2025
### What does this PR do?

Fixes volcengine#1710


![image](https://github.com/user-attachments/assets/185d37b6-a4fe-4e89-8eed-72f4477937e8)

1. vLLM 0.9.0 does not support `limit_mm_per_prompt=None`; this
parameter must be a `dict`.
2. Transformers 4.52.* changes the weight keys in the model state dict,
causing mismatches with vLLM's weight loader.

See also:
huggingface/transformers#38385
vllm-project/vllm#19054
vllm-project/vllm#19151

### Test

run `bash examples/grpo_trainer/run_qwen2_5_vl-7b.sh`


![image](https://github.com/user-attachments/assets/b8137c87-f250-40d0-b9c3-c3f44f1a40a1)

### Checklist Before Submitting

- [x] Read the [Contribute
Guide](https://github.com/volcengine/verl?tab=readme-ov-file#contribution-guide).
- [x] Apply [pre-commit
checks](https://github.com/volcengine/verl?tab=readme-ov-file#code-linting-and-formatting).
- [ ] Add `[BREAKING]` to the PR title if it breaks any API.
- [ ] Update the documentation about your changes in the
[docs](https://github.com/volcengine/verl/tree/main/docs).
- [ ] New CI unit test(s) are added to cover the code path.
- [ ] Rely on existing unit tests on CI that covers the code path.
GitMonkey0 pushed a commit to GitMonkey0/verl that referenced this pull request Jun 14, 2025
### What does this PR do?

Fixes volcengine#1710


![image](https://github.com/user-attachments/assets/185d37b6-a4fe-4e89-8eed-72f4477937e8)

1. vLLM 0.9.0 does not support `limit_mm_per_prompt=None`; this
parameter must be a `dict`.
2. Transformers 4.52.* changes the weight keys in the model state dict,
causing mismatches with vLLM's weight loader.

See also:
huggingface/transformers#38385
vllm-project/vllm#19054
vllm-project/vllm#19151

### Test

run `bash examples/grpo_trainer/run_qwen2_5_vl-7b.sh`


![image](https://github.com/user-attachments/assets/b8137c87-f250-40d0-b9c3-c3f44f1a40a1)

### Checklist Before Submitting

- [x] Read the [Contribute
Guide](https://github.com/volcengine/verl?tab=readme-ov-file#contribution-guide).
- [x] Apply [pre-commit
checks](https://github.com/volcengine/verl?tab=readme-ov-file#code-linting-and-formatting).
- [ ] Add `[BREAKING]` to the PR title if it breaks any API.
- [ ] Update the documentation about your changes in the
[docs](https://github.com/volcengine/verl/tree/main/docs).
- [ ] New CI unit test(s) are added to cover the code path.
- [ ] Rely on existing unit tests on CI that covers the code path.
yellowbee686 pushed a commit to yellowbee686/verl that referenced this pull request Jun 23, 2025
Fixes volcengine#1710

![image](https://github.com/user-attachments/assets/185d37b6-a4fe-4e89-8eed-72f4477937e8)

1. vLLM 0.9.0 does not support `limit_mm_per_prompt=None`; this
parameter must be a `dict`.
2. Transformers 4.52.* changes the weight keys in the model state dict,
causing mismatches with vLLM's weight loader.

See also:
huggingface/transformers#38385
vllm-project/vllm#19054
vllm-project/vllm#19151

run `bash examples/grpo_trainer/run_qwen2_5_vl-7b.sh`

![image](https://github.com/user-attachments/assets/b8137c87-f250-40d0-b9c3-c3f44f1a40a1)

- [x] Read the [Contribute
Guide](https://github.com/volcengine/verl?tab=readme-ov-file#contribution-guide).
- [x] Apply [pre-commit
checks](https://github.com/volcengine/verl?tab=readme-ov-file#code-linting-and-formatting).
- [ ] Add `[BREAKING]` to the PR title if it breaks any API.
- [ ] Update the documentation about your changes in the
[docs](https://github.com/volcengine/verl/tree/main/docs).
- [ ] New CI unit test(s) are added to cover the code path.
- [ ] Rely on existing unit tests on CI that covers the code path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for patch Tag issues / labels that should be included in the next patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants