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

Validate IO #131

Merged
merged 47 commits into from
Nov 28, 2022
Merged

Validate IO #131

merged 47 commits into from
Nov 28, 2022

Conversation

liamhuber
Copy link
Member

@liamhuber liamhuber commented Nov 22, 2022

Layer type checking on top of ports.

At a high level:

Implementation TODOs:

  • Modify DTypes
    • Import and inherit from ryvencore.dtypes.DType
    • Add valid_instance_classes
    • Add an __eq__ or match or something operator that compares class or instances
    • Implement a bunch of dtypes, overriding the matching criteria as needed (e.g. for Choice)
  • Spoof Session, Script, and Flow as described above, so that you can add the match criterion to check_connection_validity whenever the input has a dtype
  • Handle allowing None for dtypes
  • Override NodeOutput to make it work with dtypes just like NodeInput
  • Add create_output_dt to my Node
  • Override setup_ports in my Node
  • (De)serialize the new dtype fields
  • DEBUG: get connections working again
  • Add a front-end check to PortWidget to re-colour it when the value matches the dtype
  • Add an input_ready method to Node that checks the match of all input ports
    • Use it in the Lammps node to avoid a run unless all input is present
      • Also add delete_existing_job=True for now, since we don't otherwise handle loading jobs nicely.
  • Tests would be wise (not there for everything, but at least most of the new stuff)
  • Update demo notebooks (old nodes might have some loading after this)

@liamhuber liamhuber added the enhancement New feature; can be answer partner of "feature request" label Nov 22, 2022
@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch pyiron/ironflow/validate_io

@liamhuber
Copy link
Member Author

liamhuber commented Nov 22, 2022

Reminder to self: A quick end-of-day notebook test showed this introducing a nasty performance hit once it's actually rendered, even though the tests run as fast as ever. Try again after rebooting and if the problem persists, fix it.

Edit: performance is fine after rebooting.

Only applied it to the Lammps job output so far.
The blueprint always has a dtype, but non-dt output still doesn't. (NodeInput always does, and I could make it so NodeOutput always did, but it's a trade-off between consistency and needing to wrap less of the ryven library. Today, I choose wrapping less.)
🐛 stops the match check from borking when one has no valid classes
This is what was making it impossible to connect nodes; the connection method threw an error and stopped the connection process.
Since it always has them now, we need to modify some checks
Now that we *always* have the dtype attribute (even if it's sometimes None), we can simplify the port setup and API a bit.
Otherwise the validity of inputs gets broken by users setting what looks like a string!
Those that have corresponding widget types at least
Where poorly defined means there is no one-to-one correspondance between the dtype and the widget
They were there in the type checking import, so my IDE didn't pick up the problem. This solves the issue of the lammps node buttons not working.
Standard representations are always included now, and in this case include the structure which has the same effect.
@liamhuber liamhuber marked this pull request as ready for review November 24, 2022 22:06
@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 the format_black Invoke black formatting CI label Nov 28, 2022
@liamhuber liamhuber merged commit 3c76ae1 into main Nov 28, 2022
@liamhuber liamhuber deleted the validate_io branch November 28, 2022 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature; can be answer partner of "feature request" format_black Invoke black formatting CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants