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
| 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.
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.
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.
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.
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.
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 pytypeshows no error locally. Not sure why it makes CI red for now.