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

Adapt message arguments passing to process controller #6668

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Dec 16, 2024

Changes required after aiidateam/plumpy#301 and aiidateam/plumpy#291

For the rpc call of process.kill and process.pause, the passed in argument name is changed from msg to msg_text. Same for kill_processes and pause_processes of process controller.
Meanwhile, when calling _perform_actions the action is passed as a callable that takes no argument, it then requires the callable should be constructed with its arguments using functools.partial before hands. This increase the readability by makes the arguments stay together with the function itself where originally the arguments passed to callable is separated from other arguments for _perform_action itself.

@unkcpz unkcpz force-pushed the conform-with-new-plumpy-message-options branch from 0f5beb4 to 05b51e4 Compare December 16, 2024 14:33
@unkcpz unkcpz requested a review from agoscinski as a code owner December 16, 2024 14:33
@unkcpz unkcpz force-pushed the conform-with-new-plumpy-message-options branch from a55e16c to 8751167 Compare December 16, 2024 14:34
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

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

Project coverage is 77.99%. Comparing base (15b5caf) to head (35a0204).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/engine/processes/functions.py 50.00% 1 Missing ⚠️
src/aiida/engine/runners.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6668      +/-   ##
==========================================
+ Coverage   77.99%   77.99%   +0.01%     
==========================================
  Files         563      563              
  Lines       41761    41762       +1     
==========================================
+ Hits        32567    32570       +3     
+ Misses       9194     9192       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

Thanks a lot @unkcpz , I have just one comment.

src/aiida/engine/processes/control.py Show resolved Hide resolved
Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

@unkcpz thanks a lot! changes makes sense.
As agreed let's wait for a plumpy release before merging this. and we have to remember to remove the commit hook for plumpy so to pin the to-be-released version.

Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Looks okay, just needs to be rebased after plumpy release

src/aiida/engine/processes/control.py Outdated Show resolved Hide resolved
src/aiida/engine/processes/control.py Outdated Show resolved Hide resolved
@unkcpz unkcpz force-pushed the conform-with-new-plumpy-message-options branch from 87d7c51 to 3fd268a Compare January 8, 2025 15:26
@unkcpz
Copy link
Member Author

unkcpz commented Jan 8, 2025

@khsrali I rebase with the main branch of plumpy, all CI code tests are passed. We can make a release for plumpy.

@unkcpz unkcpz requested review from khsrali and agoscinski January 8, 2025 22:08
@unkcpz
Copy link
Member Author

unkcpz commented Jan 10, 2025

Hi @khsrali, I update plumpy version, should all good now. Can you have another look?

@unkcpz
Copy link
Member Author

unkcpz commented Jan 10, 2025

I'll merge this now, it blocks my work on #6675. The changes here are mostly variable names of function call.
In #6675, there are other engine changes that are hard to debug (I was struggle on things are working on python3.12 but not python3.9) I want to isolate the changes from this PR.

@unkcpz unkcpz requested a review from sphuber January 10, 2025 22:02
@unkcpz unkcpz force-pushed the conform-with-new-plumpy-message-options branch 4 times, most recently from f011e90 to 642f974 Compare January 11, 2025 17:50
@unkcpz unkcpz force-pushed the conform-with-new-plumpy-message-options branch from 642f974 to 35a0204 Compare January 11, 2025 17:56
Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

Thanks @unkcpz

@unkcpz unkcpz merged commit 80c1741 into aiidateam:main Jan 13, 2025
23 checks passed
@unkcpz unkcpz deleted the conform-with-new-plumpy-message-options branch January 13, 2025 15:46
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