From 597d9fff3fb7e690a2f74500c71141183ee2870c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 3 Feb 2023 21:04:13 +0100 Subject: [PATCH] Cancel scheduled evaluation on error (#1688) --- assets/js/hooks/session.js | 1 + lib/livebook/live_markdown/export.ex | 14 +-- lib/livebook/live_markdown/import.ex | 3 + lib/livebook/notebook/cell/code.ex | 15 ++- lib/livebook/runtime.ex | 1 + lib/livebook/runtime/evaluator.ex | 1 + lib/livebook/session/data.ex | 83 +++++++++++--- lib/livebook_web/live/session_live.ex | 36 +++--- .../live/session_live/cell_component.ex | 6 +- .../code_cell_settings_component.ex | 15 ++- .../live/session_live/indicators_component.ex | 15 +++ static/favicon-errored.svg | 87 +++++++++++++++ test/livebook/live_markdown/export_test.exs | 3 +- test/livebook/live_markdown/import_test.exs | 3 +- test/livebook/session/data_test.exs | 104 +++++++++++++++++- test/livebook/session_test.exs | 7 +- 16 files changed, 337 insertions(+), 57 deletions(-) create mode 100644 static/favicon-errored.svg diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index 4f5bbc607..e6c66e8c7 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -280,6 +280,7 @@ const Session = { faviconForEvaluationStatus(evaluationStatus) { if (evaluationStatus === "evaluating") return "favicon-evaluating"; if (evaluationStatus === "stale") return "favicon-stale"; + if (evaluationStatus === "errored") return "favicon-errored"; return "favicon"; }, diff --git a/lib/livebook/live_markdown/export.ex b/lib/livebook/live_markdown/export.ex index 051207546..06acbec76 100644 --- a/lib/livebook/live_markdown/export.ex +++ b/lib/livebook/live_markdown/export.ex @@ -68,11 +68,8 @@ defmodule Livebook.LiveMarkdown.Export do end defp notebook_metadata(notebook) do - put_unless_default( - %{}, - Map.take(notebook, [:persist_outputs, :autosave_interval_s]), - Map.take(Notebook.new(), [:persist_outputs, :autosave_interval_s]) - ) + keys = [:persist_outputs, :autosave_interval_s] + put_unless_default(%{}, Map.take(notebook, keys), Map.take(Notebook.new(), keys)) end defp render_section(section, notebook, ctx) do @@ -148,11 +145,8 @@ defmodule Livebook.LiveMarkdown.Export do end defp cell_metadata(%Cell.Code{} = cell) do - put_unless_default( - %{}, - Map.take(cell, [:disable_formatting, :reevaluate_automatically]), - Map.take(Cell.Code.new(), [:disable_formatting, :reevaluate_automatically]) - ) + keys = [:disable_formatting, :reevaluate_automatically, :continue_on_error] + put_unless_default(%{}, Map.take(cell, keys), Map.take(Cell.Code.new(), keys)) end defp cell_metadata(_cell), do: %{} diff --git a/lib/livebook/live_markdown/import.ex b/lib/livebook/live_markdown/import.ex index 2cc9120b1..0ab7843b5 100644 --- a/lib/livebook/live_markdown/import.ex +++ b/lib/livebook/live_markdown/import.ex @@ -393,6 +393,9 @@ defmodule Livebook.LiveMarkdown.Import do {"reevaluate_automatically", reevaluate_automatically}, attrs -> Map.put(attrs, :reevaluate_automatically, reevaluate_automatically) + {"continue_on_error", continue_on_error}, attrs -> + Map.put(attrs, :continue_on_error, continue_on_error) + _entry, attrs -> attrs end) diff --git a/lib/livebook/notebook/cell/code.ex b/lib/livebook/notebook/cell/code.ex index 238841158..33f763f61 100644 --- a/lib/livebook/notebook/cell/code.ex +++ b/lib/livebook/notebook/cell/code.ex @@ -6,7 +6,14 @@ defmodule Livebook.Notebook.Cell.Code do # It consists of text content that the user can edit # and produces some output once evaluated. - defstruct [:id, :source, :outputs, :disable_formatting, :reevaluate_automatically] + defstruct [ + :id, + :source, + :outputs, + :disable_formatting, + :reevaluate_automatically, + :continue_on_error + ] alias Livebook.Utils alias Livebook.Notebook.Cell @@ -16,7 +23,8 @@ defmodule Livebook.Notebook.Cell.Code do source: String.t() | :__pruned__, outputs: list(Cell.indexed_output()), disable_formatting: boolean(), - reevaluate_automatically: boolean() + reevaluate_automatically: boolean(), + continue_on_error: boolean() } @doc """ @@ -29,7 +37,8 @@ defmodule Livebook.Notebook.Cell.Code do source: "", outputs: [], disable_formatting: false, - reevaluate_automatically: false + reevaluate_automatically: false, + continue_on_error: false } end end diff --git a/lib/livebook/runtime.ex b/lib/livebook/runtime.ex index 85cb5f76c..f1ef9f052 100644 --- a/lib/livebook/runtime.ex +++ b/lib/livebook/runtime.ex @@ -78,6 +78,7 @@ defprotocol Livebook.Runtime do dependencies between evaluations and avoids unnecessary reevaluations. """ @type evaluation_response_metadata :: %{ + errored: boolean(), evaluation_time_ms: non_neg_integer(), code_error: code_error(), memory_usage: runtime_memory(), diff --git a/lib/livebook/runtime/evaluator.ex b/lib/livebook/runtime/evaluator.ex index 1b0c2197d..50d624913 100644 --- a/lib/livebook/runtime/evaluator.ex +++ b/lib/livebook/runtime/evaluator.ex @@ -466,6 +466,7 @@ defmodule Livebook.Runtime.Evaluator do output = state.formatter.format_result(result) metadata = %{ + errored: elem(result, 0) == :error, evaluation_time_ms: evaluation_time_ms, memory_usage: memory(), code_error: code_error, diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index 7e8f6a508..f65b4c1a0 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -88,11 +88,13 @@ defmodule Livebook.Session.Data do @type cell_eval_info :: %{ validity: cell_evaluation_validity(), status: cell_evaluation_status(), + errored: boolean(), snapshot: snapshot(), evaluation_digest: String.t() | nil, evaluation_snapshot: snapshot() | nil, evaluation_time_ms: integer() | nil, evaluation_start: DateTime.t() | nil, + evaluation_end: DateTime.t() | nil, evaluation_number: non_neg_integer(), outputs_batch_number: non_neg_integer(), bound_to_input_ids: MapSet.t(input_id()), @@ -1051,19 +1053,29 @@ defmodule Livebook.Session.Data do end defp finish_cell_evaluation(data_actions, cell, section, metadata) do - data_actions - |> update_cell_eval_info!(cell.id, fn eval_info -> - %{ - eval_info - | status: :ready, - evaluation_time_ms: metadata.evaluation_time_ms, - identifiers_used: metadata.identifiers_used, - identifiers_defined: metadata.identifiers_defined, - bound_to_input_ids: eval_info.new_bound_to_input_ids - } - end) - |> update_cell_evaluation_snapshot(cell, section) - |> set_section_info!(section.id, evaluating_cell_id: nil) + data_actions = + data_actions + |> update_cell_eval_info!(cell.id, fn eval_info -> + %{ + eval_info + | status: :ready, + errored: metadata.errored, + evaluation_time_ms: metadata.evaluation_time_ms, + identifiers_used: metadata.identifiers_used, + identifiers_defined: metadata.identifiers_defined, + bound_to_input_ids: eval_info.new_bound_to_input_ids, + evaluation_end: DateTime.utc_now() + } + end) + |> update_cell_evaluation_snapshot(cell, section) + |> set_section_info!(section.id, evaluating_cell_id: nil) + + if metadata.errored and not Map.get(cell, :continue_on_error, false) do + data_actions + |> unqueue_child_cells_evaluation(cell) + else + data_actions + end end defp update_cell_evaluation_snapshot({data, _} = data_actions, cell, section) do @@ -1241,7 +1253,8 @@ defmodule Livebook.Session.Data do data: data, # This is a rough estimate, the exact time is measured in the # evaluator itself - evaluation_start: DateTime.utc_now() + evaluation_start: DateTime.utc_now(), + evaluation_end: nil } end) |> set_section_info!(section.id, @@ -1786,9 +1799,11 @@ defmodule Livebook.Session.Data do %{ validity: :fresh, status: :ready, + errored: false, evaluation_digest: nil, evaluation_time_ms: nil, evaluation_start: nil, + evaluation_end: nil, evaluation_number: 0, outputs_batch_number: 0, bound_to_input_ids: MapSet.new(), @@ -1797,7 +1812,8 @@ defmodule Livebook.Session.Data do identifiers_defined: %{}, snapshot: nil, evaluation_snapshot: nil, - data: nil + data: nil, + reevaluates_automatically: false } end @@ -1923,6 +1939,7 @@ defmodule Livebook.Session.Data do data_actions |> compute_snapshots() |> update_validity() + |> update_reevaluates_automatically() # After updating validity there may be new stale cells, so we check # if any of them is configured for automatic reevaluation |> maybe_queue_reevaluating_cells() @@ -2066,15 +2083,45 @@ defmodule Livebook.Session.Data do end) end + defp update_reevaluates_automatically({data, _} = data_actions) do + eval_parents = cell_evaluation_parents(data) + + cells_with_section = Notebook.evaluable_cells_with_section(data.notebook) + cell_lookup = for {cell, _section} <- cells_with_section, into: %{}, do: {cell.id, cell} + + reduce(data_actions, cells_with_section, fn data_actions, {cell, _section} -> + info = data.cell_infos[cell.id] + + # Note that we disable automatic reevaluation if any evaluation + # parent errored (unless continue-on-error is enabled) + reevaluates_automatically = + Map.get(cell, :reevaluate_automatically, false) and + info.eval.validity in [:evaluated, :stale] and + not Enum.any?(eval_parents[cell.id], fn parent_id -> + errored? = data.cell_infos[parent_id].eval.errored + continue_on_error? = Map.get(cell_lookup[parent_id], :continue_on_error, false) + errored? and not continue_on_error? + end) + + if reevaluates_automatically == info.eval.reevaluates_automatically do + data_actions + else + update_cell_eval_info!( + data_actions, + cell.id, + &%{&1 | reevaluates_automatically: reevaluates_automatically} + ) + end + end) + end + defp maybe_queue_reevaluating_cells({data, _} = data_actions) do cells_to_reevaluate = data.notebook |> Notebook.evaluable_cells_with_section() |> Enum.filter(fn {cell, _section} -> info = data.cell_infos[cell.id] - - info.eval.status == :ready and info.eval.validity == :stale and - Map.get(cell, :reevaluate_automatically, false) + match?(%{status: :ready, validity: :stale, reevaluates_automatically: true}, info.eval) end) data_actions diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index ffb374ef9..b35eb84cb 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -2031,15 +2031,29 @@ defmodule LivebookWeb.SessionLive do end defp cells_status(cells, data) do + eval_infos = + for cell <- cells, + Cell.evaluable?(cell), + info = data.cell_infos[cell.id].eval, + do: Map.put(info, :id, cell.id) + + most_recent = + eval_infos + |> Enum.filter(& &1.evaluation_end) + |> Enum.max_by(& &1.evaluation_end, DateTime, fn -> nil end) + cond do - evaluating = Enum.find(cells, &evaluating?(&1, data)) -> + evaluating = Enum.find(eval_infos, &(&1.status == :evaluating)) -> {:evaluating, evaluating.id} - stale = Enum.find(cells, &stale?(&1, data)) -> + most_recent != nil and most_recent.errored -> + {:errored, most_recent.id} + + stale = Enum.find(eval_infos, &(&1.validity == :stale)) -> {:stale, stale.id} - evaluated = Enum.find(Enum.reverse(cells), &evaluated?(&1, data)) -> - {:evaluated, evaluated.id} + most_recent != nil -> + {:evaluated, most_recent.id} true -> {:fresh, nil} @@ -2055,18 +2069,6 @@ defmodule LivebookWeb.SessionLive do cells_status(cells, data) end - defp evaluating?(cell, data) do - get_in(data.cell_infos, [cell.id, :eval, :status]) == :evaluating - end - - defp stale?(cell, data) do - get_in(data.cell_infos, [cell.id, :eval, :validity]) == :stale - end - - defp evaluated?(cell, data) do - get_in(data.cell_infos, [cell.id, :eval, :validity]) == :evaluated - end - defp section_views(sections, data) do sections |> Enum.map(& &1.name) @@ -2142,6 +2144,8 @@ defmodule LivebookWeb.SessionLive do outputs: cell.outputs, validity: eval_info.validity, status: eval_info.status, + errored: eval_info.errored, + reevaluates_automatically: eval_info.reevaluates_automatically, evaluation_time_ms: eval_info.evaluation_time_ms, evaluation_start: eval_info.evaluation_start, evaluation_digest: encode_digest(eval_info.evaluation_digest), diff --git a/lib/livebook_web/live/session_live/cell_component.ex b/lib/livebook_web/live/session_live/cell_component.ex index f20846d98..f30258cd6 100644 --- a/lib/livebook_web/live/session_live/cell_component.ex +++ b/lib/livebook_web/live/session_live/cell_component.ex @@ -68,6 +68,7 @@ defmodule LivebookWeb.SessionLive.CellComponent do validity={@cell_view.eval.validity} status={@cell_view.eval.status} reevaluate_automatically={@cell_view.reevaluate_automatically} + reevaluates_automatically={@cell_view.eval.reevaluates_automatically} /> <:secondary> @@ -167,6 +168,7 @@ defmodule LivebookWeb.SessionLive.CellComponent do validity={@cell_view.eval.validity} status={@cell_view.eval.status} reevaluate_automatically={false} + reevaluates_automatically={@cell_view.eval.reevaluates_automatically} /> <:secondary> @@ -307,7 +309,7 @@ defmodule LivebookWeb.SessionLive.CellComponent do data-cell-id={@cell_id} > <%= cond do %> - <% @reevaluate_automatically and @validity in [:evaluated, :stale] -> %> + <% @reevaluates_automatically -> %> <.remix_icon icon="check-line" class="text-xl" /> Reevaluates automatically <% @validity == :evaluated -> %> @@ -653,7 +655,7 @@ defmodule LivebookWeb.SessionLive.CellComponent do defp cell_status(%{cell_view: %{eval: %{validity: :evaluated}}} = assigns) do ~H""" <.status_indicator - circle_class="bg-green-bright-400" + circle_class={if(@cell_view.eval.errored, do: "bg-red-400", else: "bg-green-bright-400")} change_indicator={true} tooltip={evaluated_label(@cell_view.eval.evaluation_time_ms)} > diff --git a/lib/livebook_web/live/session_live/code_cell_settings_component.ex b/lib/livebook_web/live/session_live/code_cell_settings_component.ex index af59c5ccf..9d63de8a1 100644 --- a/lib/livebook_web/live/session_live/code_cell_settings_component.ex +++ b/lib/livebook_web/live/session_live/code_cell_settings_component.ex @@ -12,6 +12,7 @@ defmodule LivebookWeb.SessionLive.CodeCellSettingsComponent do |> assign(assigns) |> assign_new(:disable_formatting, fn -> cell.disable_formatting end) |> assign_new(:reevaluate_automatically, fn -> cell.reevaluate_automatically end) + |> assign_new(:continue_on_error, fn -> cell.continue_on_error end) {:ok, socket} end @@ -38,6 +39,13 @@ defmodule LivebookWeb.SessionLive.CodeCellSettingsComponent do checked={@reevaluate_automatically} /> +
+ <.switch_checkbox + name="continue_on_error" + label="Continue on error" + checked={@continue_on_error} + /> +
+ + """ + end + defp global_status(%{status: :stale} = assigns) do ~H""" diff --git a/static/favicon-errored.svg b/static/favicon-errored.svg new file mode 100644 index 000000000..cd63fc106 --- /dev/null +++ b/static/favicon-errored.svg @@ -0,0 +1,87 @@ + + + + + + + image/svg+xml + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/livebook/live_markdown/export_test.exs b/test/livebook/live_markdown/export_test.exs index fecb61ac6..3667b13f3 100644 --- a/test/livebook/live_markdown/export_test.exs +++ b/test/livebook/live_markdown/export_test.exs @@ -29,6 +29,7 @@ defmodule Livebook.LiveMarkdown.ExportTest do Notebook.Cell.new(:code) | disable_formatting: true, reevaluate_automatically: true, + continue_on_error: true, source: """ Enum.to_list(1..10)\ """ @@ -101,7 +102,7 @@ defmodule Livebook.LiveMarkdown.ExportTest do $x_{i} + y_{i}$ - + ```elixir Enum.to_list(1..10) diff --git a/test/livebook/live_markdown/import_test.exs b/test/livebook/live_markdown/import_test.exs index 6ee023cff..d0d86148b 100644 --- a/test/livebook/live_markdown/import_test.exs +++ b/test/livebook/live_markdown/import_test.exs @@ -19,7 +19,7 @@ defmodule Livebook.LiveMarkdown.ImportTest do $x_{i} + y_{i}$ - + ```elixir Enum.to_list(1..10) @@ -80,6 +80,7 @@ defmodule Livebook.LiveMarkdown.ImportTest do %Cell.Code{ disable_formatting: true, reevaluate_automatically: true, + continue_on_error: true, source: """ Enum.to_list(1..10)\ """ diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs index f760eb4f4..8df6f42f8 100644 --- a/test/livebook/session/data_test.exs +++ b/test/livebook/session/data_test.exs @@ -15,7 +15,14 @@ defmodule Livebook.Session.DataTest do defp eval_meta(opts \\ []) do uses = opts[:uses] || [] defines = opts[:defines] || %{} - %{evaluation_time_ms: 10, identifiers_used: uses, identifiers_defined: defines} + errored = Keyword.get(opts, :errored, false) + + %{ + errored: errored, + evaluation_time_ms: 10, + identifiers_used: uses, + identifiers_defined: defines + } end describe "new/1" do @@ -2038,8 +2045,7 @@ defmodule Livebook.Session.DataTest do ]) operation = - {:add_cell_evaluation_response, @cid, "c1", @eval_resp, - eval_meta(%{defines: %{"c1" => 1}})} + {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta(defines: %{"c1" => 1})} assert {:ok, %{ @@ -2173,6 +2179,98 @@ defmodule Livebook.Session.DataTest do }, _actions} = Data.apply_operation(data, operation) end + test "unqueues child cells if the evaluation errored" do + data = + data_after_operations!([ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:insert_section, @cid, 1, "s2"}, + {:insert_cell, @cid, "s2", 0, :code, "c3", %{}}, + {:insert_cell, @cid, "s2", 1, :code, "c4", %{}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup"]), + {:queue_cells_evaluation, @cid, ["c1", "c2", "c3", "c4"]} + ]) + + operation = + {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta(errored: true)} + + assert {:ok, + %{ + cell_infos: %{ + "c2" => %{eval: %{status: :ready, validity: :fresh}}, + "c3" => %{eval: %{status: :ready, validity: :fresh}}, + "c4" => %{eval: %{status: :ready, validity: :fresh}} + }, + section_infos: %{ + "s1" => %{evaluating_cell_id: nil}, + "s2" => %{evaluating_cell_id: nil} + } + }, []} = Data.apply_operation(data, operation) + end + + test "disables child cells automatic reevaluation if the evaluation errored" do + data = + data_after_operations!([ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:insert_cell, @cid, "s1", 2, :code, "c3", %{}}, + {:set_cell_attributes, @cid, "c3", %{reevaluate_automatically: true}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup", "c1", "c2", "c3"], + uses: %{"c2" => ["c1"], "c3" => ["c2"]} + ), + {:queue_cells_evaluation, @cid, ["c1"]} + ]) + + operation = + {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta(errored: true)} + + assert {:ok, + %{ + cell_infos: %{ + "c2" => %{eval: %{status: :ready}}, + "c3" => %{eval: %{status: :ready, reevaluates_automatically: false}} + }, + section_infos: %{ + "s1" => %{evaluating_cell_id: nil} + } + }, []} = Data.apply_operation(data, operation) + end + + test "re-enables child cells automatic reevaluation if errored evaluation is fixed" do + data = + data_after_operations!([ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:insert_cell, @cid, "s1", 2, :code, "c3", %{}}, + {:set_cell_attributes, @cid, "c3", %{reevaluate_automatically: true}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup", "c1", "c2", "c3"], + uses: %{"c2" => ["c1"], "c3" => ["c2"]} + ), + {:queue_cells_evaluation, @cid, ["c1"]}, + {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta(errored: true)}, + {:queue_cells_evaluation, @cid, ["c1"]} + ]) + + operation = {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta()} + + assert {:ok, + %{ + cell_infos: %{ + "c2" => %{eval: %{status: :evaluating}}, + "c3" => %{eval: %{status: :queued, reevaluates_automatically: true}} + }, + section_infos: %{ + "s1" => %{evaluating_cell_id: "c2"} + } + }, _actions} = Data.apply_operation(data, operation) + end + test "if bound input value changes during cell evaluation, the cell is marked as stale afterwards" do input = %{id: "i1", type: :text, label: "Text", default: "hey"} diff --git a/test/livebook/session_test.exs b/test/livebook/session_test.exs index 614178f01..cbb6941d2 100644 --- a/test/livebook/session_test.exs +++ b/test/livebook/session_test.exs @@ -7,7 +7,12 @@ defmodule Livebook.SessionTest do alias Livebook.Notebook.{Section, Cell} alias Livebook.Session.Data - @eval_meta %{evaluation_time_ms: 10, identifiers_used: [], identifiers_defined: %{}} + @eval_meta %{ + errored: false, + evaluation_time_ms: 10, + identifiers_used: [], + identifiers_defined: %{} + } setup do session = start_session()