-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
Add run_requested_timestamp
state to delete the run_multiple_cells
action
#2542
base: main
Are you sure you want to change the base?
Conversation
Try this Pull Request!Open Julia and type: julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/fonsp/Pluto.jl", rev="remove-run_multiple_cells-action")
julia> using Pluto |
notebook.cell_inputs[cell_id].code = this.state.cell_inputs_local[cell_id].code | ||
notebook.cell_inputs[cell_id].run_requested_timestamp = Date.now() / 1000 |
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.
Maybe a dumb concern, but what if multiple clients are in different timezones/have different browser times?
In #2296, the cell code at time T
can be represented by an integer (the number of updates applied since the start) which I planned to use for the "draft" state by comparing with the editor's current version:
Pluto.jl/src/webserver/Dynamic.jl
Line 116 in fee51f7
"last_run_version" => cell.last_run_version, |
We could add last_run_version::Int
and run_requested_version::Int
in this PR? run_multiple_cells
becomes run_requested_version = last_run_version + 1
without the Operational transforms updates currently (not sure about conflicts). With operation transform it is updated to be the latest synced version:
Pluto.jl/src/webserver/Dynamic.jl
Line 497 in fee51f7
cell.last_run_version = length(cell.cm_updates) |
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.
Oh doing this with OT would be super nice!!
Right now there is a visual glitch if you collaborate, and first I run a cell, and then you run the same cell, but your clock is 60 seconds behind mine, then your "requested timestamp" will actually be less than the "last run timestamp". It will still run, but not display as "queued" in the frontend.
This is an internal change to the way executions are triggered! Rather than the frontend requesting cell execution, cells have a timestamp in their state "when the user wanted the cell to be executed", which can be compared to "when we last executed the cell". This means that execution will now also be handled by our super cool state system, instead of a separate API call.
This PR should not change any behaviour, but it will help with future features, including #220 #1694
More detail:
Right now the frontend makes a request to the backend (
run_multiple_cells
) to run cells. So when you Shift+enter, we currently do this:cell_input[cell_id].code
in the frontend state, and computes patches from that change.run_multiple_cells
request to backendAfter this PR, the
cell_input
struct will have one new field:run_requested_timestamp
. And thecell_result
already has a fieldcell_result.output.last_run_timestamp
. So now, we can say:run_requested_timestamp > last_run_timestamp
. So the frontend doesn't need to explicitly request a run anymore.last_run_timestamp = run_requested_timestamp
.run_requested_timestamp > last_run_timestamp && !running
. So we can remove thequeued
field from our state and make it a computed field.So after this PR, the process is:
cell_input[cell_id].code
andcell_input[cell_id].run_requested_timestamp = Date.now() / 1000
in the frontend state, and computes patches from that change.run_requested_timestamp
changed. This patch is matched by our wildcard as a side effect.This change is mostly internal. Right now I want to mimick the old behaviour with respect to #220, but after this PR, it will be easier to control this behaviour. Instead of having a queue of executions, we could see this as: "this is the set of cells that still needs to be executed". Then we can improve #220, and we could allow you to add new cells while older cells are still queued, and execute that new cell faster, before waiting for all cells to be done.
This also means that Shift+Enter will be slightly faster! Because it avoids the second round trip/
TODO
search for:
test for