-
Notifications
You must be signed in to change notification settings - Fork 66
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
WIP: Shell commands and function callbacks #1175
WIP: Shell commands and function callbacks #1175
Conversation
Awesome, thank you for creating this is a nice foundation! Here are some ideas to discuss how we can evolve: How about "commands" as an abstraction for either a shell command or a callback? For example:
This approach would also work if we want to use either shell cmds/callbacks as trigger inputs in the future, for example:
I agree callback should be of the form |
//----------------------------------------------------------------------------- | ||
|
||
#include <mpi.h> | ||
|
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.
to heal CI issues, need to guard including mpi with:
#ifdef ASCENT_MPI_ENABLED
...
#endif
For the callback table: We can move callback table into the Callback filter (or a combined You should be able to move the static table instance and methods right over from Workspace to the Callback (or Command) class, and wire up their use in the main ascent runtime. Let me know how this sounds and if you have questions. |
Thanks for the comments, I like the idea of combining them into a single filter. I'll make some progress this week and ping you here when it's ready to be looked at again. |
Sounds good! |
Ok, the PR is ready for re-review! I'm unsure why curl is failing to download conduit in the CI, but that's unrelated to my changes. Summary:
I haven't yet done the work to use these directly as trigger conditions, but think that could be done as a follow-up PR. Multi-line shell command example: -
action: "add_commands"
commands:
c1:
params:
shell_command: "echo A single-line shell command"
c2:
params:
shell_command: >
echo A single shell command
defined on multiple lines
c3:
params:
shell_command: |
echo The first of a multi-line shell command
echo The second of a multi-line shell command
c4:
params:
shell_command: "echo The first of a \
multi-line shell command\necho The \
second of a multi-line shell command" Multi-line callback example: -
action: "add_commands"
commands:
c1:
params:
callback: "test_func1"
c2:
params:
callback: |
test_func1
test_func2 Mixed example, with mpi behavior controls: -
action: "add_commands"
commands:
c1:
params:
shell_command: |
echo This is the first shell command
echo This is the second shell command
mpi_behavior: "all"
c2:
params:
callback: |
test_func1
test_func2
mpi_behavior: "root" |
Does it make sense to also add parameters to the callbacks? |
After trying to implement some use cases, I realize that callbacks should probably be allowed to return either void or bool (as opposed to only allowing one or the other). Void for cases where you want to perform an operation (like checkpointing) and bool for cases where you want to compute something and use the result in a trigger condition. It feels a bit odd to add What do you think? |
Sounds good, we would just need to support the two signatures and have two maps, and logic to make sure registered name is a unique name across both. |
@mvictoras for passing params to callbacks: We could have a callback that passes a conduit node as a generic mechanism -- but the question of how to select what we put in that Node from Ascent isn't clear. Another option - if they want info from ascent, we could pass the ascent instance in the callback. That use case could still be tricky b/c the info details won't be fully complete until all of the current actions are executed. People could also do bad things if they try to execute new actions in the middle of current execution. |
Everything looks good. We should add unit tests that exercise the system command and call back cases. I did have an idea to simplify overall: Registered callbacks are effectively static and shared across ascent instances. Should we just make this part of the main interface as static methods? That would cut out the plumbing and the extra logic for other runtime. It also helps with the jupyter use cases b/c you don't need an ascent instance, it could be a static method as well (like If we do this, it's more rework (I am happy take it on), but i wonder if what folks think. @nicolemarsaglia ideas? |
Moving callback registration/execution into the main interface as static methods does significantly simplify the PR, but I had trouble exposing For testing, I committed a bunch of unit tests for Finally, I'll work on creating tests that actually execute some simple shell commands and callbacks. I'll also be sure to create (at least) one that demonstrates using a bool callback as a trigger condition. |
@siramok thanks for the updates! The basic strategy for testing callbacks is close to what you have. Use a global var that gets changed when the call back is exec-ed, and then assert we have the changed value. Also, I am happy to take over the refactoring and get the final steps done, just let me know when you are done with your updates. |
I couldn't help myself and attempted the refactor again. I had better luck this time around and found the issue that I was having with the Python/Jupyter interface, so I committed the work! The Callback invocation from Jupyter will now require importing ascent explicitly, which is perfectly fine: import ascent
import conduit
ascent.execute_callback("myCallback", conduit.Node(), conduit.Node()) In addition to getting that refactor done, I added tests for executing callbacks, one for executing a shell command, and one that uses a bool callback as a trigger condition. The tests now reflect the API changes as well. |
I gave the changes another look and found a few minor things that needed to be cleaned up (basically just imports made unnecessary by the refactor). Besides that, I feel that everything is good to go. The only notably missing piece I can think of is a Python test for Finally, I see that CI is failing again due to a third party dependency issue. We'll obviously need to get CI passing again, but in the meantime you're good to give it another review @cyrush. :) Edit: worth considering, I overloaded |
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.
Awesome, the outer level interface was exactly what I was hoping for, nice refactoring!
I changed the target of the branch from develop to a branch in the ascent repo that we can both work on.
I have some minor suggestions but I can tackle those.
I will also explore how we can register python callbacks ...
@@ -66,13 +66,31 @@ class ASCENT_API Ascent | |||
conduit::Node m_info; | |||
}; | |||
|
|||
// Callback maps | |||
static std::map<std::string, void (*)(conduit::Node &, conduit::Node &)> m_void_callback_map; | |||
static std::map<std::string, bool (*)(void)> m_bool_callback_map; |
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.
we should define these in the cpp file, otherwise they could be multiple defined by users calling ascent.
PyObject *py_output = NULL; | ||
|
||
if (!PyArg_ParseTuple(args, | ||
"sOO", |
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.
we can use s|00
to make all the args optional (maybe to support the bool callback case)
PyObject *py_output = NULL; | ||
|
||
if (!PyArg_ParseTuple(args, | ||
"sOO", |
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.
also s|OO
possible here
ascent.open(ascent_opts); | ||
|
||
// An error should be thrown due to not including a name | ||
ascent::register_callback("", void_callback_1); |
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.
we can wrap this with a google test, EXPECT_THROW
to make sure an error is thrown
ascent::register_callback("", void_callback_1); | |
EXPECT_THROW(ascent::register_callback("", void_callback_1),conduit::Error); |
Node actions; | ||
std::string msg = "An example of registering a void callback"; | ||
" without a callback name."; | ||
ASCENT_ACTIONS_DUMP(actions, std::string("register_no_name_void"), msg); |
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.
FYI: we don't need to use this marco here b/c we aren't dumping any actions.
We use this to help seed our docs.
@siramok FYI: I invited you to join the ascent repo as a contributor, keep an eye out for github request. |
e741d17
into
Alpine-DAV:task/2023_08_siramok_command_integration
* WIP: Shell commands and function callbacks (#1175) Work from @siramok. * Initial callback and shell command implementations * Initial work to combine commands and callbacks * Move callback registration and execution into Command * Catch the case where neither action is defined * Allow defining multiline shell commands and callbacks * Minor formatting tweaks * Initial implementation of triggers with callbacks * Improved organization, more error catching * Make sure that triggers either have a condition or callback * Make void callbacks take conduit node parameters * Initial attempt at exposing callbacks through ascent-jupyter-bridge * Let void callbacks return arbitrary data via conduit nodes * Disallow anonymous callbacks, add some tests * Add tests for shell commands and callbacks * Refactor callback API into the main API, adjusts tests to reflect the change * Minor cleanups that I missed * Fix minor bug with the trigger + callback test * alt download link for zlib * typo * Fix several bugs in the tests * Implement all suggestions --------- Co-authored-by: Andres Sewell <30913496+siramok@users.noreply.github.com> Co-authored-by: Andres Sewell <sewellandres@gmail.com>
Adds the following new capabilities to ascent:
Example shell command usage (subject to change):
Example callback usage (subject to change):
Still to do, we discussed:
command:
.