DRAFT: poc to run mypy in CI on notebooks

[XzzX]

No description provided.

[github-actions]

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

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.01% (target: -1.00%)	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (a3d38f7)	3429	3133	91.37%
Head commit (712fdda)	3433 (+4)	3137 (+4)	91.38% (+0.01%)

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 (#575)	4	4	100.00%

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}

[XzzX]

@liamhuber What do you think? Could this solve your problem?

[liamhuber]

Yes, at least in part! 🚀 🚀 What I see missing is the ability to narrow the focus of deprecation, e.g. to specific (kw)args inside a given function. It's possible that typing_extensions.deprecated already has tools for this, I haven't dug into it yet. If not, an alternative may be to twist mypy's arm to recognize the deprecator in pyiron_snippets, which I'm pretty sure does let us narrow scope like this.

On pyproject.toml, I guess we may want different mypy config for the notebooks and the rest of the codebase: I'd like to get the rest of the codebase running on --strict, but not the notebooks where we probably want to leave stuff unhinted to not scare our least-experienced users; I guess we still want to test deprecated things so we probably don't want mypy complaining about the test suite using @deprecated functions; etc. I do like moving the ignore missing imports config there though, and anything else that proves completely universal.

[XzzX]

@liamhuber

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function"  [attr-defined]
tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree"  [attr-defined]

These are severe errors.

[XzzX]

Yes, at least in part! 🚀 🚀 What I see missing is the ability to narrow the focus of deprecation, e.g. to specific (kw)args inside a given function.

I like to be surprised here, but I do not think this is possible. kwargs is meant to be anything.

On pyproject.toml, I guess we may want different mypy config for the notebooks and the rest of the codebase: I'd like to get the rest of the codebase running on --strict, but not the notebooks where we probably want to leave stuff unhinted to not scare our least-experienced users;

If code is not annotated mypy tries to guess it or falls back to Any. We should run it also on notebooks. Checks will be less comprehensive but everything we catch is one error less. We do not need to annotate the notebooks.

I guess we still want to test deprecated things so we probably don't want mypy complaining about the test suite using @deprecated functions; etc.

True. See: 9f2e75e

I do like moving the ignore missing imports config there though, and anything else that proves completely universal.

💯

[liamhuber]

Yes, at least in part! 🚀 🚀 What I see missing is the ability to narrow the focus of deprecation, e.g. to specific (kw)args inside a given function.

I like to be surprised here, but I do not think this is possible. kwargs is meant to be anything.

Sorry, I was not very specific. One example of what I meant would be moving from def foo(some_arg, *, this_is_about_to_be_deprecated=42) to def foo(some_arg, *, this_is_what_we_call_it_now=42, this_is_about_to_be_deprecated=None). I want to catch places that still use the old kwarg this_is_about_to_be_deprecated and indicate that they should now be leveraging this_is_what_we_call_it_now.

On pyproject.toml, I guess we may want different mypy config for the notebooks and the rest of the codebase: I'd like to get the rest of the codebase running on --strict, but not the notebooks where we probably want to leave stuff unhinted to not scare our least-experienced users;

If code is not annotated mypy tries to guess it or falls back to Any. We should run it also on notebooks. Checks will be less comprehensive but everything we catch is one error less. We do not need to annotate the notebooks.

