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

Handle when backend is also in compile_kwargs #6502

Merged
merged 10 commits into from
Oct 9, 2024

Conversation

oraluben
Copy link
Contributor

@oraluben oraluben commented Sep 7, 2024

@tohtana
Copy link
Contributor

tohtana commented Sep 9, 2024

Hi @oraluben, Can you elaborate your goal with this PR?

@oraluben
Copy link
Contributor Author

oraluben commented Sep 9, 2024

Hi @oraluben, Can you elaborate your goal with this PR?

Sure! In the case where "backend" is also in compile_kwargs dict, the current version fails with an error that "backend" arg is provided twice. In this PR, I use the backend arg to override possible backend value in compile_kwargs.

An example is huggingface/accelerate#3069, I have to pop backend before pass compile_kwargs to engine.compile

@tohtana
Copy link
Contributor

tohtana commented Sep 9, 2024

Hi @oraluben,
Thank you for the clarification. It makes sense. At this point, it's not very clear how we benefit from treating backend separately from other compiler options, but we want to keep the option for compatibility.

I agree with the fix, but the method for merging dictionaries is not correct, as the tests have failed. Could you address this?
It would also be good if we could show a warning when backend is specified in both ways and one of them is overwritten.

@oraluben
Copy link
Contributor Author

Is there anything wrong with the CI? My PRs keep timeout in v100.

@loadams
Copy link
Contributor

loadams commented Sep 16, 2024

Is there anything wrong with the CI? My PRs keep timeout in v100.

Yes this looks to be an issue on our side, I will take care of this.

@tjruwase tjruwase requested review from GuanhuaWang and removed request for tjruwase and tohtana September 20, 2024 21:21
@tohtana tohtana self-requested a review September 27, 2024 19:06
@tohtana tohtana assigned tohtana and unassigned tohtana Oct 1, 2024
@tohtana tohtana enabled auto-merge October 1, 2024 17:19
@tohtana tohtana added this pull request to the merge queue Oct 8, 2024
Merged via the queue into microsoft:master with commit ca8b1fe Oct 9, 2024
13 checks passed
@oraluben oraluben deleted the patch-1 branch October 9, 2024 06:21
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.

4 participants