Added handling of hashing of types with args and typing special forms

[tclose]

• Bug fix (non-breaking change which fixes an issue)

Summary

Handles the case of hashing a class with type args, typing special forms (e.g. Union, ty.Type) and classes that inherit from ty.Generic

Checklist

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

[codecov[bot]]

Codecov Report

Patch coverage: 100.00% and project coverage change: +4.28% :tada:

Comparison is base (103cefc) 78.71% compared to head (fa7887b) 82.99%. Report is 3 commits behind head on master.

:exclamation: Current head fa7887b differs from pull request most recent head 4809dfe. Consider uploading reports for the commit 4809dfe to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #684 +/- ## ========================================== + Coverage 78.71% 82.99% +4.28% ========================================== Files 22 22 Lines 4873 4894 +21 Branches 1401 0 -1401 ========================================== + Hits 3836 4062 +226 - Misses 831 832 +1 + Partials 206 0 -206 ``` | [Flag](https://app.codecov.io/gh/nipype/pydra/pull/684/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipype) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/nipype/pydra/pull/684/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipype) | `82.99% <100.00%> (+4.42%)` | :arrow_up: | 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=nipype#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files Changed](https://app.codecov.io/gh/nipype/pydra/pull/684?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipype) | Coverage Δ | | |---|---|---| | [pydra/utils/hash.py](https://app.codecov.io/gh/nipype/pydra/pull/684?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipype#diff-cHlkcmEvdXRpbHMvaGFzaC5weQ==) | `95.03% <100.00%> (+2.88%)` | :arrow_up: | ... and [16 files with indirect coverage changes](https://app.codecov.io/gh/nipype/pydra/pull/684/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nipype)

:umbrella: View full report in Codecov by Sentry.

:loudspeaker: Have feedback on the report? Share it here.

[tclose]

@djarecka this PR is ready to merge now. I'm not sure where the coverage misses are coming from as it is primarily to do with the Dask worker (18 lines). Note that this PR is rebased off #687, so that should be merged first

[effigies]

@djarecka SLURM passed here. It might be worth digging into the runner setup info to see if some details of the machine this is run on differ between successes and crashes/timeouts.

e.g.,

[effigies]

What happens if someone uses from __future__ import annotations in their module?

[tclose]

What happens if someone uses from __future__ import annotations in their module?

That's a good question. I strongly suspect it would break things. However, we should be able to detect them and evaluate the class strings to proper classes when we create interfaces.

Happy to look into this if we end up refactoring the task/interface syntax

[tclose]

What happens if someone uses from __future__ import annotations in their module?

That's a good question. I strongly suspect it would break things. However, we should be able to detect them and evaluate the class strings to proper classes when we create interfaces.

Happy to look into this if we end up refactoring the task/interface syntax

Actually, looking at the code I think that attrs probably handles this for us

[tclose]

What happens if someone uses from __future__ import annotations in their module?

That's a good question. I strongly suspect it would break things. However, we should be able to detect them and evaluate the class strings to proper classes when we create interfaces. Happy to look into this if we end up refactoring the task/interface syntax

Actually, looking at the code I think that attrs probably handles this for us

Just checked with the following, and attrs doesn't help us

from __future__ import annotations
import attrs

@attrs.define
class A:
    x: int = 0
    y: float = 1.0

print(repr(attrs.fields(A).x.type))

[effigies]

Okay. I think it's fair to call that a limitation but not agonize over it. It might cause some cache misses based on changes in Python version or the package that defines a task, but we should be consistent across runs of the same code on the same Python version.

Under what circumstances are we actually hashing types? We're hashing values, so it seems like this should be moot for most cases.

[tclose]

Okay. I think it's fair to call that a limitation but not agonize over it. It might cause some cache misses based on changes in Python version or the package that defines a task, but we should be consistent across runs of the same code on the same Python version.

Under what circumstances are we actually hashing types? We're hashing values, so it seems like this should be moot for most cases.

As I mentioned, I expect it to be pretty specific to my code, but in Arcana, the file-format class that input data files are stored in and output data files should be stored in is passed as an input to the "source" and "sink" nodes

[tclose]

Okay. I think it's fair to call that a limitation but not agonize over it. It might cause some cache misses based on changes in Python version or the package that defines a task, but we should be consistent across runs of the same code on the same Python version.

Under what circumstances are we actually hashing types? We're hashing values, so it seems like this should be moot for most cases.

String annotations will break the type-checking though...

[effigies]

Maybe we just treat strings as Any and raise a one-time warning?

[tclose]

Maybe we just treat strings as Any and raise a one-time warning?

We could do that, or attempt to evaluate them, and if that fails fallback to Any.

[tclose]

I reckon this is ready to merge now as the string annotations are probably best addressed in a separate PR