Skip to content

Migrate flowrep.tools and parts of flowrep.workflow #410

Merged
liamhuber merged 44 commits intomainfrom
flowrep-dict
Mar 26, 2026
Merged

Migrate flowrep.tools and parts of flowrep.workflow #410
liamhuber merged 44 commits intomainfrom
flowrep-dict

Conversation

@liamhuber
Copy link
Copy Markdown
Member

@liamhuber liamhuber commented Mar 18, 2026

Depends on pyiron/flowrep#185.

With this, all remaining external references to flowrep go through flowrep.models.api, and we can remove flowrep.workflow (parts of which now live here) and flowrep.tools (all of which now lives here) and move models up to the root. @samwaseda, I copied the functions with minimal changes, brought their tests with them, and tagged you as a co-author.

I had to touch a bunch of the semantikon tests, but only in two trivial ways pertaining to running with the flowrep toy WfMS:

  1. It imports stuff from the recipe, so I had to move these out of <locals>
  2. It only takes **kwargs not *args, **kwargs, so I added keywords to all the run calls

To put this in context, I've been working so far from the bottom-up with structuring the unstructured data. Here we have a structured-to-dict conversion point. Now, I would work top-down inside semantikon to have the callers of the dictionaries start to consume structured data instead (either flowrep structured data directly, or new classes defined here), until we don't have any raw dictionaries left. I've found this and my last couple PRs here very promising that this can be done with good continuity in the structure of the test suite and how things look and behave for users -- i.e. I think we can get this migration done without breaking anything at any point along the road.

liamhuber and others added 15 commits March 17, 2026 11:08
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
From `flowrep.workflow`, along with everything it calls, its tests, and calls and tests recursively. Winds up being a handful of functions and a couple tests. All originally written by:

Co-authored-by: Sam Dareska <37879103+samwaseda@users.noreply.github.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Privatize the migrated functions that are exclusively used internally (and in tests) to support `get_hashed_node_dict`

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
This is just a direct migration of the module and tests originally written by:

Co-authored-by: Sam Dareska <37879103+samwaseda@users.noreply.github.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link
Copy Markdown

Binder 👈 Launch a binder notebook on branch pyiron/semantikon/flowrep-dict

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
@liamhuber liamhuber marked this pull request as ready for review March 19, 2026 22:24
@liamhuber liamhuber requested a review from samwaseda March 19, 2026 22:24
@liamhuber
Copy link
Copy Markdown
Member Author

@samwaseda, on my local machine visualize, qudt, ontology, and analysis are all throwing mypy errors too. TBH I'm neither super concerned nor super motivated to fix them here.

Otherwise this is working nicely, and relies only on the flowrep.models.api interface now.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 99.70149% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.64%. Comparing base (70b1133) to head (be6d36d).
⚠️ Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
semantikon/flowrep_dict.py 99.69% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   97.47%   97.64%   +0.16%     
==========================================
  Files          10       11       +1     
  Lines        1584     1908     +324     
==========================================
+ Hits         1544     1863     +319     
- Misses         40       45       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Migrates Semantikon’s remaining direct dependencies on flowrep.tools / flowrep.workflow to the flowrep.models.api surface, introducing an in-repo compatibility layer (semantikon.flowrep_dict) to convert Flowrep live nodes into the legacy nested-dict format consumed by Semantikon.

Changes:

  • Add semantikon/flowrep_dict.py to convert Flowrep live node trees to the legacy workflow dict format and to host hashing/metadata utilities previously sourced from Flowrep.
  • Update Semantikon workflow decorator/runtime path to use flowrep.models.api (parsers, schemas, wfms) and the new converter layer.
  • Update unit tests, example notebook, and environment/dependency pins to Flowrep 0.3.0 and keyword-only run calls.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
