Repository navigation
Conversation
💊 Build failures summary and remediationsAs of commit 71c7f63 (more details on the Dr. CI page):
XLA failureJob pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 55 times. |
|
Hmm that did its job, it found one issue: Fixed in last commit, still figuring out why I didn't see that locally. |
|
Ah it makes more sense now, I ran It would be better to move the CI check somewhere that validates an installed |
|
There's also issues that show up depending on file or the set of files you're running. Example: There's a |
|
Oh fun, the JIT and MyPy disagree on what a valid type annotation should look like: without EDIT: the good news is that that's the only test failure. |
|
The main thing to decide now is what the environment should be for running
My suggestion would be to make this work on a I'd prefer to start with: and start enabling those options gradually. Again to increase cycle time and not deal with everything at once. Regarding best CI job, not sure. |
|
There's about 220 errors left in I'm not sure it's worth spending time on that now, I think just blanket ignores for |
|
cc @malmaud |
Going forward, we can eliminate these problems by inlining the stubs into the py file. Also something that can be done incrementally (except for the |
Let's disable this test and add another JIT issue for it. |
I agree with develop. However, that means that having the mypy job in quickcheck is not good because now we have to build PyTorch first before we can run checks; it's probably better to roll up the mypy check with an existing CI job. Maybe we should just run mypy as part of ALL the jobs (which means that you'll end up covering the CPU and the CUDA cases). In an ideal world, the set of bindings we make available would be invariant no matter what build configuration you are in, so no matter if you build with some build option or without, mypy will typecheck the same. I don't know how true that is today.
I agree, blanket ignoring sounds the best. |
|
Changes in the code LGTM, looking forward to merging this! |
|
It LGTM, but I only skimmed it so don't hold me to anything :) |
Note: this will likely fail in CI, due to the reason given in the test_doc_examples comment.
|
Okay, adding it to the test suite as The one XLA build failure is unrelated. |
This is OK, but now you have to rebuild the docker images per the instructions at https://github.com/pytorch/pytorch/wiki/Docker-image-build-on-CircleCI I'm going to land this anyway for now but it will mean that our actually deployed images are out of sync with what the script says they are. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Will do. The tests already ran in CI though, so there may be another (unpinned probably) |
That was in I can pin it in |
Yeah let's pin it. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Looks like there are some internal failures that are causing problems. First, it looks like the Caffe2 test files cannot validly use the dot-qualified import as in internal CI their PYTHON_PATH is setup strangely: The second weirdness is that these changes have started causing some of our internal typechecking builds (using our internal typechecker called pyre, don't ask...) to start twinging. Here are two occurrences: code looks like: and context I can get in touch with the downstream teams here to see what appropriate remediations are. Some other ones: |
|
I should have some time tomorrow to look a little, but not too much. If some of these you can predict fixes for without further info, that would be great. |
Hmm, I'd expect more such conflicts to occur when people start tackling the remaining O(1000) mypy errors. I just tried Looking into the errors now. |
Those were correct, but due to a weirdness in how PYTHONPATH is set up in caffe2's internal CI, that caused an issue. Mypy on the other hand complains about absolute imports, so go for fully-qualified imports here.
This looks like a MyPy bug, python/mypy#2608 (EDIT: I assumed pyre has the same bug, will see if I can narrow that down). One workaround is to add The second error ( The rest of the errors also look similar. @ezyang is it doable to add those asserts in the code that's throwing errors? I don't see a good workaround within PyTorch. EDIT 2: Pyre has a similar open issue (meta/pyre-check#197), except they disagree it's a bug - even with the The annotations within |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Good news, I got in touch with the upstream authors here and they will fix their code (or agreed to let me pyre-fixme it for now), so no changes needed in PyTorch. Pinning pyre isn't going to do much good since FB is running on bleeding edge Pyre. Though, perhaps we should also start running Pyre in open source. Not for this PR though. |
gives the same errors with both Uncommenting the first if-statements resolves those errors: and mypy and pyre are both happy, even though that code will crash if you run it. Versions: Uncommenting the second if-statement makes no difference for type checking. Using shows:
with mypy. And with pyre: The pyre error can indeed be suppressed with: Now for the second workaround: does not work (with I also tried pyre tl;dr use |
Great! I was just in the middle of posting my giant comment above, glad we came to the same conclusion. |
|
@rgommers BTW, I heard we can get |
|
Don't worry about the merge conflict, I will take care of it internally |
Summary: This PR helps with #50513 by reducing the complexity of our `mypy` test suite and making it easier to reproduce on the command line. Previously, to reproduce how `mypy` was actually run on tracked source files (ignoring the doctest typechecking) in CI, you technically needed to run 9 different commands with various arguments: ``` $ mypy --cache-dir=.mypy_cache/normal --check-untyped-defs --follow-imports silent $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/module_list.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/namedtuple.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/opt_size.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/size.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/tensor_copy.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/torch_cuda_random.py $ mypy --cache-dir=.mypy_cache/examples --follow-imports silent --check-untyped-defs test/type_hint_tests/torch_optim.py $ mypy --cache-dir=.mypy_cache/strict --config mypy-strict.ini ``` Now you only have to run 2 much simpler commands: ``` $ mypy $ mypy --config mypy-strict.ini ``` One reason this is useful is because it will make it easier to integrate PyTorch's `mypy` setup into editors (remaining work on this to be done in a followup PR). Also, as shown in the test plan, this also reduces the time it takes to run `test/test_type_hints.py` incrementally, by reducing the number of times `mypy` is invoked while still checking the same set of files with the same configs. (Because this PR merges `test_type_hint_examples` (added in #34595) into `test_run_mypy` (added in #36584), I've added some people involved in those PRs as reviewers, in case there's a specific reason they weren't combined in the first place.) Pull Request resolved: #50631 Test Plan: Run this twice (the first time is to warm the cache): ``` $ python test/test_type_hints.py -v ``` - *Before:* ``` test_doc_examples (__main__.TestTypeHints) Run documentation examples through mypy. ... ok test_run_mypy (__main__.TestTypeHints) Runs mypy over all files specified in mypy.ini ... ok test_run_mypy_strict (__main__.TestTypeHints) Runs mypy over all files specified in mypy-strict.ini ... ok test_type_hint_examples (__main__.TestTypeHints) Runs mypy over all the test examples present in ... ok ---------------------------------------------------------------------- Ran 4 tests in 5.090s OK ``` You can also just run `mypy` to see how many files it checks: ``` $ mypy --cache-dir=.mypy_cache/normal --check-untyped-defs --follow-imports silent Success: no issues found in 1192 source files ``` - *After:* ``` test_doc_examples (__main__.TestTypeHints) Run documentation examples through mypy. ... ok test_run_mypy (__main__.TestTypeHints) Runs mypy over all files specified in mypy.ini ... ok test_run_mypy_strict (__main__.TestTypeHints) Runs mypy over all files specified in mypy-strict.ini ... ok ---------------------------------------------------------------------- Ran 3 tests in 2.404s OK ``` Now `mypy` checks 7 more files, which is the number in `test/type_hint_tests`: ``` $ mypy Success: no issues found in 1199 source files ``` Reviewed By: zou3519 Differential Revision: D25932660 Pulled By: samestep fbshipit-source-id: 26c6f00f338e7b44954e5ed89522ce24e2fdc5f0
This PR fixes a couple of syntax errors in
torch/that prevent MyPy from running, fixes simple type annotation errors (e.g. missingfrom typing import List, Tuple, Optional), and adds granular ignores for errors in particular modules as well as for missing typing in third party packages.As a result, running
mypyin the root dir of the repo now runs on:torch/aten/src/ATen/function_wrapper.py(the only file already covered in CI)In CI this runs on GitHub Actions, job Lint, sub-job "quick-checks", task "MyPy typecheck". It should give (right now):
Success: no issues found in 329 source files.Here are the details of the original 855 errors when running
mypy torchon current master (after fixing the couple of syntax errors that preventmypyfrom running through):Details
After this PR, contributors can start fixing type annotation issues per file or module by:
ignore_errors = Trueentry inmypy.ini$ mypyto show the errors$ mypyis clean againThere are still syntax errors left in
test/and incaffe2/. Those can be tackled in parallel as well, and once the syntax errors are fixed, the same strategy can be followed for them (add them tofiles, add the relevantignore_errors = True, then fix per module).Related issue: gh-16574