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

Add run_requested_timestamp state to delete the run_multiple_cells action #2542

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fonsp
Copy link
Owner

@fonsp fonsp commented Apr 26, 2023

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:

  1. Frontend changes cell_input[cell_id].code in the frontend state, and computes patches from that change.
  2. Send those patches to the backend
  3. Backend sends confirmation back to frontend
  4. Frontend sends a run_multiple_cells request to backend
  5. Backend runs cells async

After this PR, the cell_input struct will have one new field: run_requested_timestamp. And the cell_result already has a field cell_result.output.last_run_timestamp. So now, we can say:

  • A cell should run if run_requested_timestamp > last_run_timestamp. So the frontend doesn't need to explicitly request a run anymore.
  • When a cell completes, set last_run_timestamp = run_requested_timestamp.
  • A cell is "queued" if run_requested_timestamp > last_run_timestamp && !running. So we can remove the queued field from our state and make it a computed field.

So after this PR, the process is:

  1. Frontend changes cell_input[cell_id].code and cell_input[cell_id].run_requested_timestamp = Date.now() / 1000 in the frontend state, and computes patches from that change.
  2. Send those patches to the backend
  3. Backend applies those patches locally, and notices that a run_requested_timestamp changed. This patch is matched by our wildcard as a side effect.
  4. Backend runs cells async
  5. Backend sends confirmation back to frontend

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

  • handle time differences between clients

search for:

  • relay_reactivity_error!
  • last_run_timestamp
  • CellOutput
  • run_feedback
  • run_multiple_cells
  • is_process_ready
  • disabled_cells

test for

@github-actions
Copy link
Contributor

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

@fonsp fonsp added frontend Concerning the HTML editor backend Concerning the julia server and runtime reactivity The Pluto programming paradigm HTTP/WS The connection between backend and frontend labels Apr 26, 2023
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
Copy link
Collaborator

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:

"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:

cell.last_run_version = length(cell.cm_updates)

Copy link
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Concerning the julia server and runtime frontend Concerning the HTML editor HTTP/WS The connection between backend and frontend reactivity The Pluto programming paradigm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants