Fix and expand datalad.runner type annotations #6893

christian-monch · 2022-07-27T10:38:22Z

This PR fixes #6889

The runner-code passes validation tests of mypy version 0.971, i.e. with strict optional checking.

Changelog

🏠 Internal

fix type annotations in datalad.runner this resolves issue datalad/runner already has lots of annotations but ATM fails mypy (@christian-monch interested? ;)) #6889

This commit simplifies the type of `WitlessProtocol.proc_out` and `WitlessProtocol.proc_err` to `bool`. The code treated it earlier as `Optional[bool]`.

This commit fixes the type annotations in `datalad.runner.exceptions`, and adds runtime type-checks that are required by `mypy` for a successful validation.

This commit fixes type annotation problems in `datalad.runner.runnerthreads`. It also replaces `Union[x, None]` with `Optional[x]`.

This commit fixes a faulty type annotation by adding the missing `Optional[...]` clause.

This commit adds logging of the protocol that was provided to the runner in order to improve the possibility to diagnose an error based on debug log-entries.

This commit fixes existing type annotations in `datalad.runner.nonasyncrunner`. A few `isinstance()` and `assert` clauses had to be added. A number of `ThreadedRunner` attributes are only declared in the constructor. This silences some other tools that check for declarations of used attributes in the constructor of a class.

codecov · 2022-07-27T11:27:28Z

Codecov Report

Merging #6893 (895becc) into maint (1fa6441) will increase coverage by 0.99%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6893      +/-   ##
==========================================
+ Coverage   90.16%   91.15%   +0.99%     
==========================================
  Files         354      354              
  Lines       46197    46215      +18     
==========================================
+ Hits        41652    42127     +475     
+ Misses       4545     4088     -457

