mypy compliance

[liamhuber]

At the last meeting of December, @niklassiemer indicated he was fine learning whatever typing stuff was necessary in the event he takes over maintenance, so this PR is intended to close #514.

My current plan is to take any low-hanging fruit (e.g. hinting typing.Callable instead of callable) and just commit it directly here, but to break larger changes into stacked PRs so it's easier to view the diff and see intentions. I started using a file-based approach and have a branch for getting the channels sub-module compliant, but for things in the mixins it may make more sense to group the PRs so they touch multiple files, but updating a single root class.

@niklassiemer, you may want to keep an eye on this series of PRs just to get a rough sense of what was involved in this transition.

@XzzX, I'll ping you for review on this root branch when I feel it's done, but I'll freely merge the feature/file branches into it without delay. Nonetheless, feel free to also keep an eye on the whole series of PRs and interject whenever you have insight, and/or just stack fixes you want right on top of here.

EDIT: as long as it's in draft state, I'm not going to sweat about ruff and black failures. Definitely before merging, and I'll try to patch them up when I get a chance, but I am going to prioritize avoiding merge conflicts with any stacked branches over keeping the formatters permanently happy.

Progress tracker: mypy complaints

• Initial push) 161
• mypy channels #534) 148
• mypy channels redux #536 and Apply hints to IO panels #537) 126
• mypy run #541) 119
• mypy topology and find #542) 114
• mypy semantics #538) 106
• Semantics generic parent #544) 102
• Improvements to semantic labeling #547) 101
• Annotate some extra returns #548) 100

[github-actions]

👈 Launch a binder notebook on branch pyiron/pyiron_workflow/mypy

[liamhuber]

Windows tests failed, but CondaHTTPError: HTTP 000 CONNECTION FAILED for url <https://conda.anaconda.org/conda-forge/noarch/repodata.json>, so hopefully it is just a hiccup.

