Workflow: Merge changes I made so far

[samwaseda]

I open this one now because otherwise this branch only becomes larger and deviates from main.

[github-actions]

👈 Launch a binder notebook on branch pyiron/pyiron_contrib/submittable_workflow

[liamhuber]

I open this one now because otherwise this branch only becomes larger and deviates from main.

Super. I was thinking this too! I lean towards merging this before merging #729, and I'll handle the merge to rebase this stuff onto the new abstract class.

[liamhuber]

I'll hold off a full review until we get the stacked PRs merged in here?

Taking a quick glance, one thing I think would be really nice is having a concrete example in notebooks/workflow_example.ipynb of the new node-with-self functionality being used. If I think of a good one today I'll post it here.

[github-actions]

Pull Request Test Coverage Report for Build 5382047220

• 23 of 25 (92.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.07%) to 11.89%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_contrib/workflow/node.py	23	25	92.0%

Totals
Change from base Build 5357482413:	0.07%
Covered Lines:	1671
Relevant Lines:	14054

---

💛 - Coveralls

[samwaseda]

I'll hold off a full review until we get the stacked PRs merged in here?

Is there a particular reason? I have the feeling that they make this PR only larger

Taking a quick glance, one thing I think would be really nice is having a concrete example in notebooks/workflow_example.ipynb of the new node-with-self functionality being used. If I think of a good one today I'll post it here.

So far there's nothing that really changes the functionality. As soon as there's a possibility to set it up (especially the server), I would say we can add it.

[liamhuber]

I'll hold off a full review until we get the stacked PRs merged in here?
Is there a particular reason? I have the feeling that they make this PR only larger

The reason had been that tests were failing and I thought this needed the other stuff. It looks like the only other connected PR is #725. I'll just review each separately, but I think we can merge both soon.

So far there's nothing that really changes the functionality. As soon as there's a possibility to set it up (especially the server), I would say we can add it.

The ability to include self is definitely a functionality change though, and an example can be included independently. Probably in the easiest case it will need to be a sort of "counter example" in that using self will violate the "functional" aspect of the node by introducing some state -- e.g. we could make a node with an internal counter that keeps track of how many times it's been called. There should be some example of the new functionality though.

Regarding Server, I guess it will be hard to make a solid example there, because queues need to be set up locally... What do you think of unwinding the Server changes in this PR and having it be just the self stuff? Then we could make a separate PR where we introduce Executor instead of Server and exploit Marvin's Dask executor form #718 to build an architecture-independent example of running a node task in the background? Look at the diff, unwinding the server stuff would only involve removing the import and self.server= assignment in node.py and workflow.py, so it's a pretty easy change.

[liamhuber]

Ok, I basically said everything I had to say in the non-review-comment five minutes ago 😝 Here I just act on it.

Purge server and add example to the ipynb at it's good-to-go.

[samwaseda]

The reason had been that tests were failing and I thought this needed the other stuff. It looks like the only other connected PR is #725. I'll just review each separately, but I think we can merge both soon.

Ah yeah and the reason why it was failing was because the tests are apparently not triggered when the PR is not to be merged to main. There's certainly a way to make them run, but at least for now that's yet another reason why I would like to merge directly to main.

The ability to include self is definitely a functionality change though, and an example can be included independently. Probably in the easiest case it will need to be a sort of "counter example" in that using self will violate the "functional" aspect of the node by introducing some state -- e.g. we could make a node with an internal counter that keeps track of how many times it's been called. There should be some example of the new functionality though.

Hmm on the other hand there are only counter examples and nothing that enhances the capabilities. Since it's relatively obvious that self should not be used, I guess we can wait until further functionalities are added??

Regarding Server, I guess it will be hard to make a solid example there, because queues need to be set up locally... What do you think of unwinding the Server changes in this PR and having it be just the self stuff? Then we could make a separate PR where we introduce Executor instead of Server and exploit Marvin's Dask executor form #718 to build an architecture-independent example of running a node task in the background? Look at the diff, unwinding the server stuff would only involve removing the import and self.server= assignment in node.py and workflow.py, so it's a pretty easy change.

Fine by me.

[liamhuber]

Hmm on the other hand there are only counter examples and nothing that enhances the capabilities. Since it's relatively obvious that self should not be used, I guess we can wait until further functionalities are added??

Mmmm, I hear you.... but right now I'm really gearing example_workflow.ipynb towards developers rather than users, i.e. so people like Marvin and Joerg have a single point of entry where they can come familiarize themselves with all the functionality. From this perspective I feel a counter-example is actually still quite productive.

I can see you might be right and this isn't a hill I need to die on, so If you still disagree with me then let's just drop it and merge sans example, but I hope I was sufficiently persuasive in the last paragraph 😂

[samwaseda]

Mmmm, I hear you.... but right now I'm really gearing example_workflow.ipynb towards developers rather than users, i.e. so people like Marvin and Joerg have a single point of entry where they can come familiarize themselves with all the functionality. From this perspective I feel a counter-example is actually still quite productive.

Ah in that case I would rather add comments inside the code, because the code itself is a lot more visible to someone like @pmrv.

[liamhuber]

Ah in that case I would rather add comments inside the code, because the code itself is a lot more visible to someone like @pmrv.

Good point, that is an even better solution.

[samwaseda]

Now I updated the docs. I guess it's ready now.

[liamhuber]

Now I updated the docs. I guess it's ready now.

Can we modify test_with_self so that it actually assigns something to self and then verify after the run that this is present? I believe this will work fine, I would just sleep better if there's a test; e.g. to prevent changes to Node.__setattr__ from quietly breaking our expectations of what users can do by adding self.

EDIT: But otherwise, yes, that was the single outstanding thing for me. The rest is looking great!

[liamhuber]

I'd optimally still like to see test_with_self be more thorough as described here, but don't want my old "request changes" to block merging.

[samwaseda]

Do you have an idea how to use self twice? Essentially having the self from the function and self from the unit tests?

[liamhuber]

Do you have an idea how to use self twice? Essentially having the self from the function and self from the unit tests?

It gets locally scoped, so it's a non-issue.

I thought you might have (very fairly!) stopped for the day, so I implemented what I had in mind in #742. Lmk what you think!