MNT: Type-annotate Pydra

[effigies]

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]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.20 ⚠️

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

❗ 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

@@            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%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pydra/engine/core.py	92.96% <100.00%> (-0.16%)	⬇️
pydra/engine/helpers.py	84.39% <100.00%> (-1.66%)	⬇️
pydra/engine/specs.py	94.83% <100.00%> (+0.03%)	⬆️
pydra/engine/task.py	93.27% <100.00%> (-0.31%)	⬇️
pydra/utils/messenger.py	95.89% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

[effigies]

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]

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]

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]

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]

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]

@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]

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

[effigies]

No.