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

Add style check #3

Closed
wants to merge 14 commits into from
Closed

Add style check #3

wants to merge 14 commits into from

Conversation

ayushdg
Copy link
Collaborator

@ayushdg ayushdg commented Mar 18, 2024

The aim of the PR is to add pre-commit configs and a GHA for style checks.
The PR does not attempt to fix all the style failures present in the repo. That will be done in a followup PR.

The config ends up using isort, black & flake8 (some of it included in the contributing guide).

Happy to also explore ruff which should be a much faster drop in replacement for some of the tools mentioned above and has been adopted by many large python projects.

@ayushdg ayushdg marked this pull request as ready for review March 19, 2024 21:53
@ayushdg ayushdg requested a review from ryantwolf March 19, 2024 21:56
@ayushdg ayushdg marked this pull request as draft March 19, 2024 22:26
@ryantwolf
Copy link
Collaborator

Looks like the .pre-commit-config.yaml of both NeMo Aligner and NeMo are slightly different than ours. Can we change ours to match theirs exactly, or is there a reason ours should be different?

@ayushdg
Copy link
Collaborator Author

ayushdg commented Mar 20, 2024

Looks like the .pre-commit-config.yaml of both NeMo Aligner and NeMo are slightly different than ours. Can we change ours to match theirs exactly, or is there a reason ours should be different?

There's no reason for them to match exactly, some of it depends on the practices we want to follow for the repo so it's good to go over the differences:

  1. pre-commit-hooks: It makes sense to include the hooks included in these libraries, but probably makes sense to keep the additional Trailing-whitespace, end-of-file-fixer unless we find specific examples in the repo where it doesn't make sense.
    Full list of hooks here.
  2. Black: Pretty much identical
  3. Isort: I'd argue that adding the black profile should be the case since we're using black as recommended by isort.
  4. Flake8: This is personal preference, a lot of projects prefer some level of linting flake8 is one option that combines rules from PyCodeStyle and PyFlakes. black formatting arguably fixes some of these rules, but happy to omit this, don't have a strong preference.
  5. ci: I'm happy to explore using pre-commit.ci over the pre-commit action directly provided it doesn't require any additional enterprise approvals.

@ryantwolf
Copy link
Collaborator

Ok cool. Thanks for the clarification.

  1. Yup let's add the ones they use and keep the ones you already have.
  2. Good.
  3. Just looked into this tool. I like it so let's keep it too haha. Do we need to exclude any directories like aligner does?
  4. I'm fine with keeping it so long as we don't run into any issues.
  5. Yeah if you could look into that that could be good. Just to keep things in line with the rest of the repos. If it ends up being a hassle though let me know and we can stick with this.

@ayushdg
Copy link
Collaborator Author

ayushdg commented Mar 20, 2024

Isort: I'd argue that adding the black profile should be the case since we're using black as recommended by isort.

Actually I stand corrected, other repos do use this but specify the config in their pyproject.toml file. I'll probably do the same.

@ayushdg
Copy link
Collaborator Author

ayushdg commented Mar 20, 2024

This also has the side effect of pre-commit.ci auto formatting all the files. @ryantwolf Are you okay updating all the file formats in this PR?

@ryantwolf
Copy link
Collaborator

It had to happen at some point haha. Let's reformat the code 🫡

@ayushdg ayushdg marked this pull request as ready for review March 20, 2024 23:12
ryantwolf and others added 14 commits March 21, 2024 15:36
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
for more information, see https://pre-commit.ci

Signed-off-by: Ayush Dattagupta <[email protected]>
Co-authored-by: Ryan Wolf <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
This PR adds a short README.md file for the TinyStories tutorial, with
instructions on how to run it.

Signed-off-by: Mehran Maghoumi <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
* Add workflow for cpu pytests

Signed-off-by: Ayush Dattagupta <[email protected]>

* Install wheel to fix fasttext import

Signed-off-by: Ayush Dattagupta <[email protected]>

* Omit python 3.8, do not fast fail

Signed-off-by: Ayush Dattagupta <[email protected]>

* Check if updating setuptools/pip changes cython errors

Signed-off-by: Ayush Dattagupta <[email protected]>

* Explicitly install cython

Signed-off-by: Ayush Dattagupta <[email protected]>

* Try freeing up space before install

Signed-off-by: Ayush Dattagupta <[email protected]>

* Try rapids_no_initialize

Signed-off-by: Ayush Dattagupta <[email protected]>

* remove python 3.9 testing for now

Signed-off-by: Ayush Dattagupta <[email protected]>

---------

Signed-off-by: Ayush Dattagupta <[email protected]>
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Good with me. Just fix the DCO error by using the --signoff option on your commits.

@ayushdg ayushdg closed this Mar 21, 2024
@ayushdg ayushdg force-pushed the ayushdg95/ci/style-check branch from a8980a5 to f607633 Compare March 21, 2024 22:40
@ayushdg
Copy link
Collaborator Author

ayushdg commented Mar 21, 2024

Had some weirdness during the rebasing with signoff. Will open a new PR

@ayushdg ayushdg deleted the ayushdg95/ci/style-check branch March 25, 2024 19:48
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.

3 participants