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

[torch.compile] Graph break+warning instead of error in _prepare_generation_config #31297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

williamwen42
Copy link

I ran into this error when attempting to torch.compile miniCPM (https://huggingface.co/openbmb/MiniCPM-Llama3-V-2_5).

I tracked changes back to this PR #29443 and I think that we should be graph breaking (i.e. going ahead with copy.deepcopy) and issuing a warning in the case we need to, instead of crashing. But in the case where the copy doesn't need to be done, we can skip making the call in order to prevent the graph break.

cc @gante

@huggingface huggingface deleted a comment from github-actions bot Jul 8, 2024
Copy link

github-actions bot commented Aug 2, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@gante
Copy link
Member

gante commented Aug 2, 2024

Hi @williamwen42 👋 Thank you for opening this PR

The exception serves to nudge the users towards an equivalent solution that works, instead of enabling the code to run suboptimally (i.e. with a graph break).

Is there a use case that is prevented by the current exception that isn't solved by moving arguments to a GenerationConfig?

@williamwen42
Copy link
Author

williamwen42 commented Aug 2, 2024

No use case off the top of my head, but I was trying to compile a publicly available model (not mine) and ran into the crash. I think warning + graph break is a better way to get things working, but also lets users know that they might want to consider changing code.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

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