Add terminal-based steering interface via an extract #1345

siramok · 2024-07-30T18:42:06Z

This PR adds a new extract which puts the user into a basic shell-like steering interface. The main benefit is to have a simpler, no-dependency alternative to the existing ascent-jupyter-bridge steering interface.

It can be accessed via:

  action: "add_extracts"
  extracts:
    e1:
      type: "steering"

The current implementation allows users to manually, at runtime:

Add, edit, or delete numeric/string parameters with which to run callbacks (via an internally managed conduit node).
List registered callbacks (via new APIs that expose the callback names).
Run callbacks by name.
See any resulting output from running a callback.

Still to-do:

Verify that all MPI usage is properly guarded by ASCENT_MPI_ENABLED.
Similarly, verify that things work properly without MPI.
Add tests.

Despite not being ready, the big picture implementation details are unlikely to change (where I've implemented it, how I've plumbed it through). Any feedback you may have for me is welcome.

cyrush

Look great!
a few minor suggestions related to using C++ refs for strings and vectors, as well as the recipe for grabbing the right MPI comm

cyrush · 2024-07-30T22:47:31Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.cpp

+    descriptions["run"] = "Run an Ascent callback with 'run my_callback_name'.";
+
+#ifdef ASCENT_MPI_ENABLED
+    MPI_Comm_rank(MPI_COMM_WORLD, &m_rank);


We might not be using COMM_WORLD, so let's grab the right communicator using:

MPI_Comm mpi_comm = MPI_Comm_f2c(flow::Workspace::default_mpi_comm());

cyrush · 2024-07-30T23:30:13Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.hpp

+    std::map<std::string, std::function<void()>> commands;
+    std::map<std::string, std::string> descriptions;
+    conduit::Node params;
+    conduit::Node output;


We like to use m_ prefix for member vars

cyrush · 2024-07-30T23:32:15Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.hpp

+
+    void empty_run();
+    void exit_shell();
+    void parse_input(std::string cmd, std::vector<std::string> args);


Suggested change

void parse_input(std::string cmd, std::vector<std::string> args);

void parse_input(std::string &cmd, std::vector<std::string> &args);

cyrush · 2024-07-30T23:32:23Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.hpp

+    void exit_shell();
+    void parse_input(std::string cmd, std::vector<std::string> args);
+    void list_callbacks();
+    void modify_params(std::vector<std::string> args);


Suggested change

void modify_params(std::vector<std::string> args);

void modify_params(std::vector<std::string> &args);

cyrush · 2024-07-30T23:32:31Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.hpp

+    void modify_params(std::vector<std::string> args);
+    void print_help();
+    void print_params();
+    void run_callback(std::vector<std::string> args);


Suggested change

void run_callback(std::vector<std::string> args);

void run_callback(std::vector<std::string> &args);

cyrush · 2024-07-30T23:33:31Z

src/libs/ascent/runtimes/flow_filters/ascent_runtime_steering_filters.cpp

+        }
+
+#ifdef ASCENT_MPI_ENABLED
+        MPI_Bcast(&input_size, 1, MPI_INT, 0, MPI_COMM_WORLD);


grab comm from workspace

cyrush · 2024-07-30T23:34:53Z

src/libs/ascent/ascent.hpp

