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.

Sign up for GitHub

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

Implement workflow-level Play button business-logic #6427

Merged
merged 2 commits into from
Apr 27, 2023
+59 −1
Merged

Implement workflow-level Play button business-logic #6427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a75bf9d
Implement workflow-level Play button business-logic
MichaelMauderer Apr 25, 2023
e5a5b63
Clarify execution intention.
MichaelMauderer Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/gui/src/controller/graph/executed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,12 @@ impl Handle {
self.execution_ctx.set_execution_environment(execution_environment).await?;
Ok(())
}

/// Trigger the execution of the current graph.
pub async fn start_execution(&self) -> FallibleResult {
self.execution_ctx.start_execution().await?;
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add somewhere more information about this method. It's not just starting execution (as the execution is actually performed automatically on each change). According to the design. It ought to clear caches and re-execute the entire graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this handled by it using the live execution context? If not, we do need some more special handling here. I’ll have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clarified the naming and comments. The clean re-execution should be handled by the API call requesting language_server::InvalidatedExpressions::All.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call it something like recompute_in_live_environment to align with the underlying LS call. AFAIK this is not handled by the live environment specifically but rather by the usage of recompute with specific arguments (InvalidatedExpressions::All)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose trigger_clean_live_execution now, but we can do recompute_in_live_environment if that is preferable. I don't have a string opinion either way.

}


Expand Down
4 changes: 4 additions & 0 deletions app/gui/src/model/execution_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,10 @@ pub trait API: Debug {
&'a self,
execution_environment: ExecutionEnvironment,
) -> BoxFuture<'a, FallibleResult>;

/// Start the execution of the context.
#[allow(clippy::needless_lifetimes)] // Note: Needless lifetimes
fn start_execution<'a>(&'a self) -> BoxFuture<'a, FallibleResult>;
}

// Note: Needless lifetimes
Expand Down
4 changes: 4 additions & 0 deletions app/gui/src/model/execution_context/plain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,10 @@ impl model::execution_context::API for ExecutionContext {
self.execution_environment.set(environment);
futures::future::ready(Ok(())).boxed_local()
}

fn start_execution(&self) -> LocalBoxFuture<FallibleResult> {
futures::future::ready(Ok(())).boxed_local()
}
}


Expand Down
15 changes: 15 additions & 0 deletions app/gui/src/model/execution_context/synchronized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,21 @@ impl model::execution_context::API for ExecutionContext {
}
.boxed_local()
}

fn start_execution(&self) -> BoxFuture<FallibleResult> {
async move {
self.language_server
.client
.recompute(
&self.id,
&language_server::InvalidatedExpressions::All,
&Some(ExecutionEnvironment::Live),
)
.await?;
Ok(())
}
.boxed_local()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also turn on "read-only" flag on controllers, implemented by @vitvakatu AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should already happen through the double click, which emits the appropriate FRP event from the graph editor.

}
}

