Skip to content
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

[Model Support] Qwen2 update #849

Merged
merged 1 commit into from
Jun 7, 2024
Merged

[Model Support] Qwen2 update #849

merged 1 commit into from
Jun 7, 2024

Conversation

wheresmyhair
Copy link
Collaborator

@wheresmyhair wheresmyhair commented Jun 7, 2024

Description

Support Qwen2 models.
Only docs are updated, since the pipeline requirements are the same as Qwen1.5.

Pipeline Tests

  1. Full-Finetune
    full

2 LoRA
lora

Known Issue

Note: This isn't a bug in LMFlow, but we will add a logger notification ASAP to notify users when they might trigger this bug.

When do lora (or other peft tuning that uses peft library) first and saved model at a dir, say, A, and then do another finetuning work that also specifies the same output_dir A, pipeline will fail to update the model card, since Qwen2ForCausalLM doesn't have attribute .create_or_update_model_card(). But this will not affect the model saving.

Bug logi:

  1. Finetune with PeftTrainer and save leads to a modelcard with library_name = 'peft'.
  2. When do anyother finetune with same output_dir, since
    https://github.com/huggingface/transformers/blob/bdf36dcd48106a4a0278ed7f3cc26cd65ab7b066/src/transformers/trainer.py#L4114, os.path.exists(model_card_filepath) is True, and then is_peft_library is True.
  3. At https://github.com/huggingface/transformers/blob/bdf36dcd48106a4a0278ed7f3cc26cd65ab7b066/src/transformers/trainer.py#L4143, transformers will do .create_or_update_model_card(), which Qwen2ForCausalLM doesn't have.

qwenbug

We strongly recommend to use different output_dir for every finetuning work to avoid unexpected issues like the one above.

@wheresmyhair wheresmyhair marked this pull request as ready for review June 7, 2024 04:26
Copy link
Contributor

@research4pan research4pan left a comment

Choose a reason for hiding this comment

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

LGTM

@research4pan research4pan merged commit be54124 into main Jun 7, 2024
2 checks passed
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.

2 participants