-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Automatic compilation in generate: do not rely on inner function #34923
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
a05c636
compiled forward in PreTrainedModel
Cyrilvallez 5c38f2b
update
Cyrilvallez 3348e9e
style
Cyrilvallez 0627183
update name
Cyrilvallez 77751bb
trigger CIs
Cyrilvallez a430aba
Add way to use custom compile args
Cyrilvallez f027913
style
Cyrilvallez 94f8b54
switch parameterization to generation_config
Cyrilvallez 8c525e3
Add to inits
Cyrilvallez f5cdb1f
Update configuration_utils.py
Cyrilvallez 8ab5ba1
inits
Cyrilvallez d5120ea
style
Cyrilvallez 9e21fca
docs
Cyrilvallez cf33f2c
style
Cyrilvallez 622edbf
Update configuration_utils.py
Cyrilvallez 01e004e
back without dataclass for repo consistency
Cyrilvallez 6c1e29e
Update configuration_utils.py
Cyrilvallez 6fd25b4
style
Cyrilvallez 33d335a
style
Cyrilvallez a0a7ef8
style once again
Cyrilvallez a6c8cf1
add config serialization
Cyrilvallez c894edc
update
Cyrilvallez 162bbe0
true dataclass
Cyrilvallez bb91b55
trigger CIs
Cyrilvallez d433b38
merge compile methods + remove serialization of compile config
Cyrilvallez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am wondering if it would (probably) make more sense to do
at this line and delegate the actually compile only in
compiled_call
(as you already there) - when it is called (like you do ingenerate
).Having 2 places to call
torch.compile
seems a slight strange to me (but it's ok though).And if you decide to take my suggestion, probably the name
_set_compile_call
should be changed to_set_compile_config
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.
torch.compile does not compile, calling the compile funciton does compile
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 know. But what I mean here is not the underlying compile happening, but rather the call to
torch.compile
twice. 2 different methods callingtorch.compile
seems to me not very good style (for me, it's better if one method is only set config and another one take the job to calltorch.compile
)But it's no big deal but just of a habit of making each method does its own job.
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.
You're right, but maybe then using
_set_compile_call
incompiled_call
instead? That way,set_compile_call
is the only place usingtorch.compile
, andcompiled_call
is still only used for accessing the fnction? (with drawback call to_set_compile
in case it does not already exist)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.
even with that, in
generate
we still need to call_set_compile_call
thencompiled_call
right? (ascompiled_call
) doesn't contain the argument. If this is the case, then it's odd to havecompiled_call
calling_set_compile_call
.Otherwise, if you think it's ok/good to change
compiled_call
to accept compile_config arugment, then sound good to me with your suggested change.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.
oh,
compiled_call
is a property so not to take argument .... so the concern in the first paragraph in the above comment is there.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.
anyway, it's implementation details and not affecting users. Don't take it too serious if the changes will take too much time.
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.
Yes, I thought it makes sense to make it a property so that
model.compiled_call(**inputs)
could always be used directly as an alternativemodel.forward(**inputs)