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

Fix make fix-copies with type annotations #13586

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Fix make fix-copies with type annotations #13586

merged 1 commit into from
Sep 16, 2021

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Sep 15, 2021

What does this PR do?

As pointed out by #13583, there is a bug in the current check_copies utils when a function has a very long signature and a type annotation. This was due to a regex only checking for ): instead of ): or ) -> type:. This PR fixes that and pushes some changes due to divergences that went undetected.

Fixes #13583

@sgugger
Copy link
Collaborator Author

sgugger commented Sep 15, 2021

@patrickvonplaten Can you double-check the changes in HuBERT are good?

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 16, 2021

@sgugger I pulled this PR and found there is 1 edge case remained:

def serving_output(self, output: TFBaseModelOutputWithPooling) -> TFBaseModelOutputWithPooling:

Change the above line to

def serving_output(self, output: TFBaseModelOutputWithPooling):

or

def serving_output(self, inputs, output: TFBaseModelOutputWithPooling) -> TFBaseModelOutputWithPooling:

or

def serving_output(self, output2: TFBaseModelOutputWithPooling) -> TFBaseModelOutputWithPooling:

Currently (with the work in this PR), these changes won't be copied to

def serving_output(self, output: TFBaseModelOutputWithPooling) -> TFBaseModelOutputWithPooling:

@patrickvonplaten
Copy link
Contributor

Ok for HuBERT

@sgugger
Copy link
Collaborator Author

sgugger commented Sep 16, 2021

@ydshieh This is not a edge case but the way the check copies utility has been implemented. It does not check for the class name, function name, just the code inside. This is why the introduction line is ignored in the check.

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 16, 2021

@sgugger , Thanks, I understand now. However, I personally feel this is a bit strange. In my work on encoder-decoder, I need to add cross-attention and cache mechanism to some models, and these involve changing some return value types.
Like TFBaseModelOutput to TFBaseModelOutputWithPastAndCrossAttentions.

And the way check copies is implemented won't copy this kind of changes (single-line function name & signature) as shown in the provided example.

Moreover, if the function name & signature have multiple lines, the added or removed parameters will be copied (suppose they are not in the same line as the function name). This is somehow inconsistent with the single-line case above.

I will let Hugging Face to decide if it is necessary to address this :)

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

I think the issue you mention @ydshieh is a real issue but appears in very rare cases - I think while it would be nice to update it, it does require a significant rewrite for a less significant gain.

@sgugger sgugger merged commit 88dbbfb into master Sep 16, 2021
@sgugger sgugger deleted the fix_check_copies branch September 16, 2021 15:55
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 13, 2022
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.

fix-copies doesn't work well in some cases
4 participants