Test Workflows again

[jan-janssen]

No description provided.

[jan-janssen]

@liamhuber I was able to fix the workflow tests, the main difference is that my previous tests use the with statement to start and close the executor while in your tests it is only shutdown implicitly at the end of the test rather than explicitly calling shutdown(). This was fixed in #211 . Still I am not exactly sure if we need all six of these tests.

[liamhuber]

I was able to fix the workflow tests, the main difference is that my previous tests use the with statement to start and close the executor while in your tests it is only shutdown implicitly at the end of the test rather than explicitly calling shutdown(). This was fixed in #211.

@jan-janssen , that is great! We definitely want either syntax to be supported, so this is a positive change 🚀

Still I am not exactly sure if we need all six of these tests.

I mean, we definitely don't need them to all be in separate files -- that was just a (misguided, ultimately) attempt to see which ones were hanging and which not. The different tests are checking for different things though. For instance, an earlier iteration of the executor worked fine when the callable was dynamically defined but broke if one of the arguments being passed was dynamically defined, so I would be much more comfortable keeping each of them.

I'll go back to #187, merge in main, and recombine and polish the tests.

[jan-janssen]

@jan-janssen , that is great! We definitely want either syntax to be supported, so this is a positive change 🚀

I am not sure if we want to support both syntax, because it hides the fact that a secondary process is running in the background. I would prefer to use only the with statement if possible. Just like we primarily use the with statements when accessing files.

[liamhuber]

@jan-janssen , that is great! We definitely want either syntax to be supported, so this is a positive change 🚀

I am not sure if we want to support both syntax, because it hides the fact that a secondary process is running in the background. I would prefer to use only the with statement if possible. Just like we primarily use the with statements when accessing files.

So first a giant disclaimer: I am really still just learning about concurrent stuff, so what I'm saying may be waaaay off the rails dumb here.

With that out of the way...are you sure? As far as I understand, there are definitely cases where we exactly want to hide that there is a secondary process running! My understanding of the with context for these executors is that exiting the context requires the background process to be complete. I only did a bit of reading, but what convinced me of this is the following simple toy example:

with this function tucked away in a file scratch.py

def return_42():
    from time import sleep
    sleep(1)
    return 42

I get the following, perfectly sensible and useful output:

from concurrent.futures import ProcessPoolExecutor

from scratch import return_42

with ProcessPoolExecutor() as p:
    fs = p.submit(return_42)
    print(fs.done())
    fs.result()
    print(fs.done())
>>> False
>>> True

But if I try to do anything outside the with context, I have to wait for it to resolve

from concurrent.futures import ProcessPoolExecutor

from scratch import return_42

with ProcessPoolExecutor() as p:
    fs = p.submit(return_42)
print(fs.done())
fs.result()
print(fs.done())
>>> True
>>> True

I have no complaint about that, it's a perfectly reasonable way for with to behave. My argument is that there are times we want to keep going and do other stuff while the background process resolves but aren't able to write all of it inside the with clause!

Consider the following MWE that basically mocks up what my nodes are doing:

# from pympipool.mpi.executor import PyMPIExecutor as Executor
from pyiron_workflow.executors import CloudpickleProcessPoolExecutor as Executor

class Foo:
    def __init__(self):
        self.running = False
        
    def run(self):
        self.running = True
        print("Inside", self.running)
        
        executor = Executor()
        future = executor.submit(self.return_42)
        future.add_done_callback(self.finished)
        
        # Using `with` gives the undesirable result for both executors
        # with Executor() as executor:
        #     future = executor.submit(self.return_42)
        #     future.add_done_callback(self.finished)
        
        return future
        
    def return_42(self):
        from time import sleep
        sleep(1)
        return 42
    
    def finished(self, future):
        self.running = False
    
foo = Foo()
print("Before", foo.running)
fs = foo.run()
print("While", foo.running)
fs.result()  # Wait for completion
print("After", foo.running)

If I use the pyiron_workflow executor and don't wrap things in a with, I get exactly the desired result:

Before False
Inside True
While True
After False

If instead I use the with clause, I get an undesirable result where Foo.run holds everything up until the process is complete:

Before False
Inside True
While False
After False

Now, again, that's totally fair! It just isn't what I want. run is getting invoked from other places (parent nodes, the notebook, etc), sometimes we have executors sometimes we don't, and it's not reasonable to try and put everything else here inside a with -- I just want to fire off that node's process and keep going with other stuff.

What's really troubling me is that pympipool.mpi.executor.PyMPIExecutor is giving me the second, undesirable result in both cases.

What I'm hoping to get from you:

• Is my argument in favour of supporting both with and with-out convincing, or do I still have some misunderstanding that can justify only supporting with?
• Do you have any immediate insight into why PyMPIExecutor is acting like it's got a with context in both cases? I'm willing to dig into this more deeply myself later, but I typically take a 10:1 ratio for asking for help on stuff and I think there's a very real chance that you can explain in <6 minutes what would take me >1 hour to learn from digging through the code base here! 🤞

[jan-janssen]

• Is my argument in favour of supporting both with and with-out convincing, or do I still have some misunderstanding that can justify only supporting with?

So far I have primarily used the with statement based on the intention that it should be visible to the user when something is running in the background and when the background tasks are completed.

• Do you have any immediate insight into why PyMPIExecutor is acting like it's got a with context in both cases? I'm willing to dig into this more deeply myself later, but I typically take a 10:1 ratio for asking for help on stuff and I think there's a very real chance that you can explain in <6 minutes what would take me >1 hour to learn from digging through the code base here! 🤞

I had a discussion about the add_done_callback() function with @pmrv in #94 and I thought this was fixed in #104 . But apart from the tests I added in that pull request I have not used add_done_callback() a lot.

[liamhuber]

• Is my argument in favour of supporting both with and with-out convincing, or do I still have some misunderstanding that can justify only supporting with?

So far I have primarily used the with statement based on the intention that it should be visible to the user when something is running in the background and when the background tasks are completed.

Ok, then it sounds like supporting both is the right choice. Indeed, the second issue of callbacks is tightly linked to this since a callback firing is a perfectly reasonable way for a user to gain visibility that their task has completed.

I had a discussion about the add_done_callback() function with @pmrv in #94 and I thought this was fixed in #104 . But apart from the tests I added in that pull request I have not used add_done_callback() a lot.

Looks like it wasn't the callbacks exactly, but the shutdown of the queue. This has its own issue and PR now.