-
Notifications
You must be signed in to change notification settings - Fork 19
Add quality check to CI and fix existing errors #408
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey fynn, thanks for the contribution, leaving a few comments. if you're looking for additional places to contribute, we can help point you to some PRs/issues that could use some attention
@@ -282,7 +282,6 @@ def disable_hf_hook(module: torch.nn.Module): | |||
hooks = {} | |||
|
|||
def collect_hooks(module): | |||
nonlocal hooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this will break anything, hooks can be kinda finicky. cc @kylesayrs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused the following flake8 error:
F824 `nonlocal hooks` is unused: name is never assigned in scope
Essentially, nonlocal
/global
are needed if we assign (bind) a new value to the variable (e.g. hooks = {"new_dict": 0}
). However, in collect_hooks
we only access the variable hooks
(the hook[module] =
desugars to a method call), which can be done without the nonlocal
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, your justification looks right, this looks more like unnecessary code than weird logic needed for hooks
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple nits on the workflow yaml, otherwise LGTM
@@ -282,7 +282,6 @@ def disable_hf_hook(module: torch.nn.Module): | |||
hooks = {} | |||
|
|||
def collect_hooks(module): | |||
nonlocal hooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, your justification looks right, this looks more like unnecessary code than weird logic needed for hooks
Signed-off-by: Fynn Schmitt-Ulms <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thanks for updating
This pr adds a
python-style
check totest-check.yaml
which enforces the repo style rules.It tests new code by running the existing
make quality
check which tests:Commit 7b2f3bc makes the GitHub workflow change and commits c8b26be and a4e3b48 handle issues with the auto-generated
version.py
file which gets created on package install (and is not included in git repo).The remaining commits implement all the fixes to existing "quality issues" in the repo. I did my best to make minimal changes when correcting these.