impl Drop for ExecutionContext {
Expand Down
2 changes: 2 additions & 0 deletions app/gui/src/model/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ pub enum Notification {
ConnectionLost(BackendConnection),
/// Indicates that the project VCS status has changed.
VcsStatusChanged(VcsStatus),
/// Indicates that the project has finished execution.
ExecutionFinished,
}

/// Denotes one of backend connections used by a project.
Expand Down
1 change: 1 addition & 0 deletions app/gui/src/model/project/synchronized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ impl Project {
Event::Notification(Notification::ExecutionStatus(_)) => {}
Event::Notification(Notification::ExecutionComplete { context_id }) => {
execution_update_handler(context_id, ExecutionUpdate::Completed);
publisher.notify(model::project::Notification::ExecutionFinished);
Comment on lines 545 to +547
Copy link
Contributor

Choose a reason for hiding this comment

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

If this notification is sent also when any execution finishes (also those performed automatically after a change), then we could have a race where:

  1. User did a change, the automatic execution starts
  2. User clicks the play button. We set read-only flag to true.
  3. We receive "ExecutionComplete" event about execution launched in 1. We turn off "read-only" flag.
  4. Only after some time we receive "ExecutionComplete" event about execution launched in 2. Here we should turn off the flag instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. But is the old execution not aborted when we request a new one? It could still happen in a very small time window, though. But I don't think we currently have a way to match a request for re-computation with its status update, unless I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@4e6 could you help us with that? Will the engine stop the current execution when receiving a recompute message?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when the recompute message is received, the engine aborts the current execution and re-schedules a new one. In general, the engine follows the principle that if it needs to schedule a new execution, it makes sure that the other execution is stopped. Otherwise, it may accumulate those commands and re-execute the program in a sequence.

}
Event::Notification(Notification::ExpressionValuesComputed(_)) => {
// the notification is superseded by `ExpressionUpdates`.
Expand Down
18 changes: 18 additions & 0 deletions app/gui/src/presenter/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ impl Model {
self.view.graph().model.breadcrumbs.set_project_changed(changed);
}

fn execution_finished(&self) {
self.view.graph().frp.set_read_only(false);
self.view.graph().frp.execution_finished.emit(());
}

fn execution_context_interrupt(&self) {
let controller = self.graph_controller.clone_ref();
executor::global::spawn(async move {
Expand Down Expand Up @@ -306,6 +311,15 @@ impl Model {
error!("Invalid execution environment: {execution_environment:?}");
}
}

fn activate_execution(&self) {
let graph_controller = self.graph_controller.clone_ref();
executor::global::spawn(async move {
if let Err(err) = graph_controller.start_execution().await {
error!("Error starting execution: {err}");
}
});
}
}


Expand Down Expand Up @@ -405,6 +419,7 @@ impl Project {

view.set_read_only <+ view.toggle_read_only.map(f_!(model.toggle_read_only()));
eval graph_view.execution_environment((env) model.execution_environment_changed(env));
eval_ graph_view.execution_environment_play_button_pressed( model.activate_execution());
}

let graph_controller = self.model.graph_controller.clone_ref();
Expand Down Expand Up @@ -461,6 +476,9 @@ impl Project {
Notification::VcsStatusChanged(VcsStatus::Clean) => {
model.set_project_changed(false);
}
Notification::ExecutionFinished => {
model.execution_finished();
}
};
std::future::ready(())
});
Expand Down
4 changes: 3 additions & 1 deletion app/gui/view/execution-environment-selector/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,12 @@ ensogl::define_endpoints_2! {
Input {
set_available_execution_environments (ExecutionEnvironments),
set_execution_environment (ExecutionEnvironment),
reset_play_button_state (),
}
Output {
selected_execution_environment (ExecutionEnvironment),
play_press(),
size (Vector2),
size(Vector2),
}
}

Expand Down Expand Up @@ -268,6 +269,7 @@ impl component::Frp<Model> for Frp {
model.set_play_button_visibility(play_button_visibility);
});
play_button.reset <+ selected_entry.constant(());
play_button.reset <+ input.reset_play_button_state;

// == Outputs ==

Expand Down
3 changes: 3 additions & 0 deletions app/gui/view/graph-editor/src/execution_environment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ pub fn init_frp(frp: &Frp, model: &GraphEditorModelWithNetwork) {
<- any(selector.selected_execution_environment,external_update);
out.execution_environment <+ execution_environment_update;
out.execution_environment_play_button_pressed <+ selector.play_press;
frp.set_read_only <+ selector.play_press.constant(true);

// === Play Button ===
selector.reset_play_button_state <+ frp.execution_finished;

// === Layout ===

Expand Down
1 change: 1 addition & 0 deletions app/gui/view/graph-editor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,7 @@ ensogl::define_endpoints_2! {
/// Set the execution environmenta available to the graph.
set_available_execution_environments (Rc<Vec<execution_environment_selector::ExecutionEnvironment>>),
set_execution_environment (ExecutionEnvironment),
execution_finished(),


// === Debug ===
Expand Down