EDIT: Yep. Better now.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.02% (target: -1.00%)	✅ 97.18%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (53a5c14)	3356	3070	91.48%
Head commit (801bddb)	3401 (+45)	3112 (+42)	91.50% (+0.02%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#533)	142	138	97.18%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

[coveralls]

Pull Request Test Coverage Report for Build 12774551989

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 103 unchanged lines in 13 files lost coverage.
• Overall coverage increased (+0.02%) to 91.502%

---

Files with Coverage Reduction	New Missed Lines	%
nodes/function.py	1	98.41%
type_hinting.py	1	97.73%
nodes/transform.py	3	98.26%
find.py	3	90.32%
topology.py	5	88.61%
mixin/run.py	6	93.75%
nodes/macro.py	7	92.2%
mixin/preview.py	8	93.29%
nodes/composite.py	11	91.71%
io.py	14	94.4%

Totals
Change from base Build 12773583061:	0.02%
Covered Lines:	3112
Relevant Lines:	3401

---

💛 - Coveralls

[liamhuber]

I currently only point mypy at the main module; I'd like to make the tests compliant as well, but that can be done at a later time in a separate PR.

[XzzX]

I hope I got the idea of the channels, right.

[XzzX]

Suggested change

	def _valid_connection(self, other: object) -> bool:
	def _valid_connection(self, other: ConnectionPartner) -> bool:

We can statically check if connection attempt is valid.

[XzzX]

isinstance can be done here and called in subclasses via super. We could get rid of connection_partner_type, if we want to or make it non-abstract.

[liamhuber]

Yes, I agree, you'd absolutely think so, right? But as far as I can tell, this is not allowed. The first problem is that ConnectionPartner is usable only at type-checking time and has no impact on runtime, so we can't do things like isinstance(other, ConnectionPartner) because, unfortunately, at runtime the concrete child classes never convert this to the actual class they use. So you wind up with an error that isinstance is expecting something type-y and not a TypeVar. Here is a demo snippet to play with:

import abc
import typing

SomeThing = typing.TypeVar("SomeThing")

class ThingChecker(typing.Generic[SomeThing], abc.ABC):
    def is_it_the_thing(self, thing: SomeThing) -> bool:
        return isinstance(thing, SomeThing)

    def what_is_the_thing(self) -> str:
        print(f"The thing is {SomeThing}")

class StringChecker(ThingChecker[str]):
    pass

sc = StringChecker()
sc.what_is_the_thing()
sc.is_it_the_thing(1), sc.is_it_the_thing("one")

The second problem is that mypy is a little persnickety about super() calls. I guess this is because python allows multiple inheritance, so it is not as sure as it would like to be about what the super call will do. I'm confident we could get around this by invoking something like ParentClass.method_we_want instead of super().method_we_want (or similar). I also can't currently rule out that we could get super() itself working with a bit more massaging. At any rate, my first crack at getting this to work nicely failed, so I took the most expedient route and avoided super() altogether. If I learn a nice pattern for this in the future, I'd be happy to come back and fix it.

[XzzX]

Yes, I agree, you'd absolutely think so, right? But as far as I can tell, this is not allowed. The first problem is that ConnectionPartner is usable only at type-checking time and has no impact on runtime, so we can't do things like isinstance(other, ConnectionPartner) because, unfortunately, at runtime the concrete child classes never convert this to the actual class they use. So you wind up with an error that isinstance is expecting something type-y and not a TypeVar. Here is a demo snippet to play with:

:( I got carried away with what is possible in other languages. You are right.

[XzzX]

The second problem is that mypy is a little persnickety about super() calls. I guess this is because python allows multiple inheritance, so it is not as sure as it would like to be about what the super call will do. I'm confident we could get around this by invoking something like ParentClass.method_we_want instead of super().method_we_want (or similar). I also can't currently rule out that we could get super() itself working with a bit more massaging. At any rate, my first crack at getting this to work nicely failed, so I took the most expedient route and avoided super() altogether. If I learn a nice pattern for this in the future, I'd be happy to come back and fix it.

What error are you referring to? This error?

pyiron_workflow/channels.py:645: error: Signature of "__call__" incompatible with supertype "SignalChannel"  [override]
pyiron_workflow/channels.py:645: note:      Superclass:
pyiron_workflow/channels.py:645: note:          def __call__(self) -> None
pyiron_workflow/channels.py:645: note:      Subclass:
pyiron_workflow/channels.py:645: note:          def __call__(self, other: OutputSignal) -> None

This is a real problem.

[liamhuber]

That's not the problem I'm referring to. Also, where did you generate that? It looks like the incompatibility between InlutSignal and AccumulatingSignal, but I fixed that.(Maybe I'm looking at the wrong branch here?)

Edit: ah, we're in the base PR. Yes, this is was easily fixed in the PR addressing channels.

[XzzX]

@XzzX, I'll ping you for review on this root branch when I feel it's done, but I'll freely merge the feature/file branches into it without delay. Nonetheless, feel free to also keep an eye on the whole series of PRs and interject whenever you have insight, and/or just stack fixes you want right on top of here.

I favor smaller piece to review. I do not think I will be able to do a review of the final branch. But I can have a look at the smaller pieces if you want.

[liamhuber]

@XzzX, I'll ping you for review on this root branch when I feel it's done, but I'll freely merge the feature/file branches into it without delay. Nonetheless, feel free to also keep an eye on the whole series of PRs and interject whenever you have insight, and/or just stack fixes you want right on top of here.

I favor smaller piece to review. I do not think I will be able to do a review of the final branch. But I can have a look at the smaller pieces if you want.

Yes, that's fair. Ok, so then we'll modify slightly: I'll ping you for review on each of the smaller stacked PRs instead and you're free to review them (or not) at your leisure. I'll leave them open for as long as I can, but if I'm worried about merge conflicts before you have a chance to get to it, I'll still just merge them down. You can nonetheless provide feedback on them post-merge, and we can simply slap on patching branches when it's convenient, ala #535.