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

Implement workflow-level Play button business-logic #6427

Merged
merged 2 commits into from
Apr 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions app/gui/src/controller/graph/executed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,13 @@ impl Handle {
self.execution_ctx.set_execution_environment(execution_environment).await?;
Ok(())
}

/// Trigger a clean execution of the current graph with the "live" execution environment. That
/// means old computations and caches will be discarded.
pub async fn trigger_clean_live_execution(&self) -> FallibleResult {
self.execution_ctx.trigger_clean_live_execution().await?;
Ok(())
}
}


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

/// Trigger a clean execution of the current graph with the "live" execution environment. That
/// means old computations and caches will be discarded.
#[allow(clippy::needless_lifetimes)] // Note: Needless lifetimes
fn trigger_clean_live_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
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 trigger_clean_live_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
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 trigger_clean_live_execution(&self) -> BoxFuture<FallibleResult> {
async move {
self.language_server
.client
.recompute(
&self.id,
&language_server::InvalidatedExpressions::All,
&Some(ExecutionEnvironment::Live),
)
.await?;
Ok(())
}
.boxed_local()
}
}

impl Drop for ExecutionContext {
Expand Down
2 changes: 2 additions & 0 deletions app/gui/src/model/project.rs
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
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
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 trigger_clean_live_execution(&self) {
let graph_controller = self.graph_controller.clone_ref();
executor::global::spawn(async move {
if let Err(err) = graph_controller.trigger_clean_live_execution().await {
error!("Error starting clean live 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.trigger_clean_live_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
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
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
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