Impacted Files	Coverage Δ
datalad/runner/exception.py	`100.00% <100.00%> (ø)`
datalad/runner/nonasyncrunner.py	`99.65% <100.00%> (+0.01%)`	⬆️
datalad/runner/protocol.py	`100.00% <100.00%> (ø)`
datalad/runner/runner.py	`100.00% <100.00%> (ø)`
datalad/runner/runnerthreads.py	`97.47% <100.00%> (+0.02%)`	⬆️
datalad/runner/tests/test_exception.py	`100.00% <100.00%> (ø)`
datalad/runner/tests/test_nonasyncrunner.py	`100.00% <100.00%> (ø)`
datalad/runner/tests/test_threadsafety.py	`100.00% <100.00%> (ø)`
datalad/runner/tests/test_utils.py	`100.00% <100.00%> (ø)`
datalad/runner/utils.py	`100.00% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa6441...895becc. Read the comment docs.

jwodder · 2022-07-27T16:26:35Z

@christian-monch DataLad requires Python 3.7+, correct? If so, the recommended approach is to use "modern" type annotations by adding from __future__ import annotations to the top of annotated files, which lets you write, e.g., dict[str, str | int] instead of typing.Dict[str, typing.Union[str, int]].

datalad/runner/exception.py

This commit adds a missing return type to `_format_json_error_messages`. Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

christian-monch · 2022-07-27T19:08:18Z

@christian-monch DataLad requires Python 3.7+, correct? If so, the recommended approach is to use "modern" type annotations by adding from __future__ import annotations to the top of annotated files, which lets you write, e.g., dict[str, str | int] instead of typing.Dict[str, typing.Union[str, int]].

Good point, thanks. The annotations stem from the earlier versions. Will verify the requirements and use the "modern" style, which I prefer anyway :-)

yarikoptic · 2022-07-27T20:04:09Z

woohoo, thanks @christian-monch . I pushed little commit to add entire runner into typechecking CI of tox

yarikoptic · 2022-07-27T20:11:29Z

for your convenience, here is what errors CI reports

datalad/runner/tests/test_utils.py:140: error: Need type annotation for "decoded_chars"
datalad/runner/tests/test_threadsafety.py:61: error: Need type annotation for "args"
datalad/runner/tests/test_witless_runner.py:22: error: Cannot find implementation or library stub for module named "pytest"
datalad/runner/tests/test_witless_runner.py:22: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 3 errors in 3 files (checked 22 source files)
ERROR: InvocationError for command /home/runner/work/datalad/datalad/.tox/typing/bin/mypy --follow-imports skip datalad/api.py datalad/downloaders/providers.py datalad/metadata/indexers/base.py datalad/runner datalad/support/collections.py (exited with code 1)

so only something funny left in the runner tests.

Overcomes error: Cannot find implementation or library stub for module named "pytest" and I do not think there is types-pytest

yarikoptic · 2022-07-27T20:19:28Z

For

error: Cannot find implementation or library stub for module named "pytest"

added pytest to deps of typing (@jwodder is there a better way?) . So we are left only with the other two

jwodder · 2022-07-27T20:23:31Z

tox.ini

@@ -29,6 +29,7 @@ commands = flake8 {posargs}
 [testenv:typing]
 deps =
    mypy~=0.900
+    pytest


Rather than manually specifying test dependencies, it would be better to tell tox to install all of them via the tests extra:

[testenv:typing] extras = tests ...

Someone please address this.

christian-monch · 2022-07-28T07:19:18Z

for your convenience, here is what errors CI reports

datalad/runner/tests/test_utils.py:140: error: Need type annotation for "decoded_chars"
datalad/runner/tests/test_threadsafety.py:61: error: Need type annotation for "args"
datalad/runner/tests/test_witless_runner.py:22: error: Cannot find implementation or library stub for module named "pytest"
datalad/runner/tests/test_witless_runner.py:22: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 3 errors in 3 files (checked 22 source files)
ERROR: InvocationError for command /home/runner/work/datalad/datalad/.tox/typing/bin/mypy --follow-imports skip datalad/api.py datalad/downloaders/providers.py datalad/metadata/indexers/base.py datalad/runner datalad/support/collections.py (exited with code 1)

so only something funny left in the runner tests.

Thx, forgot to check the tests-directory. Fixing it.

This commit includes some minor reformattings without semantic changes.

jwodder · 2022-07-28T13:08:28Z

datalad/runner/tests/test_threadsafety.py

@@ -32,7 +33,7 @@ def pipe_data_received(self, fd, data):
            self.send_result((fd, line))


-def _runner_with_protocol(protocol) -> ThreadedRunner:
+def _runner_with_protocol(protocol: Type[WitlessProtocol]) -> ThreadedRunner:


from __future__ import annotations lets you use the builtin type instead of typing.Type here.

Thx. Changed it to type[...]

This commit modernizes class-type annotations in the runner code and runner tests

jwodder · 2022-07-28T15:00:59Z

tox.ini

@@ -29,6 +29,7 @@ commands = flake8 {posargs}
 [testenv:typing]
 deps =
    mypy~=0.900
+    pytest


Someone please address this.

This commit moves test dependencies from `tox.ini` into `setup.py` and just references them in `tox.ini`.

christian-monch · 2022-07-28T15:14:25Z

Moved the type-test dependency definitions to setup.py. Tests and type-checking dependencies are both in requires['tests'].

christian-monch added 6 commits July 27, 2022 12:07

simplify type of proc_out and proc_err

21b4388

This commit simplifies the type of `WitlessProtocol.proc_out` and `WitlessProtocol.proc_err` to `bool`. The code treated it earlier as `Optional[bool]`.

fix type annotations in runner exceptions

c054caf

This commit fixes the type annotations in `datalad.runner.exceptions`, and adds runtime type-checks that are required by `mypy` for a successful validation.

fix type annotations in runnerthreads.py

662236f

This commit fixes type annotation problems in `datalad.runner.runnerthreads`. It also replaces `Union[x, None]` with `Optional[x]`.

fix type annotations in datalad.runner.utils

5881f20

This commit fixes a faulty type annotation by adding the missing `Optional[...]` clause.

log the protocol used in WitlessRunner.run

85f2494

This commit adds logging of the protocol that was provided to the runner in order to improve the possibility to diagnose an error based on debug log-entries.

christian-monch added the semver-internal Changes only affect the internal API label Jul 27, 2022

christian-monch changed the base branch from master to maint July 27, 2022 10:38

christian-monch requested review from yarikoptic and jwodder July 27, 2022 16:20

jwodder requested changes Jul 27, 2022

View reviewed changes

datalad/runner/exception.py Outdated Show resolved Hide resolved

add missing return type to function

0c9bbcc

This commit adds a missing return type to `_format_json_error_messages`. Co-authored-by: John T. Wodder II <jwodder@users.noreply.github.com>

ENH(TST): include entire datalad/runner into tox.ini for type checks

503a360

BF: add pytest into deps for typing since needed

da28303

Overcomes error: Cannot find implementation or library stub for module named "pytest" and I do not think there is types-pytest

jwodder reviewed Jul 27, 2022

View reviewed changes

christian-monch added 4 commits July 28, 2022 10:28

fix type annotation errors in runner-tests

cff3fe3

use new annotation style in runner tests

317f20d

modernize type annotations in runner code

ff26563

reformatting

8780390

This commit includes some minor reformattings without semantic changes.

christian-monch requested a review from jwodder July 28, 2022 09:27

jwodder requested changes Jul 28, 2022

View reviewed changes

use type[...] instead of Type[...]

cb53f76

This commit modernizes class-type annotations in the runner code and runner tests

christian-monch requested a review from jwodder July 28, 2022 15:00

jwodder requested changes Jul 28, 2022

View reviewed changes

move test dependencies to setup.py

895becc

This commit moves test dependencies from `tox.ini` into `setup.py` and just references them in `tox.ini`.

christian-monch requested a review from jwodder July 28, 2022 18:41

jwodder approved these changes Jul 28, 2022

View reviewed changes

christian-monch merged commit f57c417 into datalad:maint Jul 28, 2022

christian-monch deleted the fix-issue-6889-runner-type-annotations-2 branch July 28, 2022 19:02

yarikoptic changed the title ~~Fix runner type annotations~~ Fix and expand datalad.runner type annotations Jul 29, 2022

christian-monch mentioned this pull request Aug 24, 2022

datalad/runner already has lots of annotations but ATM fails mypy (@christian-monch interested? ;)) #6889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and expand datalad.runner type annotations #6893

Fix and expand datalad.runner type annotations #6893

christian-monch commented Jul 27, 2022

codecov bot commented Jul 27, 2022 •

edited

Loading

jwodder commented Jul 27, 2022

christian-monch commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

jwodder Jul 27, 2022

jwodder Jul 28, 2022

christian-monch commented Jul 28, 2022 •

edited

Loading

jwodder Jul 28, 2022

christian-monch Jul 28, 2022

jwodder Jul 28, 2022

christian-monch commented Jul 28, 2022 •

edited

Loading

Fix and expand datalad.runner type annotations #6893

Fix and expand datalad.runner type annotations #6893

Conversation

christian-monch commented Jul 27, 2022

Changelog

🏠 Internal

codecov bot commented Jul 27, 2022 • edited Loading

Codecov Report

jwodder commented Jul 27, 2022

christian-monch commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

yarikoptic commented Jul 27, 2022

jwodder Jul 27, 2022

Choose a reason for hiding this comment

jwodder Jul 28, 2022

Choose a reason for hiding this comment

christian-monch commented Jul 28, 2022 • edited Loading

jwodder Jul 28, 2022

Choose a reason for hiding this comment

christian-monch Jul 28, 2022

Choose a reason for hiding this comment

jwodder Jul 28, 2022

Choose a reason for hiding this comment

christian-monch commented Jul 28, 2022 • edited Loading

codecov bot commented Jul 27, 2022 •

edited

Loading

christian-monch commented Jul 28, 2022 •

edited

Loading

christian-monch commented Jul 28, 2022 •

edited

Loading