Feature/python typing/components command.py

[NovakApis]

PR checklist

• This comment contains a description of changes (with reason)
  Added types to the components_command.py
  As this is the main component used in other components, let us nail this one!

[awgymer]

I wonder if we need to think quite carefully about the approach to type hinting within tools?

File-by-file seems like a good approach but would we benefit from some custom types (e.g. a pathlike = Union[str, Path] being defined somewhere central that can be used across the codebase rather than redefining those sorts of unions in each file?

[mirpedrol]

looks good :)

[mirpedrol]

After reading @awgymer comments, I think it makes sense to talk about it a bit more 🙂

[fabianegli]

Hi Novak

I just pushed a commit to this PR to add mypy checks to the CI. This should help to make sure we don't add any conflicting type annotations. I am not sure if it is entirely correct, but we'll see the test once the CI runs.

👋

[codecov]

Codecov Report

Merging #2223 (5c88dbf) into dev (a220bcd) will increase coverage by 0.14%.
Report is 10 commits behind head on dev.
The diff coverage is 64.86%.

❗ Current head 5c88dbf differs from pull request most recent head 9171fa0. Consider uploading reports for the commit 9171fa0 to get more accurate results

@@            Coverage Diff             @@
##              dev    #2223      +/-   ##
==========================================
+ Coverage   70.60%   70.74%   +0.14%     
==========================================
  Files          87       87              
  Lines        9437     9439       +2     
==========================================
+ Hits         6663     6678      +15     
+ Misses       2774     2761      -13     

Files	Coverage Δ
nf_core/components/components_command.py	71.32% <94.11%> (ø)
nf_core/components/components_utils.py	46.51% <40.00%> (-1.69%)	⬇️

... and 2 files with indirect coverage changes

[mirpedrol]

Second try with a more conscious review, taking into account all the comments we have been discussing :)
My line of thought was to only annotate variables which are class arguments or declared such empty lists, etc. Feel free to disagree! 🙂

[awgymer]

I have never used mypy properly before but my assumption is that when it runs it will flag anything that is not annotated or derivable that should be, so that the best way to proceed will be to add fewer annotations and only add things like the above examples if mypy complains?

If I am wrong about how mypy works then feel free to disregard

[NovakApis]

As it stands, mypy will fail every time we run tests. Doing a thorough check breaks on third-party library imports (first lines of .py files). The current repo doesn't include stubs for:

• yaml - stub
• questionary
• rich
• git
• prompt_toolkit
• packaging
• requests stub
• requests_cache

Is it wise to create a new issue including stubs for these libraries?
What if some are not provided with a stub, should we skip them in mypy tests?

[fabianegli]

The mypy config in the pyproject.toml should solve the missing stub issue. (in theory at least. hopefully...)

[fabianegli]

@mashehu suggested we limit the checking to a PR-by-PR basis

The problem with that approach is that now it will add files to the checks even if someone just changes a docstring somewhere and then that is blocked unless the whole file is correctly type hinted because of previous - PR unrelated unrelated - false type annotations. It will create a greater mess than simply removing erroneous type hints now.

@mashehu Please reconsider

[kedhammar]

@fabianegli one problem I am having with eliminating repository-wide MyPy errors is that the typing of external packages can contradict itself, e.g.

nf_core/utils.py:56: error: Argument 1 to "join" has incompatible type "Optional[str]"; expected "Union[str, PathLike[str]]"

here the output types of os.environ.get() is incompatible with the expected inputs of os.path.join().

Do you have any good ideas in mind for addressing this?

[kedhammar]

Summary:

• I've decreased the scope of MyPy to only check modified files.
• I've addressed MyPy errors by mostly trimming out superfluous typing and in some cases by minor refactoring of the code to prevent issues.

I tried expanding the scope of MyPy to the entire repository and addressed all issues I could find by running MyPy locally (see c97ef6e). However, even if the repo passed locally the GHA CI threw a lot of new errors which I believe would be too time consuming to address during the Hackathon.

@fabianegli you are welcome to create a new PR to expand the scope of MyPy to the entire repo, but @mashehu asked me to proceed with the limited scope.

outdated

[fabianegli]

nf_core/utils.py:56: error: Argument 1 to "join" has incompatible type "Optional[str]"; expected "Union[str, PathLike[str]]"

here the output types of os.environ.get() is incompatible with the expected inputs of os.path.join().

Do you have any good ideas in mind for addressing this?

This is exactly why type hints are useful. They tell us that what we coded together might not wrk as expected. It basically tells us that we need to handle the case when the environment variable is not found and os.environ.get() returns None, because None can not be joined. So to fix this, one should ask the question "What should tools do, when said environment variable is undefined?" and then adapt the code accordingly.