Incorporate type hints when possible

[tsalo]

Summary

Should we start using type hints, when possible, in our functions?

I believe that it would reduce the amount of argument validation that we do at the beginning of each function. Plus it would make our code more transparent.

One drawback is that, in order to support type hints for numpy arrays (one of our most common argument types), we would probably need an additional dependency like nptyping.

Yea: :+1: Nay: :-1:

Additional Detail

@jbteves has started using type hints in #692, which I quite like. He also brought up style guidelines in this month's developers meeting (see #701), so I thought I'd get the ball rolling with this issue.

Next Steps

1. Discuss (feel free to weigh in with a vote, but please explain any nays)
2. Possibly implement

[jbteves]

Just FYI, the types aren't actually checked at runtime unless we add mypy as well. That's why you had to correct one of my type hints in that PR; if they were checked then I would have been getting runtime errors. We can add it if we think it's worth it, however. I included it in that PR mostly to improve documentation and also because frankly I've been using type hints in a different project and forgot we're not using them here 😆 . My view is that we should begin adding type hints in PRs as we modify functions, not do them in one big PR, and add nptyping as a dependency to make that possible. I need to experiment a bit with mypy and npytyping to decide if they're reliable enough to add type-checking to the whole codebase from my point of view.

Thanks for opening @tsalo !

[tsalo]

Just FYI, the types aren't actually checked at runtime unless we add mypy as well.

I did not know that. That's somewhat depressing and simultaneously pretty cool!

My view is that we should begin adding type hints in PRs as we modify functions, not do them in one big PR, and add nptyping as a dependency to make that possible.

My concern about this approach (along with the same approach to reformatting with black or isort) is that there are a number of functions that are very stable, and we may not touch them (and thus get them up to snuff) for a long time. If we want to maintain a coding standard across the package, at some point we'll need to bite the bullet and apply our rules to all of the code, or else we'll eventually have a bit of a Frankenstein's monster situation.

EDIT: Another relevant task is #448, which could be done piecemeal, but hopefully won't be.

[jbteves]

That's somewhat depressing and simultaneously pretty cool!

Intriguingly that's my relationship with Python in a nutshell!

My concern about this approach (along with the same approach to reformatting with black or isort) is that there are a number of functions that are very stable, and we may not touch them (and thus get them up to snuff) for a long time.

I think that's fair. That said, I think that we have enough things going on that adding a major refactor of style is not up at the top of my list. If other developers feel differently, that's worth a discussion, however.

[handwerkerd]

I may be optimistic, but, IF types are added to the BIDS outputs branch #691, then modularize metrics #591, and then decision tree modularization #592, we'll have covered a substantial portion of the code base. At that point, systematic addition of types to the rest of the code will be annoying, but should be do-able.

[emdupre]

To clarify my :-1: : I'm for adding type-hints in general -- in that I think they're nice to read -- but I'm against adding dependencies to enforce / check the types. It feels a bit non-pythonic, somehow :)

[notZaki]

Would those additional dependencies have to be general dependencies that all users have to install/import or will they be test dependencies? (... and does that matter in swaying opinions?)

[jbteves]

It's pretty tricky to make them dependencies for only "some" users, though pretty straightforward to make them dependencies for developers only. I personally think that's a recipe for confusion and should be avoided, however.

[tsalo]

I think the main potential benefits would be (1) automatic type checking and (2) automatic type documentation, right? @emdupre I personally like the idea of automatic type checking, but I'm fine not using it. Unfortunately, as far as (2), it doesn't seem like napoleon or numpydoc support type hints automatically, and the Sphinx extensions that support it don't work with numpydoc-style docstrings yet. All of which is to say that if we don't want to actually validate parameter types, I don't know how much type hints will get us with our current documentation structure. 😞

[jbteves]

I am obviously biased but (1) but it helps me when doing things on CLI and (2) coming from strongly typed languages it makes things a little more familiar to me. I doubt (2) is as much of a consideration for others, though. Other users may be using servers where it's difficult to set up some sort of IDE, however, and in that case it's nice to have the hints inline with the parameters.

[tsalo]

The nilearn dev team recently discussed type hints as well. The consensus was (1) the primary benefit is for users/devs using code editors like VSCode, rather than any kind of automatic input validation or documentation, and (2) they would encourage, but not require, type hints in new contributions, but type hints would not be feasible/interpretable for some parts of the library.

Of course, we would still need to add nptyping to include type hints for most of our parameters, and I don't think we could make it a dev-only requirement unless we did a duecredit-like thing and included a dummy module to handle ImportErrors automatically...

EDIT: Also, given the rather limited impact this would have (short of using mypy and figuring out how to support numpydoc with napoleon's type hinting extensions), I'm going to downgrade the impact label for this issue.

[handwerkerd]

This might be a naive question, but could you clarify what nptying adds over the the numpy typing that's native to Python 3.8 and can be backported to 3.6? https://numpy.org/devdocs/reference/typing.html Can we get most of the benefits of type hints for now without adding another dependency?

[tsalo]

I didn't know that numpy types were native to 3.8, so that's good news. How can this be backported to 3.6/3.7 though? It looks like we'd need to install typing-extensions.

[handwerkerd]

I overlooked that typing-extensions isn't a native library & would need to be added. That said, IF this meets our needs, then we can mark it as a dependency only for people running python 3.6 or 3.7 and we'd be able to remove the dependency when we eventually move to requiring 3.8.

[tsalo]

I think that's a reasonable solution. Does anyone have any issues with adding the typing-extensions package as a dependency?

[emdupre]

I'm still a bit concerned about this. Just to confirm, this can't be done as a dev-only dependency ?

[tsalo]

I guess we could have a fake module like we do for duecredit? Otherwise, I don't think so. The types appear in function definitions, so we can't use try/except ImportError statements.

[notZaki]

Not sure if this changes things, but bokeh already requires the typing-extensions package for Python >= 3.6 (since ~2 years ago). So it won't necessarily be a new dependency.

[tsalo]

Thanks @notZaki! I think we can add typing-extensions without affecting anything then.

[tsalo]

Our minimum Python version is now 3.8, so I don't think we need any extra dependencies. I will open a PR adding a checklist to the PR template with things like "If I edited functions, I included type hints".