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

adding script to fix the style and adding modified/fixed files #591

Closed
wants to merge 1 commit into from

Conversation

ssusie
Copy link
Collaborator

@ssusie ssusie commented Apr 11, 2024

No description provided.

@ssusie ssusie force-pushed the ssusie-pylint branch 6 times, most recently from f99c315 to 8e44502 Compare April 12, 2024 03:14
.github/workflows/PyLintFixOnPush.yaml Outdated Show resolved Hide resolved
.github/workflows/UnitTests.yml Outdated Show resolved Hide resolved
@ssusie ssusie force-pushed the ssusie-pylint branch 4 times, most recently from 86163f2 to d7da974 Compare April 12, 2024 19:45
Copy link
Collaborator

@khatwanimohit khatwanimohit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@rwitten rwitten left a comment

Choose a reason for hiding this comment

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

@gobbleturk can you own this one?

@rwitten rwitten assigned gobbleturk and unassigned rwitten Apr 15, 2024
Copy link
Collaborator

@gobbleturk gobbleturk left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you! MaxText is becoming more professional every day =)

Please wait for @rwitten thoughts on changing line length from 125 to 80, I vaguely recall we purposefully changed from 80 to 125 at some point early on

MaxText/generate_param_only_checkpoint.py Outdated Show resolved Hide resolved
@@ -249,7 +254,10 @@ generated-members=
[FORMAT]

# Maximum number of characters on a single line.
max-line-length=125
max-line-length=80
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is fine, although a significant change. I am very appreciative that the provided script should wrap the lines for me. @rwitten do you have a preference here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the quick review!

I chose line length=80 because that is the google standard. I can ping the reference offline. On the other hand PEP8 standard is 79 https://peps.python.org/pep-0008/#maximum-line-length

code_style.sh Outdated Show resolved Hide resolved
@gobbleturk
Copy link
Collaborator

@gobbleturk can you own this one?

Reviewed and LGTM, but I think you should confirm the line length change from 125 -> 80

@ssusie
Copy link
Collaborator Author

ssusie commented Apr 16, 2024

Rafi wants 125 as the max line length and files here are fixed with 80. I am closing this PR in favor of #596 .

@ssusie ssusie closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants