MNT: Type-annotate Pydra

effigies commented 1 year ago

Types of changes

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Summary

Building off of #621. Interested in feedback as we go.

Checklist

[ ] I have added tests to cover my changes (if necessary)
[ ] I have updated documentation (if necessary)

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.20 :warning:

Comparison is base (64bfa89) 81.77% compared to head (1d133a9) 81.57%.

:exclamation: Current head 1d133a9 differs from pull request most recent head 5c048b1. Consider uploading reports for the commit 5c048b1 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #648 +/- ## ========================================== - Coverage 81.77% 81.57% -0.20% ========================================== Files 20 20 Lines 4394 4397 +3 Branches 1264 0 -1264 ========================================== - Hits 3593 3587 -6 - Misses 797 810 +13 + Partials 4 0 -4 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `81.57% <100.00%> (-0.20%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [pydra/engine/core.py](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL2NvcmUucHk=) | `92.96% <100.00%> (-0.16%)` | :arrow_down: | | [pydra/engine/helpers.py](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL2hlbHBlcnMucHk=) | `84.39% <100.00%> (-1.66%)` | :arrow_down: | | [pydra/engine/specs.py](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL3NwZWNzLnB5) | `94.83% <100.00%> (+0.03%)` | :arrow_up: | | [pydra/engine/task.py](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL3Rhc2sucHk=) | `93.27% <100.00%> (-0.31%)` | :arrow_down: | | [pydra/utils/messenger.py](https://app.codecov.io/gh/nipype/pydra/pull/648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvdXRpbHMvbWVzc2VuZ2VyLnB5) | `95.89% <100.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/nipype/pydra/pull/648/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

effigies commented 1 year ago

Okay, it looks like we're going to have fights with type checkers about File and Directory. If we use File = NewType("File", Path), then Python 3.10+ will happily accept it, but prior to that, there are pickling errors. We can't subclass Path, which is a known limitation of Python and won't be fixed until Python 3.12 at a minimum (I'm not sure of the status of that effort). Importing NewType from typing_extensions doesn't give us 3.10+ behavior in 3.7.

@djarecka What exactly are the semantics of these? It looks like they are labels but not actually instantiable as classes containing paths, so when we write things like

@task
def dirname(a: File) -> Directory:
    return Path(a).parent

We get a type failure. And we can't wrap Directory(Path(a).parent), since Directory() doesn't take args.

One possibility could be to just implement a very thin os.PathLike interface:

class FileObj:
    def __init__(self, path: ty.Union[str, os.PathLike]):
        self._path = Path(path)
    def __fspath__(self):
        return str(self._path)
    def __repr__(self):
        return repr(self._path)

class File(FileObj): ...
class Directory(FileObj): ...

I think this would get us close enough to 3.10-style NewTypes, so we could eventually get rid of these. But it may make more sense to get rid of/deprecate these concepts altogether.

djarecka commented 1 year ago

I think File and Directory was used a bit as placeholders for future class, so we can use something like FileObj.

I'm not sure what you mean by "concept" that we might want to get rid of.

effigies commented 1 year ago

Sorry, I just meant the classes. I'm not sure how all they're used, so it's unclear what we need to replace if we just delete them.

ghisvail commented 1 year ago

I'd propose to define File and Directory as aliases to os.PathLike which is the generic protocol for paths in Python. File would represent a pathlike instance pointing to a file, Directory to a directory.

Anything satisfying os.PathLike can be converted to pathlib.[Pure]Path underneath, so it should not be a problem for the internal Pydra engine machinery. Besides, it would satisfy estabished good OOP practices of "committing to an interface, not an implementation", thereby allowing alternatives like fsspec to be used as input arguments as well.

ghisvail commented 1 year ago

We also need to make sure we would not be accidentally breaking pieces of formatting or callable logic like this which relies on isinstance / pattern matching on types.

djarecka commented 1 year ago

@effigies - I mostly used them in Input/Output spec so it's clear what people expect to get. I also use this type when doing the current hashing of setting the bindings to the container. Not saying that it would be impossible without them, but not sure if I want to remove them completely... Can think more

ghisvail commented 1 year ago

Given the major changes recently introduced, do you think it's worth keeping this PR open?

effigies commented 1 year ago

No.

nipype / pydra