Conversation
This comment has been minimized.
This comment has been minimized.
for more information, see https://pre-commit.ci
…into add-auto-workers
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
hmm, seems like because the CI doesn't like that im using these mocks |
| The ``MYPY_NUM_WORKERS`` environment variable accepts the same values and | ||
| overrides this setting. |
There was a problem hiding this comment.
The order of precedence is: command line flag > environment variable > configuration file; yes? Does that match the other options?
There was a problem hiding this comment.
Yes, I think this is the standard order for mypy.
| from io import StringIO | ||
| from pathlib import Path | ||
| from typing import Any, cast | ||
| from unittest import mock |
There was a problem hiding this comment.
in general, use pytest.monkeypatch instead of unittest; though neither can mock mypyc compiled code
There was a problem hiding this comment.
Yes, @KevinRK29 mypyc doesn't support monkey patching. Instead, you can try setting (private) cached value like this:
mypy.util._AVAILABLE_THREADS = 32
# <...testing...>
mypy.util._AVAILABLE_THREADS = NoneYou can even write a simple context manager to do this (but do not expose it, keep it private to this test file)
There was a problem hiding this comment.
@KevinRK29 to be clear mock.patch.dict is fine, the problem is only with trying to patch a function.
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
LG, thanks! I have few comments.
| The ``MYPY_NUM_WORKERS`` environment variable also accepts ``auto``. This | ||
| setting will override the environment variable if it is set. |
There was a problem hiding this comment.
I don't think you need to add anything about MYPY_NUM_WORKERS here, just keep the original text.
| The ``MYPY_NUM_WORKERS`` environment variable also accepts ``auto``. This | |
| setting will override the environment variable if it is set. | |
| This setting will override the ``MYPY_NUM_WORKERS`` environment | |
| variable if it is set. |
| * Parallel mode requires and automatically enables :option:`--native-parser`. | ||
| * Parallel mode requires and automatically enables :option:`--native-parser` | ||
| and :ref:`incremental mode <incremental>`. Specifying | ||
| :option:`--no-incremental` has no effect in parallel mode. |
There was a problem hiding this comment.
I was thinking more about this, and silently overriding it may be not the best thing. People may need it for some niche things like running benchmarks. So we should respect it, but show a warning instead. More on this below.
| if options.num_workers: | ||
| # Supporting both parsers would be really tricky, so just support the new one. | ||
| options.native_parser = True | ||
| options.incremental = True |
There was a problem hiding this comment.
I think we should do something like this here instead:
if not options.incremental and os.path.isdir(options.cache_dir):
print("Warning: disabling incremental mode may severely reduce performance")
print(f"If this is intentional, delete '{options.cache_dir}' to suppress this warning")| metavar="VALUE", | ||
| default=0, | ||
| help="Number of separate mypy worker processes (experimental)", | ||
| help="Number of separate mypy worker processes, or 'auto' (experimental)", |
There was a problem hiding this comment.
I don't think we need to say that parallel type checking is experimental anymore.
3 participants
Fixes #21477
Allow
autoanywhere the number of parallel workers can be configured.The auto selection reuses
get_available_threadsand is capped at 8 workers. You can still exceed the cap by specifying the number of workers.Also
0still disables parallel checking,1selects one worker, and the default remains0.Also this automatically enables incremental mode whenever parallel checking is enabled.