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

Jump to bottom

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
Draft

Add run_requested_timestamp state to delete the run_multiple_cells action #2542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b882ae
Add `run_requested_timestamp` state to delete the `run_multiple_cells…
fonsp Apr 26, 2023
55b004e
etc etc
fonsp 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
20 changes: 18 additions & 2 deletions frontend/components/Cell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,17 @@ const on_jump = (hasBarrier, pluto_actions, cell_id) => () => {
}
}

export const is_queued = (/** @type {import("./Editor.js").CellInputData} */ cell_input, /** @type {import("./Editor.js").CellResultData} */ cell_result) =>
cell_input.run_requested_timestamp > (cell_result.output.last_run_timestamp ?? 0) && !cell_result.running

export const is_running = (/** @type {import("./Editor.js").CellInputData} */ cell_input, /** @type {import("./Editor.js").CellResultData} */ cell_result) =>
cell_result.running

export const is_queued_or_running = (
/** @type {import("./Editor.js").CellInputData} */ cell_input,
/** @type {import("./Editor.js").CellResultData} */ cell_result
) => is_queued(cell_input, cell_result) || is_running(cell_input, cell_result)

/**
* @param {{
* cell_result: import("./Editor.js").CellResultData,
Expand All @@ -100,8 +111,8 @@ const on_jump = (hasBarrier, pluto_actions, cell_id) => () => {
* }} props
* */
export const Cell = ({
cell_input: { cell_id, code, code_folded, metadata },
cell_result: { queued, running, runtime, errored, output, logs, published_object_keys, depends_on_disabled_cells, depends_on_skipped_cells },
cell_input,
cell_result,
cell_dependencies,
cell_input_local,
notebook_id,
Expand All @@ -113,6 +124,11 @@ export const Cell = ({
nbpkg,
global_definition_locations,
}) => {
const { cell_id, code, code_folded, metadata, run_requested_timestamp } = cell_input
const { running, runtime, errored, output, logs, published_object_keys, depends_on_disabled_cells, depends_on_skipped_cells } = cell_result

const queued = is_queued(cell_input, cell_result)

const { show_logs, disabled: running_disabled, skip_as_script } = metadata
let pluto_actions = useContext(PlutoActionsContext)
// useCallback because pluto_actions.set_doc_query can change value when you go from viewing a static document to connecting (to binder)
Expand Down
2 changes: 1 addition & 1 deletion frontend/components/CellInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ const InputContextMenu = ({ on_delete, cell_id, run_cell, skip_as_script, runnin
const is_copy_output_supported = () => {
let notebook = /** @type{import("./Editor.js").NotebookData?} */ (pluto_actions.get_notebook())
let cell_result = notebook?.cell_results?.[cell_id]
return !!cell_result && !cell_result.errored && !cell_result.queued && cell_result.output.mime === "text/plain" && cell_result.output.body
return cell_result != null && !cell_result.errored && cell_result.output.mime === "text/plain" && cell_result.output.body
}

const copy_output = () => {
Expand Down
65 changes: 28 additions & 37 deletions frontend/components/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { setup_mathjax } from "../common/SetupMathJax.js"
import { slider_server_actions, nothing_actions } from "../common/SliderServerClient.js"
import { ProgressBar } from "./ProgressBar.js"
import { NonCellOutput } from "./NonCellOutput.js"
import { IsolatedCell } from "./Cell.js"
import { IsolatedCell, is_queued_or_running } from "./Cell.js"
import { RawHTMLContainer } from "./CellOutput.js"
import { RecordingPlaybackUI, RecordingUI } from "./RecordingUI.js"
import { HijackExternalLinksToOpenInNewTab } from "./HackySideStuff/HijackExternalLinksToOpenInNewTab.js"
Expand Down Expand Up @@ -122,6 +122,7 @@ const first_true_key = (obj) => {
* code: string,
* code_folded: boolean,
* metadata: CellMetaData,
* run_requested_timestamp: number,
* }}
*/

Expand Down Expand Up @@ -151,7 +152,6 @@ const first_true_key = (obj) => {
* @typedef CellResultData
* @type {{
* cell_id: string,
* queued: boolean,
* running: boolean,
* errored: boolean,
* runtime: number?,
Expand Down Expand Up @@ -301,7 +301,7 @@ export class Editor extends Component {

this.state = {
notebook: /** @type {NotebookData} */ initial_notebook_state,
cell_inputs_local: /** @type {{ [id: string]: CellInputData }} */ ({}),
cell_inputs_local: {},
desired_doc_query: null,
recently_deleted: /** @type {Array<{ index: number, cell: CellInputData }>} */ ([]),
recently_auto_disabled_cells: /** @type {Map<string,[string,string]>} */ ({}),
Expand Down Expand Up @@ -375,14 +375,10 @@ export class Editor extends Component {
},
add_deserialized_cells: async (data, index_or_id, deserializer = deserialize_cells) => {
let new_codes = deserializer(data)
/** @type {Array<CellInputData>} Create copies of the cells with fresh ids */
/** Create copies of the cells with fresh ids */
let new_cells = new_codes.map((code) => ({
cell_id: uuidv4(),
code: code,
code_folded: false,
metadata: {
...DEFAULT_CELL_METADATA,
},
}))

let index
Expand All @@ -406,7 +402,7 @@ export class Editor extends Component {
* (the usual flow is keyboard event -> cm -> local_code and not the opposite )
* See ** 1 **
*/
this.setState(
await this.setStatePromise(
immer((/** @type {EditorState} */ state) => {
// Deselect everything first, to clean things up
state.selected_cells = []
Expand All @@ -428,9 +424,11 @@ export class Editor extends Component {
...cell,
// Fill the cell with empty code remotely, so it doesn't run unsafe code
code: "",
code_folded: false,
metadata: {
...DEFAULT_CELL_METADATA,
},
run_requested_timestamp: 0.0,
}
}
notebook.cell_order = [
Expand Down Expand Up @@ -464,15 +462,16 @@ export class Editor extends Component {
const cells_to_add = parts.map((code) => {
return {
cell_id: uuidv4(),
code: code,
code: submit ? code : "",
code_folded: false,
metadata: {
...DEFAULT_CELL_METADATA,
},
run_requested_timestamp: submit ? Date.now() / 1000 : 0.0,
}
})

this.setState(
await this.setStatePromise(
immer((/** @type {EditorState} */ state) => {
for (let cell of cells_to_add) {
state.cell_inputs_local[cell.cell_id] = cell
Expand Down Expand Up @@ -527,10 +526,10 @@ export class Editor extends Component {
code,
code_folded: false,
metadata: { ...DEFAULT_CELL_METADATA },
run_requested_timestamp: Date.now() / 1000,
}
notebook.cell_order = [...notebook.cell_order.slice(0, index), id, ...notebook.cell_order.slice(index, Infinity)]
})
await this.client.send("run_multiple_cells", { cells: [id] }, { notebook_id: this.state.notebook.notebook_id })
return id
},
add_remote_cell: async (cell_id, before_or_after, code) => {
Expand All @@ -540,7 +539,7 @@ export class Editor extends Component {
},
confirm_delete_multiple: async (verb, cell_ids) => {
if (cell_ids.length <= 1 || confirm(`${verb} ${cell_ids.length} cells?`)) {
if (cell_ids.some((cell_id) => this.state.notebook.cell_results[cell_id].running || this.state.notebook.cell_results[cell_id].queued)) {
if (cell_ids.some((cell_id) => is_queued_or_running(this.state.notebook.cell_inputs[cell_id], this.state.notebook.cell_results[cell_id]))) {
if (confirm("This cell is still running - would you like to interrupt the notebook?")) {
this.actions.interrupt_remote(cell_ids[0])
}
Expand All @@ -560,7 +559,6 @@ export class Editor extends Component {
}
notebook.cell_order = notebook.cell_order.filter((cell_id) => !cell_ids.includes(cell_id))
})
await this.client.send("run_multiple_cells", { cells: [] }, { notebook_id: this.state.notebook.notebook_id })
}
}
},
Expand Down Expand Up @@ -588,31 +586,12 @@ export class Editor extends Component {
if (cell_ids.length > 0) {
await update_notebook((notebook) => {
for (let cell_id of cell_ids) {
if (this.state.cell_inputs_local[cell_id]) {
if (this.state.cell_inputs_local[cell_id] != null) {
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:

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)

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.

}
}
})
// This is a "dirty" trick, as this should actually be stored in some shared request_status => status state
// But for now... this is fine 😼
await this.setStatePromise(
immer((/** @type {EditorState} */ state) => {
for (let cell_id of cell_ids) {
if (state.notebook.cell_results[cell_id] != null) {
state.notebook.cell_results[cell_id].queued = this.is_process_ready()
} else {
// nothing
}
}
})
)
const result = await this.client.send("run_multiple_cells", { cells: cell_ids }, { notebook_id: this.state.notebook.notebook_id })
const { disabled_cells } = result.message
if (Object.entries(disabled_cells).length > 0) {
await this.setStatePromise({
recently_auto_disabled_cells: disabled_cells,
})
}
}
},
/**
Expand Down Expand Up @@ -783,6 +762,13 @@ patch: ${JSON.stringify(
})

break
case "run_feedback":
const { disabled_cells } = message
if (Object.entries(disabled_cells).length > 0) {
this.setStatePromise({
recently_auto_disabled_cells: disabled_cells,
})
}
default:
console.error("Received unknown update type!", update)
// alert("Something went wrong 🙈\n Try clearing your browser cache and refreshing the page")
Expand Down Expand Up @@ -930,13 +916,18 @@ patch: ${JSON.stringify(
// console.info("All scripts finished!")
this.send_queued_bond_changes()
})
this.any_cells_queued_or_running = () =>
this.state.notebook.cell_order.some((cell_id) =>
is_queued_or_running(this.state.notebook.cell_inputs[cell_id], this.state.notebook.cell_results[cell_id])
)

/** Is the notebook ready to execute code right now? (i.e. are no cells queued or running?) */
this.notebook_is_idle = () => {
return !(
this.waiting_for_bond_to_trigger_execution ||
this.pending_local_updates > 0 ||
// a cell is running:
Object.values(this.state.notebook.cell_results).some((cell) => cell.running || cell.queued) ||
this.any_cells_queued_or_running() ||
// a cell is initializing JS:
!_.isEmpty(this.js_init_set) ||
!this.is_process_ready()
Expand Down Expand Up @@ -1159,7 +1150,7 @@ patch: ${JSON.stringify(
// }
if (e.key.toLowerCase() === "q" && has_ctrl_or_cmd_pressed(e)) {
// This one can't be done as cmd+q on mac, because that closes chrome - Dral
if (Object.values(this.state.notebook.cell_results).some((c) => c.running || c.queued)) {
if (this.any_cells_queued_or_running()) {
this.actions.interrupt_remote()
}
e.preventDefault()
Expand Down
20 changes: 16 additions & 4 deletions frontend/components/Notebook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ const useMemoDebug = (fn, args) => {
}, args)
}

/**
* @param {{
* cell_result: import("./Editor.js").CellResultData,
* cell_input: import("./Editor.js").CellInputData,
* cell_input_local: { code: String },
* cell_dependencies: import("./Editor.js").CellDependencyData
* nbpkg: import("./Editor.js").NotebookPkgData?,
* selected: boolean,
* force_hide_input: boolean,
* focus_after_creation: boolean,
* [key: string]: any,
* }} props
* */
let CellMemo = ({
cell_result,
cell_input,
Expand All @@ -41,8 +54,8 @@ let CellMemo = ({
global_definition_locations,
}) => {
const { body, last_run_timestamp, mime, persist_js_state, rootassignee } = cell_result?.output || {}
const { queued, running, runtime, errored, depends_on_disabled_cells, logs, depends_on_skipped_cells } = cell_result || {}
const { cell_id, code, code_folded, metadata } = cell_input || {}
const { running, runtime, errored, depends_on_disabled_cells, logs, depends_on_skipped_cells } = cell_result || {}
const { cell_id, code, code_folded, metadata, run_requested_timestamp } = cell_input || {}
return useMemo(() => {
return html`
<${Cell}
Expand All @@ -65,9 +78,9 @@ let CellMemo = ({
cell_id,
...Object.keys(metadata),
...Object.values(metadata),
run_requested_timestamp,
depends_on_disabled_cells,
depends_on_skipped_cells,
queued,
running,
runtime,
errored,
Expand Down Expand Up @@ -154,7 +167,6 @@ export const Notebook = ({ notebook, cell_inputs_local, last_created_cell, selec
key=${cell_id}
cell_result=${notebook.cell_results[cell_id] ?? {
cell_id: cell_id,
queued: true,
running: false,
errored: false,
runtime: null,
Expand Down
16 changes: 9 additions & 7 deletions frontend/components/ProgressBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import _ from "../imports/lodash.js"
import { html, useContext, useEffect, useMemo, useState } from "../imports/Preact.js"
import { is_queued, is_queued_or_running, is_running } from "./Cell.js"
import { scroll_cell_into_view } from "./Scroller.js"

export const useDelayed = (value, delay = 500) => {
Expand Down Expand Up @@ -28,9 +29,7 @@ export const ProgressBar = ({ notebook, backend_launch_phase, status }) => {

useEffect(
() => {
const currently = Object.values(notebook.cell_results)
.filter((c) => c.running || c.queued)
.map((c) => c.cell_id)
const currently = notebook.cell_order.filter((c) => is_queued_or_running(notebook.cell_inputs[c], notebook.cell_results[c]))

set_currently_running(currently)

Expand All @@ -42,7 +41,7 @@ export const ProgressBar = ({ notebook, backend_launch_phase, status }) => {
set_recently_running(_.union(currently, recently_running))
}
},
Object.values(notebook.cell_results).map((c) => c.running || c.queued)
notebook.cell_order.map((c) => is_queued_or_running(notebook.cell_inputs[c], notebook.cell_results[c]))
)

let cell_progress = recently_running.length === 0 ? 0 : 1 - Math.max(0, currently_running.length - 0.3) / recently_running.length
Expand Down Expand Up @@ -74,9 +73,12 @@ export const ProgressBar = ({ notebook, backend_launch_phase, status }) => {
`}
onClick=${(e) => {
if (!binder_loading) {
const running_cell = Object.values(notebook.cell_results).find((c) => c.running) ?? Object.values(notebook.cell_results).find((c) => c.queued)
if (running_cell) {
scroll_cell_into_view(running_cell.cell_id)
const running_cell_id =
notebook.cell_order.find((c) => is_running(notebook.cell_inputs[c], notebook.cell_results[c])) ??
notebook.cell_order.find((c) => is_queued(notebook.cell_inputs[c], notebook.cell_results[c]))

if (running_cell_id != null) {
scroll_cell_into_view(running_cell_id)
}
}
}}
Expand Down
4 changes: 2 additions & 2 deletions src/analysis/Errors.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ function showerror(io::IO, mde::MultipleDefinitionsError)
end

"Send `error` to the frontend without backtrace. Runtime errors are handled by `WorkspaceManager.eval_format_fetch_in_workspace` - this function is for Reactivity errors."
function relay_reactivity_error!(cell::Cell, error::Exception)
function relay_reactivity_error!(cell::Cell, error::Exception, timestamp::Float64)
body, mime = PlutoRunner.format_output(CapturedException(error, []))
cell.output = CellOutput(
body=body,
mime=mime,
rootassignee=nothing,
last_run_timestamp=time(),
last_run_timestamp=timestamp,
persist_js_state=false,
)
cell.published_objects = Dict{String,Any}()
Expand Down
Loading