Implement workflow-level Play button business-logic #6427
Conversation
MichaelMauderer
added
the
CI: No changelog needed
MichaelMauderer
requested review from
wdanilo,
farmaazon,
kazcw and
mwu-tow
as code owners
|
|
||
| /// Trigger the execution of the current graph. | ||
| pub async fn start_execution(&self) -> FallibleResult { | ||
| self.execution_ctx.start_execution().await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I clarified the naming and comments. The clean re-execution should be handled by the API call requesting language_server::InvalidatedExpressions::All.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
We should also turn on "read-only" flag on controllers, implemented by @vitvakatu AFAIK.
There was a problem hiding this comment.
That should already happen through the double click, which emits the appropriate FRP event from the graph editor.
| Event::Notification(Notification::ExecutionComplete { context_id }) => { | ||
| execution_update_handler(context_id, ExecutionUpdate::Completed); | ||
| publisher.notify(model::project::Notification::ExecutionFinished); |
There was a problem hiding this comment.
If this notification is sent also when any execution finishes (also those performed automatically after a change), then we could have a race where:
- User did a change, the automatic execution starts
- User clicks the play button. We set read-only flag to true.
- We receive "ExecutionComplete" event about execution launched in 1. We turn off "read-only" flag.
- Only after some time we receive "ExecutionComplete" event about execution launched in 2. Here we should turn off the flag instead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@4e6 could you help us with that? Will the engine stop the current execution when receiving a recompute message?
There was a problem hiding this comment.
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.
QA: passed 🟢 |
MichaelMauderer
added
the
CI: Ready to merge
Pull Request Description
Implements #6179
Peek.2023-04-26.00-30.mp4
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.