aiidateam / plumpy

A python workflows library that supports writing Processes with a well defined set of inputs and outputs that can be strung together.
https://plumpy.readthedocs.io
Other
8 stars 17 forks source link

♻️ Make `Process.run` async #272

Open chrisjsewell opened 1 year ago

chrisjsewell commented 1 year ago

As discussed with @giovannipizzi, to allow for introducing async methods to the AiiDA Transport classes (particularly for uploading and downloading files)

The downstream implementation and explanation in aiida-core is here: https://github.com/aiidateam/aiida-core/pull/6079

(Also makes me think, can you also allow for optional async Workflow steps this way)

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (49a7117) 90.82% compared to head (0b30f35) 90.82%.

:exclamation: Current head 0b30f35 differs from pull request most recent head ff1b699. Consider uploading reports for the commit ff1b699 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## support/0.21.x #272 +/- ## ================================================== - Coverage 90.82% 90.82% -0.00% ================================================== Files 21 21 Lines 2973 2972 -1 ================================================== - Hits 2700 2699 -1 Misses 273 273 ``` | [Impacted Files](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [src/plumpy/futures.py](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS9mdXR1cmVzLnB5) | `74.47% <100.00%> (ø)` | | | [src/plumpy/process\_states.py](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS9wcm9jZXNzX3N0YXRlcy5weQ==) | `88.26% <100.00%> (ø)` | | | [src/plumpy/processes.py](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS9wcm9jZXNzZXMucHk=) | `92.46% <100.00%> (ø)` | | | [src/plumpy/utils.py](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS91dGlscy5weQ==) | `81.38% <100.00%> (-0.12%)` | :arrow_down: | | [src/plumpy/workchains.py](https://app.codecov.io/gh/aiidateam/plumpy/pull/272?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS93b3JrY2hhaW5zLnB5) | `94.35% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

sphuber commented 1 year ago

Thanks @chrisjsewell . If we can test this in an integrated way with aiida-core to show that it will speed up the daemon throughput when under heavy io load, this would be great. One open question is that this does break the interface, so I guess technically we should release this with 0.22, but that is already out since a while, but is not being used yet in aiida-core. There has been an open PR for it for a long time (https://github.com/aiidateam/aiida-core/pull/5732) but there is a small problem with the tests that I haven't been able to fix. Running normally seems ok, but have not dared merge it yet. We probably need to set aside some time with the team to test it thoroughly so we can merge it.

Then again, given that 0.22 has already been released, if we are going to bother with proper semver, I guess we should really not merge it in there either and release it with 0.23 instead 🤔

chrisjsewell commented 1 year ago

Yeh so I rebased it onto v0.21, just so I could get https://github.com/aiidateam/aiida-core/pull/6079 to work (without PR5732)

Here I mention that I haven't actually implemented any (new) asyncio behaviour for the local/ssh transports, as this may take some more thinking, for a "production ready" to ensure it doesn't create any new problems (e.g. limiting how many file transfers can be running at any one time)

I'm sure for ssh (or Firecrest) it should not be too difficult to show a toy example of a speed up. Obviously it is very dépendant on the type of calculations you are running though, I guess most suited if you are uploading/downloading many 1000s of files and/or very large files (and there is also different async strategies for both)

chrisjsewell commented 1 year ago

For sure getting PR5732 through would be nice 👍