-
Notifications
You must be signed in to change notification settings - Fork 233
Adapt message arguments passing to process controller #6668
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
Conversation
0f5beb4 to
05b51e4
Compare
a55e16c to
8751167
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
khsrali
left a comment
There was a problem hiding this 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.
khsrali
left a comment
There was a problem hiding this 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.
agoscinski
left a comment
There was a problem hiding this 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
87d7c51 to
3fd268a
Compare
|
@khsrali I rebase with the main branch of plumpy, all CI code tests are passed. We can make a release for plumpy. |
|
Hi @khsrali, I update plumpy version, should all good now. Can you have another look? |
f011e90 to
642f974
Compare
642f974 to
35a0204
Compare
khsrali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @unkcpz
Changes required after aiidateam/plumpy#301 and aiidateam/plumpy#291
For the rpc call of
process.killandprocess.pause, the passed in argument name is changed frommsgtomsg_text. Same forkill_processesandpause_processesof process controller.Meanwhile, when calling
_perform_actionsthe action is passed as a callable that takes no argument, it then requires the callable should be constructed with its arguments usingfunctools.partialbefore 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_actionitself.