-
Notifications
You must be signed in to change notification settings - Fork 244
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
Upgrade pytype #801
Upgrade pytype #801
Conversation
I am not sure about the solution inside context manager related fix, they are all related with |
.pre-commit-config.yaml
Outdated
@@ -78,7 +78,7 @@ repos: | |||
name: pytype | |||
language: system | |||
types: [python] | |||
entry: "bash -c 'pytype -j ${NUM_CPUS:-auto}'" | |||
entry: "bash -c 'pytype ./ --keep-going -j ${NUM_CPUS:-auto}'" |
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.
I wouldn't be surprised if using ./
causes problems relative to specifying the partcular source directories (I see pytype is erroring out with multiple rules generate /imitation/.pytype/pyi/imitation/scripts/parallel.pyi
now)
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.
Yes, it is weird. I can take a further look into it later. But before that, does the other changes look good to you? If not, I think we can probably keep the original pytype version first...
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.
The other changes, i.e. suppressing the errors due to pytype not understanding Sacred capture functions with type ignores, are fine.
What happens if you just revert the change on this line? Does it still have the same pytype error?
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.
The new pytype version requires specifying the file or directory explicitly, so ./
part is required. The --keep-going
part means: "Keep going past errors to analyze as many files as possible.". So reverting that part wouldn't help resolve this issue.
okay, I will look into SSH and check its detail.
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.
The new pytype version requires specifying the file or directory explicitly
New pytype version looks in pyproject.toml
rather than setup.cfg
for config, so our existing config including list of source directories was being ignored. I've ported it now, and this resolved the problem. Running in .
is not equivalent to running separately for src/
, tests/
, etc so it makes sense there'd be a difference but I'm not sure quite what the origin of the error was. There are multiple scripts called parallel.py (e.g. there's a joblib/parallel.py in my venv), so it's possible pytype
pulled in one of those and got confused.
Description
Originally from this commit to remove workaround for old versions. But it turns out we need to address many more type issues than expected. So I moved pytype related fix from #785 to here.
Testing
pytype --version && pre-commit run --all pytype
shows no error locally. Not sure why it makes CI red for now.