pyiron / pyiron_workflow

Graph-and-node based workflows
BSD 3-Clause "New" or "Revised" License
12 stars 1 forks source link

[minor] Extract responsibility for `run` to its own class #246

Closed liamhuber closed 7 months ago

liamhuber commented 8 months ago

This continues #243 by pulling out responsibility for handling run behaviour, that means having a status and talking to the future and handling callbacks. Runnable doesn't know anything about IO or graphs, so Node still implements all the different run modes (pulling, etc).

This is only intended to be a pure refactoring, so while I'm definitely interested in feedback on the architecture of Runnable, I might delay implementing changes that would echo into the downstream classes to a future PR.

~This is all under-the-hood, so no changes for users or node designers.~ Setting check_readiness=False in run will now run even if readiness failed due to a status issue; so you can really just brute-force run un-ready things with this flag.

github-actions[bot] commented 8 months ago

Binder :point_left: Launch a binder notebook on branch _pyiron/pyironworkflow/runnable

liamhuber commented 8 months ago

In the process I uncovered that the while meta-node is subject to recursion errors waaaay earlier than I'd like. I tested dropping the test threshold from 0.1 to 0.001 on main as well, and you hit the exact same recursion error, so this problem is quantitatively exacerbated by pulling out a superclass, but qualitatively unrelated to the changes in this PR; I'll open an issue and patch the meta-node's docstring with a heads-up separately. Actually solving the recursion problem will probably involve re-designing the while-loop.

codacy-production[bot] commented 8 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: +0.05% (target: -1.00%) :white_check_mark: 90.48%
Coverage variation details | | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (5918aced03a8a8708146b0d3171c9e16f1b35a6e) | 3236 | 2815 | 86.99% | | | Head commit (0e62ae0e6fb92478e42665dbd0a154ab854b84ef) | 3265 (+29) | 2842 (+27) | 87.04% (**+0.05%**) | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `
Diff coverage details | | Coverable lines | Covered lines | Diff coverage | | ------------- | ------------- | ------------- | ------------- | | Pull request (#246) | 105 | 95 | **90.48%** | **Diff coverage** is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: `/ * 100%`

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 8403924096

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_workflow/has_interface_mixins.py 3 4 75.0%
pyiron_workflow/run.py 86 95 90.53%
<!-- Total: 98 108 90.74% -->
Files with Coverage Reduction New Missed Lines %
has_interface_mixins.py 1 83.33%
node.py 19 93.16%
<!-- Total: 20 -->
Totals Coverage Status
Change from base Build 8403918323: -0.09%
Covered Lines: 6041
Relevant Lines: 6595

💛 - Coveralls