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

Run pytype on pull requests #1364

Closed
Anthchirp opened this issue Aug 2, 2020 · 1 comment
Closed

Run pytype on pull requests #1364

Anthchirp opened this issue Aug 2, 2020 · 1 comment
Assignees

Comments

@Anthchirp
Copy link
Member

As laid out in #1357 I've played a bit with static type checking using pytype.

pytype takes a lot of time to run on the code, so I'm not suggesting to run this as a pre-commit. However we can run it as a GitHub action. I've made a script that can validate pull requests and check for newly introduced errors only and add warnings via annotations:
image

This would allow us to start using pytype today without having to put in massive amounts of work (or thousands of # ignore comments).

In addition to the example I made in the mypy-ticket I've picked out some more example alerts below so if you want you can have a look at what sort of stuff this can catch.
Note: This was all run on unmodified source code - so no added extra annotations. In fact none of the code I looked at below had type declarations, so all these warnings come from type inference. When we add type declarations this will of course make pytype more powerful, but I'd say it's already quite useful as it is.

There are also false positives, mostly caused by imports of unknown (and therefore: uninferrable) packages. We'll have to see how bad this is in real life - in any case the mergability of pull requests will not depend on the pytype action passing.

dxtbx:

38 out of 320 files were scanned
pytype found 30 issues in the code base

format/FormatPilatusHelpers.py:207:1:attribute-error in determine_eiger_mask: No attribute 'module_size_fast' on None

control flow catches a None input and runs an fallback function - and assumes that will always return something other than None

libtbx_refresh.py:31:1:wrong-arg-types in <module>: Function imp.find_module was called with the wrong arguments

called with an iterable, but Python spec says it should be a list

util/dlsnxs2cbf.py:182:1:unsupported-operands in make_cbf: unsupported operand type(s) for +: 'bytearray' and 'bytes'

looks like it works, but suggests that casting the bytearray to bytes (or removing bytearray entirely) would be safer

dials:

186 out of 743 files were scanned
pytype found 363 issues in the code base

algorithms/refinement/target.py:114:1:ignored-metaclass in <module>: Metaclass abc.ABCMeta on class Target ignored in Python 3

overlooked in Python 3 conversion

algorithms/symmetry/cosym/target.py:299:1:attribute-error in _compute_rij_wij: No attribute 'todense' on None

loop construction assumes you never run on an empty list

util/rebin_images.py:78:1:unsupported-operands in write_image: unsupported operand type(s) for +: 'str' and 'bytes'

overlooked in Python 3 conversion

util/image.py:22:1:attribute-error in get_data: No attribute 'cbf_handle' on reader

the problem with pseudo-classes is that they are built on assumptions

util/version.py:68:1:wrong-arg-types in get_git_version: Function bytes.count was called with the wrong arguments

overlooked in Python 3 conversion

report/plots.py:494:1:attribute-error in second_moments_plot: No attribute 'binner' on None

check for 'None' is done indirectly and unnecessarily calls an external function

xia2:

105 out of 556 files were scanned
pytype found 27 issues in the code base

command_line/to_shelx.py:212:1:import-error in to_shelx: Can't find module 'dxtbx.model.experiment.experiment_list'.

ohai lack of testing?

command_line/get_image_number.py:21:1:attribute-error in image_path_obtainer: No attribute 'groups' on None

catches an implicit assumption which is not guaranteed to be true

Modules/DeltaCcHalf.py:50:1:attribute-error in __init__: No attribute 'eliminate_sys_absent' on None

loop construction assumes you never run on an empty list

@Anthchirp
Copy link
Member Author

Dials core meeting discussion outcome:

  • Sounds useful
  • Have to monitor the output on pull requests, we don't want this to annoy us by regularly taking away the green tick

I will prepare a pull request.

Anthchirp added a commit that referenced this issue Sep 17, 2020
See discussion in https://dials.github.io/kb/core/20200917 and #1364
for the motivation and background.
Anthchirp added a commit that referenced this issue Sep 22, 2020
Fix for pull request annotations, draw in a few 3rd party libraries
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

No branches or pull requests

1 participant