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

Improve generate docstring (for TF and FLAX) #18432

Closed

Conversation

JoaoLages
Copy link
Contributor

What does this PR do?

Just a continuation PR of #18198 for TF and FLAX code

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).

Who can review?

@sgugger @gante

@JoaoLages JoaoLages mentioned this pull request Aug 2, 2022
1 task
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for expanding to the other generate functions!

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 2, 2022

The documentation is not available anymore as the PR was closed or merged.

@sgugger
Copy link
Collaborator

sgugger commented Aug 2, 2022

Looks like we just need a quick make style to be ready to merge :-)

@sgugger
Copy link
Collaborator

sgugger commented Aug 2, 2022

Mmm no the formatting has done something very wrong here, we can't have that. There is likely some syntax error in the docstring.
Problem looks to be in the input_ids argument at line 422 of the TF generation files, the type should all be on one line.

@JoaoLages
Copy link
Contributor Author

Mmm no the formatting has done something very wrong here, we can't have that. There is likely some syntax error in the docstring. Problem looks to be in the input_ids argument at line 422 of the TF generation files, the type should all be on one line.

make extra_style_checks does that automatically. What do you propose?

@sgugger
Copy link
Collaborator

sgugger commented Aug 2, 2022

As I said, there is a syntax error in the docstring that makes the styling script behave erratically. The first step is to revert the changes, fix the syntax error then re-run it.

@JoaoLages
Copy link
Contributor Author

I changed this line

 input_ids (`tf.Tensor` of shape `(batch_size, sequence_length)`, `(batch_size, sequence_length,
            feature_dim)` or `(batch_size, num_channels, height, width)`, *optional*):

to this

 input_ids (`tf.Tensor` of shape `(batch_size, sequence_length)`, `(batch_size, sequence_length, feature_dim)` or `(batch_size, num_channels, height, width)`, *optional*):

but then make extra_style_checks reverts the change

@sgugger
Copy link
Collaborator

sgugger commented Aug 2, 2022

Ah yes, just tried locally and it's due to the empty line between Parameters: and input_ids. If you remove it, then your changes should not be overwritten.

@JoaoLages
Copy link
Contributor Author

Ah yes, just tried locally and it's due to the empty line between Parameters: and input_ids. If you remove it, then your changes should not be overwritten.

Nice catch, but it still does look strange in the docs 🤔

image

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

You did not revert the bad changes made by the style doc string, that's why. I showed you what needs to happen on the first arguments (basically the type has to be on its own line ending with a colon) but the easiest is probably to start from the commit before the style changes.

JoaoLages and others added 2 commits August 3, 2022 12:17
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
JoaoLages and others added 7 commits August 3, 2022 13:33
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@gante
Copy link
Member

gante commented Aug 3, 2022

Uhmm, there is something wrong with the automatic styler -- e.g. the pytorch generate file should not be touched at all in this PR. As Sylvain wrote, the easiest solution is to start from a new branch 🤔

@JoaoLages
Copy link
Contributor Author

Uhmm, there is something wrong with the automatic styler -- e.g. the pytorch generate file should not be touched at all in this PR. As Sylvain wrote, the easiest solution is to start from a new branch 🤔

The docs seem fine now, right? It's just that this test, doc-builder style src/transformers docs/source --max_len 119 --check_only --path_to_docs docs/source, is not allowing to have more than 119 characters per line, but we need it here.

@sgugger
Copy link
Collaborator

sgugger commented Aug 3, 2022

Nope, the docs are not fine for the PyTorch side with all the changes in this PR (and as @gante mentioned that file should not be touched at all). The doc-style is completely comfortable with lines that are more than 119 chars when it identifies they are parameter introduction lines, you just needed to remove the blank line between Parameters: and the first argument in generate.

@JoaoLages
Copy link
Contributor Author

Ah right, it has these strange hyphens...
image

I will close the PR then, let's disregard these changes.

@JoaoLages JoaoLages closed this Aug 3, 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.

4 participants