Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix node registration #53

Merged
merged 40 commits into from
Oct 31, 2023
Merged

Fix node registration #53

merged 40 commits into from
Oct 31, 2023

Conversation

liamhuber
Copy link
Member

@liamhuber liamhuber commented Oct 26, 2023

Adds the node registration method back, although at the moment it's just called in Creator.__init__ so you don't see it.

TODO:

  • Tests
  • Think about what API you want to support -- should we also handle lists of nodes, or modules containing a nodes attribute (as long as it has the right type), or URLs? (EDIT: Right now I'm still only accepting string import paths to modules that have a nodes: list[Node] property, but the Creator.register method has good encapsulation of the logic for checking whether (a) newly proposed packages are just a re-registration of an existing package, and (b) whether the proposed package will actually be usable for generating nodes, so extending this in the future to other node package identifiers should be easy. For now I want to prioritize getting any fix in so we at least have some way to register things!)
  • Stop registering atomistics by default and update the demo and docs as needed
  • More gracefully handle re-registering things -- if it's the same nodes being set to the same domain name, just let it pass silently! This may be helpful when...
  • We encourage somewhere in the docs that macros register the necessary packages inside the macro definition whenever the macro uses .create... (i.e. unless the macro is part of the same .py file that all its children are coming from, in which case they should be available in-scope without .create

Outside of scope:

  • Actually supporting all manner of registration types (like a URL) -- I just want infrastructure that will be flexible enough to handle that in the future, not to get it all solved now!

Closes #51

When single value node fails to find an attribute on its single value
And import and create node packages as requested based off these references
For now at least, to keep behaviour the same
@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_workflow/fix_node_registration

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Pull Request Test Coverage Report for Build 6700105758

  • 66 of 67 (98.51%) changed or added relevant lines in 4 files are covered.
  • 53 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.05%) to 87.774%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_workflow/interfaces.py 41 42 97.62%
Files with Coverage Reduction New Missed Lines %
node_library/standard.py 1 95.65%
pyiron_workflow/node_library/standard.py 2 92.31%
channels.py 7 91.35%
function.py 8 91.57%
interfaces.py 8 87.18%
composite.py 9 91.36%
node_library/atomistics.py 18 0.0%
Totals Coverage Status
Change from base Build 6620662485: -0.05%
Covered Lines: 3209
Relevant Lines: 3656

💛 - Coveralls

@codacy-production
Copy link

codacy-production bot commented Oct 26, 2023

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-1.15% (target: -1.00%) 81.43%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (091abf1) 1802 1501 83.30%
Head commit (6189e02) 1843 (+41) 1514 (+13) 82.15% (-1.15%)

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 (#53) 70 57 81.43%

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

So updates to Function and SingleValue happen in a single place
@liamhuber liamhuber added the format_black trigger the Black formatting bot label Oct 26, 2023
@liamhuber liamhuber removed the format_black trigger the Black formatting bot label Oct 27, 2023
@liamhuber liamhuber added the format_black trigger the Black formatting bot label Oct 27, 2023
@liamhuber
Copy link
Member Author

The 3.9 tests break:

...
TypeError: unsupported operand type(s) for |: 'type' and 'type'

The above exception was the direct cause of the following exception:
...
ValueError: The package identifier is pyiron_workflow.node_library.standard is not valid. Please ensure it is an importable module with a list of Node objects stored in the variable `nodes`.

At first I was confused why importing from pyiron_workflow.node_library.standard would not work, but scrolling up the trace it's actually breaking when trying to process the type hint. This makes sense since 3.9 doesn't handle | but requires typing.Union, hence why we brute-force skip almost all the tests until the version is >= 3.10.

This failure happens in:

ERROR: executors.test_cloudprocesspool (unittest.loader._FailedTest)
ERROR: test_macro (unittest.loader._FailedTest)
ERROR: test_util (unittest.loader._FailedTest)
ERROR: test_workflow (unittest.loader._FailedTest)

In each case starting with

ImportError: Failed to import test module: [test module name]

So I guess I've somehow added a "bad" type hint somewhere before we get to the skipif version guard.

@liamhuber
Copy link
Member Author

The problem is when Creator tries to register the standard node package, which makes enough sense since we parse the type hints there and definitely use the new style.

What doesn't yet make sense is why there's a creator existing in, e.g., the tests/unit/executors/test_cloudprocesspool? And since the standard registration happens in __init__ is there really one of these being initialized, or is this some strange side-effect of the Creator being a Singleton?

@liamhuber
Copy link
Member Author

Ah, ok, I think maybe because right in pyiron_workflow.__init__ we import Workflow, which inherits from Composite, which instantiates a Creator as a class attribute.

We no longer get away with this because now we typing.get_type_hints as part of the decorator creating new node classes.

Alright, so all perfectly sensible now how to fix it.

For packages that rely on type hinting only available in >=3.10
@liamhuber liamhuber added format_black trigger the Black formatting bot and removed format_black trigger the Black formatting bot labels Oct 30, 2023
@liamhuber
Copy link
Member Author

Notebook failure is expected since I stopped registering atomistics by default and broke the scatter plot node over to its own package.

@codacy-production
Copy link

codacy-production bot commented Oct 30, 2023

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-1.15% (target: -1.00%) 81.43%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (091abf1) 1802 1501 83.30%
Head commit (23b8827) 1843 (+41) 1514 (+13) 82.15% (-1.15%)

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 (#53) 70 57 81.43%

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@liamhuber liamhuber added format_black trigger the Black formatting bot and removed format_black trigger the Black formatting bot labels Oct 30, 2023
@liamhuber
Copy link
Member Author

Codacy Coverage Report — -1.15% coverage variation (-1% target)

Coverage is just angry because pyiron_workflow.node_library.atomistics is no longer getting caught in the unit tests, but that's kind of the point; the unit tests are meant to test the infrastructure not the nodes we build on top of it. When the nodes get spun off to their own repos they can have their own tests.

@liamhuber liamhuber marked this pull request as ready for review October 31, 2023 00:20
@liamhuber liamhuber merged commit bb79722 into main Oct 31, 2023
13 of 15 checks passed
@liamhuber liamhuber deleted the fix_node_registration branch October 31, 2023 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black trigger the Black formatting bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Re)allow registering new node packages
2 participants