semantikon/flowrep_dict.py New compatibility layer: live-node → legacy dict conversion + hashing/metadata utilities
semantikon/workflow.py Switch workflow decorator/run to Flowrep v0.3 APIs and new dict conversion
semantikon/ontology.py Update hashed-node import source to semantikon.flowrep_dict
semantikon/converter.py Update function-metadata import source to semantikon.flowrep_dict
tests/unit/test_flowrep_dict.py New tests covering flowrep_dict conversion, graph helpers, hashing, and utilities
tests/unit/test_workflow.py Update serialization import and adjust workflow validation expectations
tests/unit/test_ontology.py Move workflow fixtures out of locals and convert .run() calls to keyword args
tests/unit/test_analysis.py Convert .run() calls to keyword args; move workflow fixtures out of locals
pyproject.toml Pin flowrep==0.3.0
notebooks/knowledge_graph.ipynb Remove Flowrep legacy dict example; update .run() call style
docs/environment.yml Pin flowrep=0.3.0
.ci_support/lower_bound.yml Pin flowrep=0.3.0
.ci_support/environment.yml Pin flowrep=0.3.0
.binder/environment.yml Pin flowrep=0.3.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread semantikon/flowrep_dict.py Outdated
Comment thread semantikon/workflow.py
Comment thread semantikon/flowrep_dict.py Outdated
Comment thread semantikon/flowrep_dict.py Outdated
Comment thread semantikon/flowrep_dict.py Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Satisfies 5 mypy complaints, all about indexed assignments of type `Collection[Any]`. This is a very broad hint, but at least this way the value is not lying, and mypy reassures us the keys really are strings throughout.

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
They are not valid input for .asdict

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
I don't know how you'd get yourself in this situation, so I can't provide a more informative error than this, but the subsequent edge-string creation will certainly break if you have any None values left here. Silence mypy by failing hard.

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
@liamhuber
Copy link
Copy Markdown
Member Author

@samwaseda I made some small changes to get it mypy compliant and extended the coverage far enough to get the green ticks -- there are some untested logical branches inside the digraph conversions that still aren't hit, but I don't grok those well enough to see how to construct examples that reach them.

@samwaseda
Copy link
Copy Markdown
Member

@samwaseda I made some small changes to get it mypy compliant and extended the coverage far enough to get the green ticks -- there are some untested logical branches inside the digraph conversions that still aren't hit, but I don't grok those well enough to see how to construct examples that reach them.

@copilot Address the last part of Liam's comment

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 24, 2026

@samwaseda I've opened a new pull request, #414, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 7 commits March 24, 2026 15:28
Add tests for uncovered branches in digraph conversions
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
# Conflicts:
#	notebooks/knowledge_graph.ipynb
liamhuber added a commit to pyiron/flowrep that referenced this pull request Mar 24, 2026
Everything that was used downstream in semantikon is migrated into semantikon in pyiron/semantikon#410

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
# Conflicts:
#	notebooks/knowledge_graph.ipynb
@samwaseda
Copy link
Copy Markdown
Member

It looks like knowledge_graph.ipynb was not executed. Is there a particular reason for this?

@liamhuber
Copy link
Copy Markdown
Member Author

It looks like knowledge_graph.ipynb was not executed. Is there a particular reason for this?

Looks like it ran fine to me. From the latest CI build notebooks:

Screenshot 2026-03-25 at 10 00 43 AM

@samwaseda
Copy link
Copy Markdown
Member

samwaseda commented Mar 25, 2026

No I mean you uploaded an unexecuted notebook

@liamhuber
Copy link
Copy Markdown
Member Author

No I mean you uploaded an unexecuted notebook

Oh, that is just a user error. Or something that happened during merge conflict resolution. Either way, I'll execute and upload. I also find it nice to see the executed one when browsing on GitHub.

For output rendering right in the GitHub repo/readthedocs s.t. it can just be read

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
@samwaseda
Copy link
Copy Markdown
Member

samwaseda commented Mar 26, 2026

Here Copilot correctly pointed out that memory locations etc should not be included in the notebooks because they change after each execution. You can delete print(workflow_dict) from the notebook and execute it again

Edit: I did it myself

@liamhuber liamhuber merged commit 6df56f5 into main Mar 26, 2026
21 of 22 checks passed
@liamhuber liamhuber deleted the flowrep-dict branch March 26, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants