-
Notifications
You must be signed in to change notification settings - Fork 31.3k
🚨 Delete generation params from model config #41695
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
🚨 Delete generation params from model config #41695
Conversation
|
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. |
| url = "https://qianwen-res.oss-cn-beijing.aliyuncs.com/Qwen-Audio/glass-breaking-151256.mp3" | ||
| audio, sr = librosa.load(BytesIO(urlopen(url).read()), sr=processor.feature_extractor.sampling_rate) | ||
| inputs = processor(text=prompt, audios=audio, return_tensors="pt").to(model.device) | ||
| inputs = processor(text=prompt, audio=audio, return_tensors="pt").to(model.device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Audio renaming is not related, accidentally slipped from another branch. I'll rebase main after #41681 is merged
|
cc @vasqu maybe since Joao left and I need someone to help merge generation PRs. This one particularly is blocking me. I'd love to clean-up and break BC for v5, since generation config is too complicated at this point This PR particularly splits generation config away from model config, a dependency we had for a few ages due to legacy reasons. It's time to move on I think and raise a NOTE: bad formatted configs from hub can still be loaded for BC |
vasqu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in general aligned with this, just have a few nits/questions just to be sure.
Don't forget
- the bad audio(s)
- v5 tag and add it to the issue
Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
vasqu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, lgtm
Time to merge with main and check conflicts 👀 just the same things as I said before re audios + v5
|
[For maintainers] Suggested jobs to run (before merge) run-slow: bart, encoder_decoder, mvp, rag, speech_encoder_decoder, udop, vision_encoder_decoder |
|
Rebased main, now the diff has no |
What does this PR do?
Disentangles config and generation config. Now the model's config will throw an error if generation params are found. To keep BC, we'll still be loading models from the hub where generation params are in config, but we'll manually drop these params from being set as an attribute.