If we run mypy --strict it will throw an error if annotations are missing, not fall back on Any. We should run mypy on notebooks and not necessarily annotate them, and (#567) run mypy --strict on the codebase (and test suite too, I think). This is why I say we cannot have a single mypy configuration for all mypy jobs. I see from your next point that there is also exclusions; I don't have super strong feelings about defining --strict universally and then excluding it for notebooks all in the pyproject.toml vs adding it onto the centralized config only for mypy jobs that need it.

[liamhuber]

@liamhuber

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function"  [attr-defined]
tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree"  [attr-defined]

These are severe errors.

Severe in what sense? These seem very benign to me.

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function" [attr-defined]

It's just mad that we reference it without demanding it. We can just make it an abstract class property. Alternatively, we might be able to not reference it at all and simply set "_io_defining_function": staticmethod(io_defining_function), instead of "_decorated_function": staticmethod(io_defining_function),, but I don't have time to dig into this right now.

tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree" [attr-defined]

I don't even know why it's complaining about this. I'm just rescoping an imported class onto the test case class. If it really is never going to be happy we could just rescope it onto the test case instance instead of the test case class by moving the offending lines from setUpClass to setUp.

[XzzX]

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function" [attr-defined]

It's just mad that we reference it without demanding it. We can just make it an abstract class property. Alternatively, we might be able to not reference it at all and simply set "_io_defining_function": staticmethod(io_defining_function), instead of "_decorated_function": staticmethod(io_defining_function),, but I don't have time to dig into this right now.

According to your class definition I am allowed to call ScrapesFromDecorated._io_defining_function(). This will crash.

tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree" [attr-defined]

I don't even know why it's complaining about this. I'm just rescoping an imported class onto the test case class. If it really is never going to be happy we could just rescope it onto the test case instance instead of the test case class by moving the offending lines from setUpClass to setUp.

cls is a type, not an instance. There is no memory associated with cls. Therefore, you cannot append anything to it. Difference between class methods and instance methods.

[liamhuber]

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function" [attr-defined]

It's just mad that we reference it without demanding it. We can just make it an abstract class property. Alternatively, we might be able to not reference it at all and simply set "_io_defining_function": staticmethod(io_defining_function), instead of "_decorated_function": staticmethod(io_defining_function),, but I don't have time to dig into this right now.

According to your class definition I am allowed to call ScrapesFromDecorated._io_defining_function(). This will crash.

Look, of course, that's what I said. We can fix it by making the thing abstract and having the underlying, missing _decorated_function be abstractly required.

tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree" [attr-defined]

I don't even know why it's complaining about this. I'm just rescoping an imported class onto the test case class. If it really is never going to be happy we could just rescope it onto the test case instance instead of the test case class by moving the offending lines from setUpClass to setUp.

cls is a type, not an instance. There is no memory associated with cls. Therefore, you cannot append anything to it. Difference between class methods and instance methods.

I think you need to take off your compiled, strongly-typed hat, and put on your python hat. Python classes have a certain mutability:

class Foo:
    def __init__(self, x):
        self.x = x

Foo.y = 42

f = Foo(0)
print(f.x, f.y)

g = Foo(1)
Foo.y = None

print(g.x, g.y)
print(f.x, f.y)

I say again, this fine, mypy is just angry that it didn't know about the class attribute before hand. I didn't realize it would be this picky, but it can be resolved just like the other problem by indicating that such a class attribute exists.

[XzzX]

tests/unit/mixin/test_preview.py:16: error: "type[ScrapesFromDecorated]" has no attribute "_decorated_function" [attr-defined]

It's just mad that we reference it without demanding it. We can just make it an abstract class property. Alternatively, we might be able to not reference it at all and simply set "_io_defining_function": staticmethod(io_defining_function), instead of "_decorated_function": staticmethod(io_defining_function),, but I don't have time to dig into this right now.

According to your class definition I am allowed to call ScrapesFromDecorated._io_defining_function(). This will crash.

Look, of course, that's what I said. We can fix it by making the thing abstract and having the underlying, missing _decorated_function be abstractly required.

tests/unit/nodes/test_for_loop.py:130: error: "type[TestForNode]" has no attribute "AddThree" [attr-defined]

I don't even know why it's complaining about this. I'm just rescoping an imported class onto the test case class. If it really is never going to be happy we could just rescope it onto the test case instance instead of the test case class by moving the offending lines from setUpClass to setUp.

cls is a type, not an instance. There is no memory associated with cls. Therefore, you cannot append anything to it. Difference between class methods and instance methods.

I think you need to take off your compiled, strongly-typed hat, and put on your python hat. Python classes have a certain mutability:

class Foo:
    def __init__(self, x):
        self.x = x

Foo.y = 42

f = Foo(0)
print(f.x, f.y)

g = Foo(1)
Foo.y = None

print(g.x, g.y)
print(f.x, f.y)

I say again, this fine, mypy is just angry that it didn't know about the class attribute before hand. I didn't realize it would be this picky, but it can be resolved just like the other problem by indicating that such a class attribute exists.

You are the author, you are the code owner and most importantly you are the maintainer. Your code, your rules.
I am just reporting anything I consider bad practice and detremental to long-term maintainability, since I was explicitly asked to do this. And yes, this is within a test, so even a, in principle, severe error is not that severe.

[XzzX]

To summarize:
This instance of the error is not severe at all, agreed.
The error in general I would consider severe as it indicates a violation of design principles.
I know, it can be fixed easily. I would have done it myself, but I wanted to raise attention to some design princple I think one should adhere to.
Just my personal opinion.

[XzzX]

Do you want to merge #582 into this one?

[XzzX]

BTT: How do you want to proceed? Should I remove the dummy deprecations and we merge?

[liamhuber]

Do you want to merge #582 into this one?

Sure, but I based #582 off main since this PR had so much going on in it. In principle I'm content with what I did in #582 and I think it will resolve failures here so getting it in somehow is perfect.

[liamhuber]

BTT: How do you want to proceed? Should I remove the dummy deprecations and we merge?

• remove dummy deprecations
• resolve conflict with main
• pull in Mypy tests too #582

Then if there are further test failures, either fix them or ping me if you're not clear how. For things that involve architecture changes or changes to the UI, I would appreciate getting a ping on the plan just to avoid you sinking time into work that I have some deep issue with; for stuff like #582 where the functionality is all the same but we're just being more rigorous, I don't anticipate ever having serious objections so just go for it if the path forward is clear to you.

[XzzX]

-> #590