search

pyiron / pyiron_workflow

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

Using macOS-11 for CI results in long wait times #387

Closed jan-janssen closed 3 months ago

jan-janssen commented 4 months ago

Summary

Both executorlib and previously pympipool could already use macos-latest since the introduction of the hostname_localhost parameter at the beginning of the year: https://github.com/pyiron/executorlib/pull/258

pyiron Version and Platform

macOS

Expected Behavior

By adding the hostname_localhost parameter for the CI, the CI wait time is reduced.

Actual Behavior

Currently macos-11 is required as later versions like macos-14 do not allow connections to localhost on macOS.

Steps to Reproduce

It happens at every pull request

Further Information, Files, and Links

liamhuber commented 4 months ago

The only reason I used macos-11 was to accommodate pympipool, so I am totally fine updating it to macos-latest -- it had just been so long since I looked at https://github.com/pyiron/executorlib/pull/258 that I forgot about it!

In #388 the macos-latest tests started basically right away; whereas the hang for macos-11 without the localhost business was effectively infinite. From the issue it's a bit unclear -- is macos-11+localhost even faster than macos-latest? I see you use latest in executorlib, so my guess is this is your recommended course of action here too?

jan-janssen commented 4 months ago

In #388 the macos-latest tests started basically right away; whereas the hang for macos-11 without the localhost business was effectively infinite. From the issue it's a bit unclear -- is macos-11+localhost even faster than macos-latest? I see you use latest in executorlib, so my guess is this is your recommended course of action here too?

No macos-latest requires hostname_localhost otherwise the tests fail.

liamhuber commented 4 months ago

Ok, that's not a fundamental problem, but it does mean I want to change the default Workflow.create.Executor away from executorlib.Executor, since I'd need to add a bunch of executorlib-specific kwargs throughout the tests. I do this in #389, where we still use the executorlib one in notebooks (process pool fundamentally cannot handle that case since locally defined nodes become unimportable).

Since ProcessPoolExecutor doesn't work in notebooks, and notebooks are the key expected use case for the package, I'm now considering getting rid of the Creator.Executor entirely and making users explicitly choose one instead of giving a biased "default".