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

Why are running and failed still coexisting? #461

Open
samwaseda opened this issue Sep 19, 2024 · 1 comment
Open

Why are running and failed still coexisting? #461

samwaseda opened this issue Sep 19, 2024 · 1 comment

Comments

@samwaseda
Copy link
Member

I remember to have talked about it some time ago, but doesn't it make more sense to do like in pyiron_base to define one class for the states, like initialised, running, failed, finished? If I understand the current implementation correctly, it is technically not impossible to set both running and failed to true, but it should be logically impossible right? Also I find it difficult to understand what are possible states, because running and failed are listed among a lot of items on the base level of Node.

@liamhuber
Copy link
Member

The short answer is: yes, there should absolutely be an abstraction coupling running and failed and making them mutually exclusive. But it's not broken as-is and doing this has been low-priority enough that it just hasn't happened.

doesn't it make more sense to do like in pyiron_base to define one class for the states, like initialised, running, failed, finished?

I like the way base has a state object that shows these attributes are all connected and makes them mutually exclusive, but I don't like it enough to just copy-paste it over here. They also don't all apply:

  • initialised: doesn't even make sense here, nodes are much simpler than jobs
  • running: yep
  • failed: yep
  • finished: We could have it, but I'm not convinced it makes as much sense. You can re-run nodes multiple times, so it's not really mutually exclusive with initialised, and I don't know that it's meaningful to track if it's been run. Also it seems very redundant to node.outputs.ready

it is technically not impossible to set both running and failed to true, but it should be logically impossible right?

Correct on all counts! But unless you run into a case where they are both true, fixing it will remain low-priority for me.

Also I find it difficult to understand what are possible states, because running and failed are listed among a lot of items on the base level of Node.

That's totally fair. They're the only two states in their mutually exclusive group. If mutual exclusion wasn't how we're grouping things, stuff like ready and import_ready could also be included in the state-ness collection.

I've tried super hard to keep "because history" out of the explanations for the codebase here, but honestly this is one place where it snuck in. I started with just failed as a bool, later introduced running as a bool but didn't have time to abstract them together, and then just stopped touching that corner of the code base for a long, long time. I totally agree an abstraction would be nice here, but I'm not sure I'll get it (and I might even make things slightly worse in the short term trying to get the functionality of #458 merged).

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

No branches or pull requests

2 participants