Conversation
aphedges
requested review from
RezaYazdaniAminabadi,
ShadenSmith,
awan-10,
cli99,
conglongli,
eltonzheng,
jeffra,
minjiaz,
niumanar,
samyam and
tjruwase
as code owners
|
This is awesome, thank you @aphedges! |
|
You're welcome! I was inspired by #1598, which solved an issue I had noticed and was also bothering me. On a related note, how do you feel about some simple other checks being added? For example, I was thinking of setting up flake8 (with disabled formatting checks) because it's pretty lightweight (runs in only a couple of seconds on this repository) and would have probably caught #1601. |
I think we'd definitely be open to flake8 with some of the more strict formatting guidelines disabled. I do think it will find a lot of issues in our codebase. That being said it would be awesome to address the issues flake8 sees and would improve the DeepSpeed codebase significantly! For example, I just ran it on |
|
I just tested yapf v0.31.0 locally and I think the changes it suggests are fine, can you bump that version as well while we are at it? |
jeffra
left a comment
jeffra
left a comment
There was a problem hiding this comment.
Wonderful contribution, thank you :)
|
When I ran flake8 on the entire codebase with all formatting options disabled, I think I still saw hundreds of issues. I will definitely take a stab at it, and I was planning on a separate PR. I might even do several PRs, depending on the number of changes that will need to be made. I'll make the yapf bump now. It should be up in a couple of minutes. You're welcome! Glad to help! :) |
2 participants
I was working on another PR when I realized these commit hooks are outdated and could use some improvement. I made the following changes:
pre-commit-hooksandpre-commit-cppcheck-json, which detected invalid syntax in a JSON filehjsoninstead ofjson. However, the Hjson syntax specification says Hjson files should use the.hjsonextension. If the invalid syntax was intentional, then the extension should probably change as well.fix-encoding-pragma, which removed redundant pragmas because UTF-8 is already the default encoding for Python 3 (PEP 3120)requirements-txt-fixerto keep the requirements files sortedexclude: "DeepSpeedExamples/"lines were redundant.pre-commit-hooksthat seemed relevant and useful but didn't fix any immediate issuesIn terms of performance, these changes add under 3 seconds to the run time on my Mac, so I don't think this will be a big compute burden to add.
I did not update yapf because the new version changes the formatting in multiple places, and I don't know how you feel about that.