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

Support use of multiprocessing start methods other than "spawn" (e.g. "dragon") #554

Merged

Conversation

applio
Copy link
Contributor

@applio applio commented Aug 18, 2024

Currently, cubed.runtime.executors.local.async_execute_dag() hard codes the use of the "spawn" start method when employing multiprocessing / concurrent.futures processes. This PR proposes a means for the user to specify their preferred start method via the existing keyword argument use_processes.

This proposed change would permit users to select from the existing multiprocessing start methods of "fork", "spawn", and "forkserver" as well as the newer "dragon" HPC distributed execution start method provided by the Dragon project (https://github.com/dragonhpc/dragon). An example snippet showing how a different start method can now be specified:

cubed.to_zarr(
    some_data,
    store=zg,
    use_processes="dragon",
)

It probably makes sense to document this new functionality though it appears that the keyword argument, use_processes, does not yet appear anywhere in the documentation. The "Configuration" page (https://cubed-dev.github.io/cubed/configuration.html#processes) might be a good spot to describe use_processes in general along with this added control. I would be happy to propose some documentation text if others agree on where it ought to go.

Copy link
Member

@tomwhite tomwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @applio! It's great that this is the only change needed to run on Dragon!

I was wondering if there was some way of adding a unit test (without installing Dragon), but I'm not sure there is. So I'm happy to merge it as it stands.

The "Configuration" page (https://cubed-dev.github.io/cubed/configuration.html#processes) might be a good spot to describe use_processes in general along with this added control.

Yes that would be ideal - please add some docs to that page (either as a part of this or another PR).

@TomNicholas
Copy link
Member

It's great that this is the only change needed to run on Dragon!

Cool! Thanks @applio.

adding a unit test (without installing Dragon)

I think we should add unit tests which do install Dragon, but we can leave those to follow-up PRs.

@TomNicholas
Copy link
Member

It's great that it's this easy to get running on Dragon, but I do wonder whether this use_processes/spawn interface is just going to be confusing for users. I would like to be able to recommend different Executors to users based on the system they are trying to run on (i.e. "Cloud? Use lithops! Local machine? Use the local executor! HPC? Use the DragonExecutor!").

Whilst the use_processes is extremely neat for us developers I wonder if that subtlety should actually be hidden from the users behind a DragonExecutor abstraction, even if it uses similar codepaths under the hood.

@tomwhite
Copy link
Member

I agree with adding a DragonExecutor in a new dragon.py module. It would also be a natural place to add Dragon-specific configuration in the future.

@tomwhite
Copy link
Member

We discussed this in the meeting and decided to merge it as it may be generally useful for specifying multiprocessing start methods other than "spawn". @applio will still do a separate PR for the DragonExecutor as discussed above.

@tomwhite tomwhite merged commit 4ff032b into cubed-dev:main Sep 17, 2024
11 checks passed
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

Successfully merging this pull request may close these issues.

3 participants