aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
411 stars 184 forks source link

Engine: recover behaviour of `upload_calculation` #6447

Closed mbercx closed 3 weeks ago

mbercx commented 3 weeks ago

This is a replacement PR for https://github.com/aiidateam/aiida-core/pull/6348 that contains a set of commits to recover the behaviour of upload_calculation the latest release (v2.5.1).

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.86%. Comparing base (ef60b66) to head (c49b963). Report is 24 commits behind head on main.

Files Patch % Lines
src/aiida/engine/daemon/execmanager.py 90.48% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6447 +/- ## ========================================== + Coverage 77.51% 77.86% +0.36% ========================================== Files 560 562 +2 Lines 41444 41794 +350 ========================================== + Hits 32120 32539 +419 + Misses 9324 9255 -69 ``` | [Flag](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6447/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [presto](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6447/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | `73.19% <92.60%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mbercx commented 3 weeks ago

@sphuber I could still reorder or merge the commits a bit, but I tried to separate them in order to keep them more atomic and clear.

I forgot to add you as a co-author for the engine fix commit, apologies! Will fix that during review.

sphuber commented 3 weeks ago

@sphuber I could still reorder or merge the commits a bit, but I tried to separate them in order to keep them more atomic and clear.

Think they look fine (minus the emojis of course ;) ). I would maybe just merge the second and third, since they are both dealing with centralizing the serialize_file_hierarchy fixture. Think it is fine and perhaps better even to have them in a single commit.

I forgot to add you as a co-author for the engine fix commit, apologies! Will fix that during review.

S'all good man

mbercx commented 3 weeks ago

Think they look fine (minus the emojis of course ;) )

Oops, did I add emojis? I didn't even notice 😇

I would maybe just merge the second and third, since they are both dealing with centralizing the serialize_file_hierarchy fixture. Think it is fine and perhaps better even to have them in a single commit.

Makes sense, will do! Do I go ahead and do it now or wait for your first round of review?

sphuber commented 3 weeks ago

Makes sense, will do! Do I go ahead and do it now or wait for your first round of review?

Going through it now. Might as well hold on a bit

mbercx commented 3 weeks ago

@sphuber hierarchy commits squashed and nit picked! Let's I didn't mess up and the tests still pass ;)

sphuber commented 3 weeks ago

Fan-fucking-tastic @mbercx thanks a lot