@@ -87,6 +87,14 @@ void        ASCENT_API execute_callback(std::string callback_name,
 //-----------------------------------------------------------------------------
 bool        ASCENT_API execute_callback(std::string callback_name);

+//-----------------------------------------------------------------------------
+std::vector<std::string>


ref copy out style will be slight more efficient:

void ASCENT_API get_void_callbacks(std::vector<std::string> &name);

cyrush · 2024-07-30T23:35:22Z

src/libs/ascent/ascent.hpp

+
+//-----------------------------------------------------------------------------
+std::vector<std::string>
+            ASCENT_API get_bool_callbacks();


ref copy out style will be slight more efficient:

void ASCENT_API get_bool_callbacks(std::vector<std::string> &name);

cyrush · 2024-07-30T23:35:55Z

src/libs/ascent/ascent.hpp

@@ -87,6 +87,14 @@ void        ASCENT_API execute_callback(std::string callback_name,
 //-----------------------------------------------------------------------------
 bool        ASCENT_API execute_callback(std::string callback_name);


Suggested change

bool ASCENT_API execute_callback(std::string callback_name);

bool ASCENT_API execute_callback(const std::string &callback_name);

cyrush · 2024-07-30T23:36:19Z

src/libs/ascent/ascent.cpp

@@ -928,6 +928,28 @@ execute_callback(std::string callback_name)
    return callback_function();
 }

+//-----------------------------------------------------------------------------
+std::vector<std::string>


copy out still ref args are a bit more efficient (see below)

cyrush

thanks for the tweaks!

cyrush · 2024-08-02T18:37:42Z

Tests exercising steering look good - let me know when you feel its ready to merge.

siramok · 2024-08-02T19:25:32Z

Sweet, thanks Cyrus. I just made another polishing pass. Added comments, fixed formatting, simplified logic where I could, and made some tweaks after testing it out for myself. I feel comfortable merging it in it's current state.

I'll go create a new branch to work on an example instrumentation which makes use of this.

siramok added 3 commits July 30, 2024 09:27

Put some foundation in place

20abd1e

Cleanup output messages

4cd5fd0

Forgot to guard the mpi.h include

f94bda4

siramok requested a review from cyrush July 30, 2024 18:42

siramok marked this pull request as draft July 30, 2024 18:42

cyrush reviewed Jul 30, 2024

View reviewed changes

Address feedback

fe8cda3

cyrush approved these changes Jul 31, 2024

View reviewed changes

Add tests, formatting pass

bb61ae2

Polishing, improvements

11197e0

cyrush marked this pull request as ready for review August 2, 2024 21:10

cyrush force-pushed the develop branch from a0458f4 to fce0b2a Compare August 2, 2024 23:54

siramok force-pushed the task/terminal-steering branch from d77c26b to 11197e0 Compare August 3, 2024 00:14

Merge branch 'Alpine-DAV:develop' into task/terminal-steering

53823b5

cyrush changed the title ~~Draft: Add terminal-based steering interface via an extract~~ Add terminal-based steering interface via an extract Aug 3, 2024

cyrush merged commit 0eb44cd into Alpine-DAV:develop Aug 3, 2024
19 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add terminal-based steering interface via an extract #1345

Add terminal-based steering interface via an extract #1345

siramok commented Jul 30, 2024 •

edited

Loading

cyrush left a comment •

edited

Loading

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush Jul 30, 2024

cyrush left a comment

cyrush commented Aug 2, 2024

siramok commented Aug 2, 2024

	void parse_input(std::string cmd, std::vector<std::string> args);
	void parse_input(std::string &cmd, std::vector<std::string> &args);

	void modify_params(std::vector<std::string> args);
	void modify_params(std::vector<std::string> &args);

	void run_callback(std::vector<std::string> args);
	void run_callback(std::vector<std::string> &args);

		@@ -87,6 +87,14 @@ void ASCENT_API execute_callback(std::string callback_name,
		//-----------------------------------------------------------------------------
		bool ASCENT_API execute_callback(std::string callback_name);

	bool ASCENT_API execute_callback(std::string callback_name);
	bool ASCENT_API execute_callback(const std::string &callback_name);

Add terminal-based steering interface via an extract #1345

Add terminal-based steering interface via an extract #1345

Conversation

siramok commented Jul 30, 2024 • edited Loading

cyrush left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush left a comment

Choose a reason for hiding this comment

cyrush commented Aug 2, 2024

siramok commented Aug 2, 2024

siramok commented Jul 30, 2024 •

edited

Loading

cyrush left a comment •

edited

Loading