From 6b11f24d4ec1de6bbcf70eba8fdf9af94d76c25f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 25 May 2023 21:24:19 +0200 Subject: [PATCH 1/2] Automatically reevaluate app sessions and support interrupt --- lib/livebook/app.ex | 4 +- lib/livebook/notebook.ex | 11 ++ lib/livebook/runtime.ex | 6 +- lib/livebook/runtime/evaluator.ex | 5 + .../runtime/evaluator/default_formatter.ex | 3 + lib/livebook/session/data.ex | 110 ++++++++++-------- lib/livebook_web/live/app_helpers.ex | 28 +++-- lib/livebook_web/live/app_live.ex | 2 +- lib/livebook_web/live/app_session_live.ex | 58 ++++++--- lib/livebook_web/live/apps_live.ex | 7 +- lib/livebook_web/live/output.ex | 64 ++++++++-- .../live/output/frame_component.ex | 1 + lib/livebook_web/live/session_live.ex | 23 +++- .../live/session_live/app_info_component.ex | 23 ++-- .../live/session_live/cell_component.ex | 1 + test/livebook/app_test.exs | 38 ++++-- test/livebook/apps_test.exs | 8 +- test/livebook/session/data_test.exs | 100 ++++++++++++++-- test/livebook/session_test.exs | 16 ++- test/livebook_web/live/app_live_test.exs | 5 +- .../live/app_session_live_test.exs | 48 +++++++- test/livebook_web/live/apps_live_test.exs | 7 +- test/livebook_web/live/session_live_test.exs | 8 +- 23 files changed, 431 insertions(+), 145 deletions(-) diff --git a/lib/livebook/app.ex b/lib/livebook/app.ex index 713d9a734b0..04b4756bb0e 100644 --- a/lib/livebook/app.ex +++ b/lib/livebook/app.ex @@ -269,7 +269,7 @@ defmodule Livebook.App do pid: session.pid, version: state.version, created_at: session.created_at, - app_status: :executing, + app_status: %{execution: :executing, lifecycle: :active}, client_count: 0, started_by_id: user && user.id } @@ -318,7 +318,7 @@ defmodule Livebook.App do defp shutdown_old_versions(state), do: state defp shutdown_session(app_session) do - if Livebook.Session.Data.app_active?(app_session.app_status) do + if app_session.app_status.lifecycle == :active do Livebook.Session.app_shutdown(app_session.pid) end end diff --git a/lib/livebook/notebook.ex b/lib/livebook/notebook.ex index 6a9448b0b3e..5bce1c77c67 100644 --- a/lib/livebook/notebook.ex +++ b/lib/livebook/notebook.ex @@ -603,6 +603,17 @@ defmodule Livebook.Notebook do end) end + @doc """ + Removes all outputs from the notebook. + """ + @spec clear_outputs(t()) :: t() + def clear_outputs(notebook) do + update_cells(notebook, fn + %{outputs: _outputs} = cell -> %{cell | outputs: []} + cell -> cell + end) + end + @doc """ Adds new output to the given cell. diff --git a/lib/livebook/runtime.ex b/lib/livebook/runtime.ex index e412d39c019..4ec5ea3dd9d 100644 --- a/lib/livebook/runtime.ex +++ b/lib/livebook/runtime.ex @@ -66,7 +66,11 @@ defprotocol Livebook.Runtime do # A control element | {:control, attrs :: map()} # Internal output format for errors - | {:error, message :: String.t(), type :: {:missing_secret, String.t()} | :other} + | {:error, message :: String.t(), + type :: + {:missing_secret, name :: String.t()} + | {:interrupt, variant :: :normal | :error, message :: String.t()} + | :other} @typedoc """ Additional information about a completed evaluation. diff --git a/lib/livebook/runtime/evaluator.ex b/lib/livebook/runtime/evaluator.ex index 167f6994cd1..74761fc6bfe 100644 --- a/lib/livebook/runtime/evaluator.ex +++ b/lib/livebook/runtime/evaluator.ex @@ -467,6 +467,11 @@ defmodule Livebook.Runtime.Evaluator do metadata = %{ errored: elem(result, 0) == :error, + interrupted: + match?( + {:error, _kind, error, _stacktrace} when is_struct(error, Kino.InterruptError), + result + ), evaluation_time_ms: evaluation_time_ms, memory_usage: memory(), code_error: code_error, diff --git a/lib/livebook/runtime/evaluator/default_formatter.ex b/lib/livebook/runtime/evaluator/default_formatter.ex index ac754f91fd6..9cccba73832 100644 --- a/lib/livebook/runtime/evaluator/default_formatter.ex +++ b/lib/livebook/runtime/evaluator/default_formatter.ex @@ -139,5 +139,8 @@ defmodule Livebook.Runtime.Evaluator.DefaultFormatter do defp error_type(%System.EnvError{env: "LB_" <> secret_name}), do: {:missing_secret, secret_name} + defp error_type(error) when is_struct(error, Kino.InterruptError), + do: {:interrupt, error.variant, error.message} + defp error_type(_), do: :other end diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index f99557e244c..55951ed724c 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -149,10 +149,13 @@ defmodule Livebook.Session.Data do @type session_mode :: :default | :app - # Note that technically the first state is :initial, but we always - # expect app to start evaluating right away, so distinguishing that - # state from :executing would not bring any value - @type app_status :: :executing | :executed | :error | :shutting_down | :deactivated + @type app_status :: %{ + # Note that technically the first state is :initial, but we always + # expect app to start evaluating right away, so distinguishing that + # state from :executing would not bring any value + execution: :executing | :executed | :error, + lifecycle: :active | :shutting_down | :deactivated + } @type app_data :: %{ status: app_status() @@ -242,15 +245,23 @@ defmodule Livebook.Session.Data do notebook = opts[:notebook] + notebook = + if opts[:mode] == :app do + Livebook.Notebook.clear_outputs(notebook) + else + notebook + end + default_runtime = - case opts[:mode] do - :app -> Livebook.Config.default_app_runtime() - _ -> Livebook.Config.default_runtime() + if opts[:mode] == :app do + Livebook.Config.default_app_runtime() + else + Livebook.Config.default_runtime() end app_data = if opts[:mode] == :app do - %{status: :executing} + %{status: %{execution: :executing, lifecycle: :active}} end hub = Hubs.fetch_hub!(notebook.hub_id) @@ -529,7 +540,6 @@ defmodule Livebook.Session.Data do end) |> maybe_connect_runtime(data) |> update_validity_and_evaluation() - |> app_compute_status() |> wrap_ok() else :error @@ -573,7 +583,6 @@ defmodule Livebook.Session.Data do |> update_validity_and_evaluation() |> update_smart_cell_bases(data) |> mark_dirty_if_persisting_outputs() - |> app_compute_status() |> wrap_ok() else _ -> :error @@ -600,7 +609,7 @@ defmodule Livebook.Session.Data do |> with_actions() |> clear_main_evaluation() |> update_smart_cell_bases(data) - |> app_compute_status() + |> app_update_execution_status() |> wrap_ok() end @@ -610,7 +619,7 @@ defmodule Livebook.Session.Data do |> with_actions() |> clear_section_evaluation(section) |> update_smart_cell_bases(data) - |> app_compute_status() + |> app_update_execution_status() |> wrap_ok() end end @@ -622,7 +631,7 @@ defmodule Livebook.Session.Data do |> with_actions() |> cancel_cell_evaluation(cell, section) |> update_smart_cell_bases(data) - |> app_compute_status() + |> app_update_execution_status() |> wrap_ok() else _ -> :error @@ -824,7 +833,6 @@ defmodule Livebook.Session.Data do data |> with_actions() |> set_runtime(data, runtime) - |> app_compute_status() |> wrap_ok() end @@ -1213,6 +1221,7 @@ defmodule Livebook.Session.Data do eval_info | status: :ready, errored: metadata.errored, + interrupted: metadata.interrupted, evaluation_time_ms: metadata.evaluation_time_ms, identifiers_used: metadata.identifiers_used, identifiers_defined: metadata.identifiers_defined, @@ -1598,13 +1607,7 @@ defmodule Livebook.Session.Data do defp erase_outputs({data, _} = data_actions) do data_actions |> clear_all_evaluation() - |> set!( - notebook: - Notebook.update_cells(data.notebook, fn - %{outputs: _outputs} = cell -> %{cell | outputs: []} - cell -> cell - end) - ) + |> set!(notebook: Notebook.clear_outputs(data.notebook)) |> update_every_cell_info(fn %{eval: _} = info -> info = update_in(info.eval.outputs_batch_number, &(&1 + 1)) @@ -1809,18 +1812,19 @@ defmodule Livebook.Session.Data do defp app_deactivate(data_actions) do data_actions - |> set_app_data!(status: :deactivated) + |> update_app_data!(&put_in(&1.status.lifecycle, :deactivated)) |> add_action(:app_report_status) end defp app_shutdown(data_actions) do data_actions - |> set_app_data!(status: :shutting_down) + |> update_app_data!(&put_in(&1.status.lifecycle, :shutting_down)) |> add_action(:app_report_status) end defp app_maybe_terminate({data, _} = data_actions) do - if data.mode == :app and data.app_data.status == :shutting_down and data.clients_map == %{} do + if data.mode == :app and data.app_data.status.lifecycle == :shutting_down and + data.clients_map == %{} do add_action(data_actions, :app_terminate) else data_actions @@ -2009,6 +2013,7 @@ defmodule Livebook.Session.Data do validity: :fresh, status: :ready, errored: false, + interrupted: false, evaluation_digest: nil, evaluation_time_ms: nil, evaluation_start: nil, @@ -2083,13 +2088,8 @@ defmodule Livebook.Session.Data do Enum.all?(attrs, fn {key, _} -> Map.has_key?(struct, key) end) end - defp set_app_data!({data, _} = data_actions, changes) do - app_data = - Enum.reduce(changes, data.app_data, fn {key, value}, app_data -> - Map.replace!(app_data, key, value) - end) - - set!(data_actions, app_data: app_data) + defp update_app_data!({data, _} = data_actions, fun) do + set!(data_actions, app_data: fun.(data.app_data)) end @doc """ @@ -2157,12 +2157,14 @@ defmodule Livebook.Session.Data do data_actions |> compute_snapshots() |> update_validity() + |> app_update_execution_status() |> update_reevaluates_automatically() # After updating validity there may be new stale cells, so we check - # if any of them is configured for automatic reevaluation + # if any of them should be automatically reevaluated |> maybe_queue_reevaluating_cells() |> queue_prerequisite_cells_evaluation_for_queued() |> maybe_evaluate_queued() + |> app_update_execution_status() end defp compute_snapshots({data, _} = data_actions) do @@ -2301,6 +2303,9 @@ defmodule Livebook.Session.Data do end) end + defp update_reevaluates_automatically({data, _} = data_actions) when data.mode == :app, + do: data_actions + defp update_reevaluates_automatically({data, _} = data_actions) do eval_parents = cell_evaluation_parents(data) @@ -2339,7 +2344,18 @@ defmodule Livebook.Session.Data do |> Notebook.evaluable_cells_with_section() |> Enum.filter(fn {cell, _section} -> info = data.cell_infos[cell.id] - match?(%{status: :ready, validity: :stale, reevaluates_automatically: true}, info.eval) + + case data.mode do + :default -> + match?( + %{status: :ready, validity: :stale, reevaluates_automatically: true}, + info.eval + ) + + :app -> + match?(%{status: :ready, validity: :stale}, info.eval) and + data.app_data.status.execution in [:executing, :executed] + end end) cell_ids = for {cell, _section} <- cells_to_reevaluate, do: cell.id @@ -2351,43 +2367,43 @@ defmodule Livebook.Session.Data do end) end - defp app_compute_status({data, _} = data_actions) + defp app_update_execution_status({data, _} = data_actions) when data.mode != :app, do: data_actions - defp app_compute_status({data, _} = data_actions) - when data.app_data.status in [:shutting_down, :deactivated], - do: data_actions - - defp app_compute_status({data, _} = data_actions) do - status = + defp app_update_execution_status({data, _} = data_actions) do + execution_status = data.notebook |> Notebook.evaluable_cells_with_section() |> Enum.find_value(:executed, fn {cell, _section} -> case data.cell_infos[cell.id].eval do %{validity: :aborted} -> :error + %{interrupted: true} -> :interrupted %{errored: true} -> :error %{validity: :fresh} -> :executing %{status: :evaluating} -> :executing + %{status: :queued} -> :executing _ -> nil end end) data_actions = - if data.app_data.status == status do + if data.app_data.status.execution == execution_status do data_actions else add_action(data_actions, :app_report_status) end + # If everything was executed and an error happened, it means it + # was a runtime crash and everything is aborted data_actions = - if data.app_data.status == :executed and status == :error do + if data.app_data.status.execution == :executed and execution_status == :error do add_action(data_actions, :app_recover) else data_actions end - set_app_data!(data_actions, status: status) + update_app_data!(data_actions, &put_in(&1.status.execution, execution_status)) end @doc """ @@ -2566,12 +2582,4 @@ defmodule Livebook.Session.Data do secret.value end end - - @doc """ - Checks if the app should be accessible and accepts new clients. - """ - @spec app_active?(app_status()) :: boolean() - def app_active?(app_status) do - app_status not in [:deactivated, :shutting_down] - end end diff --git a/lib/livebook_web/live/app_helpers.ex b/lib/livebook_web/live/app_helpers.ex index fbf9402058a..e638f7d6a62 100644 --- a/lib/livebook_web/live/app_helpers.ex +++ b/lib/livebook_web/live/app_helpers.ex @@ -15,36 +15,42 @@ defmodule LivebookWeb.AppHelpers do @doc """ Renders app status with indicator. """ - attr :status, :atom, required: true + attr :status, :map, required: true attr :show_label, :boolean, default: true - def app_status(%{status: :executing} = assigns) do + def app_status(%{status: %{lifecycle: :shutting_down}} = assigns) do ~H""" - <.app_status_indicator text={@show_label && "Executing"} variant={:progressing} /> + <.app_status_indicator text={@show_label && "Shutting down"} variant={:inactive} /> """ end - def app_status(%{status: :executed} = assigns) do + def app_status(%{status: %{lifecycle: :deactivated}} = assigns) do ~H""" - <.app_status_indicator text={@show_label && "Executed"} variant={:success} /> + <.app_status_indicator text={@show_label && "Deactivated"} variant={:inactive} /> """ end - def app_status(%{status: :error} = assigns) do + def app_status(%{status: %{execution: :executing}} = assigns) do ~H""" - <.app_status_indicator text={@show_label && "Error"} variant={:error} /> + <.app_status_indicator text={@show_label && "Executing"} variant={:progressing} /> """ end - def app_status(%{status: :shutting_down} = assigns) do + def app_status(%{status: %{execution: :executed}} = assigns) do ~H""" - <.app_status_indicator text={@show_label && "Shutting down"} variant={:inactive} /> + <.app_status_indicator text={@show_label && "Executed"} variant={:success} /> """ end - def app_status(%{status: :deactivated} = assigns) do + def app_status(%{status: %{execution: :error}} = assigns) do ~H""" - <.app_status_indicator text={@show_label && "Deactivated"} variant={:inactive} /> + <.app_status_indicator text={@show_label && "Error"} variant={:error} /> + """ + end + + def app_status(%{status: %{execution: :interrupted}} = assigns) do + ~H""" + <.app_status_indicator text={@show_label && "Interrupted"} variant={:waiting} /> """ end diff --git a/lib/livebook_web/live/app_live.ex b/lib/livebook_web/live/app_live.ex index 5756e39da35..def0ca2a9f5 100644 --- a/lib/livebook_web/live/app_live.ex +++ b/lib/livebook_web/live/app_live.ex @@ -109,6 +109,6 @@ defmodule LivebookWeb.AppLive do def handle_info(_message, socket), do: {:noreply, socket} defp active_sessions(sessions) do - Enum.filter(sessions, &Livebook.Session.Data.app_active?(&1.app_status)) + Enum.filter(sessions, &(&1.app_status.lifecycle == :active)) end end diff --git a/lib/livebook_web/live/app_session_live.ex b/lib/livebook_web/live/app_session_live.ex index 37993be054b..d924e5a4c19 100644 --- a/lib/livebook_web/live/app_session_live.ex +++ b/lib/livebook_web/live/app_session_live.ex @@ -14,7 +14,7 @@ defmodule LivebookWeb.AppSessionLive do app_session = Enum.find(app.sessions, &(&1.id == session_id)) - if app_session && Session.Data.app_active?(app_session.app_status) do + if app_session && app_session.app_status.lifecycle == :active do %{pid: session_pid} = app_session session = Session.get_by_pid(session_pid) @@ -127,19 +127,30 @@ defmodule LivebookWeb.AppSessionLive do
-
- -
+ <%= if @data_view.app_status.execution == :error do %> +
+ <.remix_icon icon="error-warning-line" class="text-xl" /> + Something went wrong +
+ <% else %> +
+ +
+ <% end %>
+
+ <.app_status status={@data_view.app_status} /> +
<.modal @@ -167,6 +178,18 @@ defmodule LivebookWeb.AppSessionLive do @impl true def handle_params(_params, _url, socket), do: {:noreply, socket} + @impl true + def handle_event("queue_interrupted_cell_evaluation", %{"cell_id" => cell_id}, socket) do + data = socket.private.data + + with {:ok, cell, _section} <- Notebook.fetch_cell_and_section(data.notebook, cell_id), + true <- data.cell_infos[cell.id].eval.interrupted do + Session.queue_full_evaluation(socket.assigns.session.pid, [cell_id]) + end + + {:noreply, socket} + end + @impl true def handle_info({:operation, operation}, socket) do {:noreply, handle_operation(socket, operation)} @@ -250,10 +273,11 @@ defmodule LivebookWeb.AppSessionLive do notebook_name: data.notebook.name, output_views: for( - output <- visible_outputs(data.notebook), + {cell_id, output} <- visible_outputs(data.notebook), do: %{ output: output, - input_values: input_values_for_output(output, data) + input_values: input_values_for_output(output, data), + cell_id: cell_id } ), app_status: data.app_data.status, @@ -272,7 +296,7 @@ defmodule LivebookWeb.AppSessionLive do cell <- Enum.reverse(section.cells), Cell.evaluable?(cell), output <- filter_outputs(cell.outputs, notebook.app_settings.output_type), - do: output + do: {cell.id, output} end defp filter_outputs(outputs, :all), do: outputs @@ -310,5 +334,11 @@ defmodule LivebookWeb.AppSessionLive do {idx, {:frame, outputs, metadata}} end + defp filter_output({idx, {:error, _message, {:interrupt, _, _}} = output}), + do: {idx, output} + defp filter_output(_output), do: nil + + defp show_app_status?(%{execution: :executed, lifecycle: :active}), do: false + defp show_app_status?(_status), do: true end diff --git a/lib/livebook_web/live/apps_live.ex b/lib/livebook_web/live/apps_live.ex index df8c3484bd5..3868bea0af6 100644 --- a/lib/livebook_web/live/apps_live.ex +++ b/lib/livebook_web/live/apps_live.ex @@ -116,10 +116,7 @@ defmodule LivebookWeb.AppsLive do <:actions :let={app_session}> @@ -131,7 +128,7 @@ defmodule LivebookWeb.AppsLive do <.remix_icon icon="terminal-line" class="text-lg" /> - <%= if Livebook.Session.Data.app_active?(app_session.app_status) do %> + <%= if app_session.app_status.lifecycle == :active do %> + + """ + end + defp render_output({:error, formatted, _type}, %{}) do render_formatted_error_message(formatted) end diff --git a/lib/livebook_web/live/output/frame_component.ex b/lib/livebook_web/live/output/frame_component.ex index bc237d7b7db..c98effb06d6 100644 --- a/lib/livebook_web/live/output/frame_component.ex +++ b/lib/livebook_web/live/output/frame_component.ex @@ -87,6 +87,7 @@ defmodule LivebookWeb.Output.FrameComponent do session_pid={@session_pid} input_values={@input_values} client_id={@client_id} + cell_id={@cell_id} /> <% end %> diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index 86181d2b460..3466a5e460f 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -1173,6 +1173,18 @@ defmodule LivebookWeb.SessionLive do {:noreply, socket} end + @impl true + def handle_event("queue_interrupted_cell_evaluation", %{"cell_id" => cell_id}, socket) do + data = socket.private.data + + with {:ok, cell, _section} <- Notebook.fetch_cell_and_section(data.notebook, cell_id), + true <- data.cell_infos[cell.id].eval.interrupted do + Session.queue_cell_evaluation(socket.assigns.session.pid, cell_id) + end + + {:noreply, socket} + end + def handle_event("queue_section_evaluation", %{"section_id" => section_id}, socket) do Session.queue_section_evaluation(socket.assigns.session.pid, section_id) @@ -2327,9 +2339,10 @@ defmodule LivebookWeb.SessionLive do end defp app_status_color(nil), do: "bg-gray-400" - defp app_status_color(:executing), do: "bg-blue-500" - defp app_status_color(:executed), do: "bg-green-bright-400" - defp app_status_color(:error), do: "bg-red-400" - defp app_status_color(:shutting_down), do: "bg-gray-500" - defp app_status_color(:deactivated), do: "bg-gray-500" + defp app_status_color(%{lifecycle: :shutting_down}), do: "bg-gray-500" + defp app_status_color(%{lifecycle: :deactivated}), do: "bg-gray-500" + defp app_status_color(%{execution: :executing}), do: "bg-blue-500" + defp app_status_color(%{execution: :executed}), do: "bg-green-bright-400" + defp app_status_color(%{execution: :error}), do: "bg-red-400" + defp app_status_color(%{execution: :interrupted}), do: "bg-gray-400" end diff --git a/lib/livebook_web/live/session_live/app_info_component.ex b/lib/livebook_web/live/session_live/app_info_component.ex index 305d2adb11b..bb032a18b87 100644 --- a/lib/livebook_web/live/session_live/app_info_component.ex +++ b/lib/livebook_web/live/session_live/app_info_component.ex @@ -96,10 +96,7 @@ defmodule LivebookWeb.SessionLive.AppInfoComponent do
@@ -112,34 +109,34 @@ defmodule LivebookWeb.SessionLive.AppInfoComponent do <.remix_icon icon="terminal-line" class="text-lg" /> - <%= if app_session.app_status in [:deactivated, :shutting_down] do %> - + <%= if app_session.app_status.lifecycle == :active do %> + <% else %> - + <% end %> diff --git a/lib/livebook_web/live/session_live/cell_component.ex b/lib/livebook_web/live/session_live/cell_component.ex index d3d6041787d..cd976ba02b6 100644 --- a/lib/livebook_web/live/session_live/cell_component.ex +++ b/lib/livebook_web/live/session_live/cell_component.ex @@ -594,6 +594,7 @@ defmodule LivebookWeb.SessionLive.CellComponent do session_id={@session_id} session_pid={@session_pid} client_id={@client_id} + cell_id={@cell_view.id} input_values={@cell_view.eval.input_values} />
diff --git a/test/livebook/app_test.exs b/test/livebook/app_test.exs index 538ed5a04ee..cf1a6b1d31c 100644 --- a/test/livebook/app_test.exs +++ b/test/livebook/app_test.exs @@ -62,7 +62,9 @@ defmodule Livebook.AppTest do assert_receive {:app_updated, %{ version: 2, - sessions: [%{id: ^session_id, app_status: :executed, version: 1}] + sessions: [ + %{id: ^session_id, app_status: %{execution: :executed}, version: 1} + ] }} end @@ -74,12 +76,17 @@ defmodule Livebook.AppTest do App.subscribe(slug) app_pid = start_app(notebook) - assert_receive {:app_updated, %{sessions: [%{app_status: :executed, version: 1}]}} + + assert_receive {:app_updated, + %{sessions: [%{app_status: %{execution: :executed}, version: 1}]}} App.deploy(app_pid, notebook) - assert_receive {:app_updated, %{sessions: [%{app_status: :executing, version: 2}]}} - assert_receive {:app_updated, %{sessions: [%{app_status: :executed, version: 2}]}} + assert_receive {:app_updated, + %{sessions: [%{app_status: %{execution: :executing}, version: 2}]}} + + assert_receive {:app_updated, + %{sessions: [%{app_status: %{execution: :executed}, version: 2}]}} end test "keeps old executed session during single-session zero-downtime deployment" do @@ -90,19 +97,22 @@ defmodule Livebook.AppTest do App.subscribe(slug) app_pid = start_app(notebook) - assert_receive {:app_updated, %{sessions: [%{app_status: :executed, version: 1}]}} + + assert_receive {:app_updated, + %{sessions: [%{app_status: %{execution: :executed}, version: 1}]}} App.deploy(app_pid, notebook) assert_receive {:app_updated, %{ sessions: [ - %{app_status: :executing, version: 2}, - %{app_status: :executed, version: 1} + %{app_status: %{execution: :executing}, version: 2}, + %{app_status: %{execution: :executed}, version: 1} ] }} - assert_receive {:app_updated, %{sessions: [%{app_status: :executed, version: 2}]}} + assert_receive {:app_updated, + %{sessions: [%{app_status: %{execution: :executed}, version: 2}]}} end end @@ -130,21 +140,25 @@ defmodule Livebook.AppTest do App.subscribe(slug) app_pid = start_app(notebook) - assert_receive {:app_updated, %{sessions: [%{id: session_id1, app_status: :executed}]}} + + assert_receive {:app_updated, + %{sessions: [%{id: session_id1, app_status: %{execution: :executed}}]}} App.deploy(app_pid, notebook) assert_receive {:app_updated, %{ sessions: [ - %{id: session_id2, app_status: :executing}, - %{id: ^session_id1, app_status: :executed} + %{id: session_id2, app_status: %{execution: :executing}}, + %{id: ^session_id1, app_status: %{execution: :executed}} ] }} assert ^session_id1 = App.get_session_id(app_pid) - assert_receive {:app_updated, %{sessions: [%{id: ^session_id2, app_status: :executed}]}} + assert_receive {:app_updated, + %{sessions: [%{id: ^session_id2, app_status: %{execution: :executed}}]}} + assert ^session_id2 = App.get_session_id(app_pid) end diff --git a/test/livebook/apps_test.exs b/test/livebook/apps_test.exs index a8f33866873..f5938168e69 100644 --- a/test/livebook/apps_test.exs +++ b/test/livebook/apps_test.exs @@ -26,10 +26,14 @@ defmodule Livebook.AppsTest do Livebook.Apps.deploy_apps_in_dir(tmp_dir) assert_receive {:app_created, %{slug: "app1"} = app1} - assert_receive {:app_updated, %{slug: "app1", sessions: [%{app_status: :executed}]}} + + assert_receive {:app_updated, + %{slug: "app1", sessions: [%{app_status: %{execution: :executed}}]}} assert_receive {:app_created, %{slug: "app2"} = app2} - assert_receive {:app_updated, %{slug: "app2", sessions: [%{app_status: :executed}]}} + + assert_receive {:app_updated, + %{slug: "app2", sessions: [%{app_status: %{execution: :executed}}]}} Livebook.App.close(app1.pid) Livebook.App.close(app2.pid) diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs index a14ace54c75..564802a2a18 100644 --- a/test/livebook/session/data_test.exs +++ b/test/livebook/session/data_test.exs @@ -15,9 +15,11 @@ defmodule Livebook.Session.DataTest do uses = opts[:uses] || [] defines = opts[:defines] || %{} errored = Keyword.get(opts, :errored, false) + interrupted = Keyword.get(opts, :interrupted, false) %{ errored: errored, + interrupted: interrupted, evaluation_time_ms: 10, identifiers_used: uses, identifiers_defined: defines @@ -66,6 +68,27 @@ defmodule Livebook.Session.DataTest do assert snapshot != nil end + + test "clears all outputs when in app mode" do + notebook = %{ + Notebook.new() + | sections: [ + %{ + Notebook.Section.new() + | id: "s1", + cells: [ + %{Notebook.Cell.new(:code) | id: "c1", outputs: [{0, {:stdout, "Hello!"}}]} + ] + } + ] + } + + assert %{notebook: %{sections: [%{cells: [%{outputs: [{0, {:stdout, "Hello!"}}]}]}]}} = + Data.new(notebook: notebook) + + assert %{notebook: %{sections: [%{cells: [%{outputs: []}]}]}} = + Data.new(notebook: notebook, mode: :app) + end end describe "apply_operation/2 given :insert_section" do @@ -3779,7 +3802,7 @@ defmodule Livebook.Session.DataTest do operation = {:app_deactivate, @cid} - assert {:ok, %{app_data: %{status: :deactivated}}, [:app_report_status]} = + assert {:ok, %{app_data: %{status: %{lifecycle: :deactivated}}}, [:app_report_status]} = Data.apply_operation(data, operation) end end @@ -3800,8 +3823,8 @@ defmodule Livebook.Session.DataTest do operation = {:app_shutdown, @cid} - assert {:ok, %{app_data: %{status: :shutting_down}}, [:app_report_status, :app_terminate]} = - Data.apply_operation(data, operation) + assert {:ok, %{app_data: %{status: %{lifecycle: :shutting_down}}}, + [:app_report_status, :app_terminate]} = Data.apply_operation(data, operation) end test "does not return terminate action if there are clients" do @@ -3814,7 +3837,7 @@ defmodule Livebook.Session.DataTest do operation = {:app_shutdown, @cid} - assert {:ok, %{app_data: %{status: :shutting_down}}, [:app_report_status]} = + assert {:ok, %{app_data: %{status: %{lifecycle: :shutting_down}}}, [:app_report_status]} = Data.apply_operation(data, operation) end end @@ -3833,7 +3856,7 @@ defmodule Livebook.Session.DataTest do operation = {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta()} - assert {:ok, %{app_data: %{status: :executing}}, _actions} = + assert {:ok, %{app_data: %{status: %{execution: :executing}}}, _actions} = Data.apply_operation(data, operation) end @@ -3851,7 +3874,7 @@ defmodule Livebook.Session.DataTest do operation = {:add_cell_evaluation_response, @cid, "c1", @eval_resp, eval_meta(errored: true)} - assert {:ok, %{app_data: %{status: :error}}, [:app_report_status]} = + assert {:ok, %{app_data: %{status: %{execution: :error}}}, [:app_report_status]} = Data.apply_operation(data, operation) end @@ -3868,7 +3891,7 @@ defmodule Livebook.Session.DataTest do operation = {:add_cell_evaluation_response, @cid, "c2", @eval_resp, eval_meta()} - assert {:ok, %{app_data: %{status: :executed}}, [:app_report_status]} = + assert {:ok, %{app_data: %{status: %{execution: :executed}}}, [:app_report_status]} = Data.apply_operation(data, operation) end @@ -3884,7 +3907,43 @@ defmodule Livebook.Session.DataTest do operation = {:reflect_main_evaluation_failure, @cid} - assert {:ok, %{app_data: %{status: :error}}, [:app_report_status, :app_recover]} = + assert {:ok, %{app_data: %{status: %{execution: :error}}}, + [:app_report_status, :app_recover]} = Data.apply_operation(data, operation) + end + + test "changes status to :error when evaluation finishes with error" do + data = + data_after_operations!(Data.new(mode: :app), [ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup", "c1"]), + {:queue_cells_evaluation, @cid, ["c2"]} + ]) + + operation = + {:add_cell_evaluation_response, @cid, "c2", @eval_resp, eval_meta(errored: true)} + + assert {:ok, %{app_data: %{status: %{execution: :error}}}, [:app_report_status]} = + Data.apply_operation(data, operation) + end + + test "changes status to :interrupted when evaluation fails with interrupt error" do + data = + data_after_operations!(Data.new(mode: :app), [ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup", "c1"]), + {:queue_cells_evaluation, @cid, ["c2"]} + ]) + + operation = + {:add_cell_evaluation_response, @cid, "c2", @eval_resp, eval_meta(interrupted: true)} + + assert {:ok, %{app_data: %{status: %{execution: :interrupted}}}, [:app_report_status]} = Data.apply_operation(data, operation) end @@ -3903,7 +3962,7 @@ defmodule Livebook.Session.DataTest do operation = {:add_cell_evaluation_response, @cid, "c2", @eval_resp, eval_meta()} - assert {:ok, %{app_data: %{status: :executed}}, [:app_report_status]} = + assert {:ok, %{app_data: %{status: %{execution: :executed}}}, [:app_report_status]} = Data.apply_operation(data, operation) end @@ -3918,7 +3977,28 @@ defmodule Livebook.Session.DataTest do operation = {:client_leave, @cid} - assert {:ok, %{app_data: %{status: :shutting_down}}, [:app_terminate]} = + assert {:ok, %{app_data: %{status: %{lifecycle: :shutting_down}}}, [:app_terminate]} = + Data.apply_operation(data, operation) + end + + test "changes status to :executing on automatic reevaluation" do + input = %{id: "i1", type: :text, label: "Text", default: "hey"} + + data = + data_after_operations!(Data.new(mode: :app), [ + {:insert_section, @cid, 0, "s1"}, + {:insert_cell, @cid, "s1", 0, :code, "c1", %{}}, + {:insert_cell, @cid, "s1", 1, :code, "c2", %{}}, + {:set_runtime, @cid, connected_noop_runtime()}, + evaluate_cells_operations(["setup"]), + {:queue_cells_evaluation, @cid, ["c1"]}, + {:add_cell_evaluation_response, @cid, "c1", {:input, input}, eval_meta()}, + evaluate_cells_operations(["c2"], bind_inputs: %{"c2" => ["i1"]}) + ]) + + operation = {:set_input_value, @cid, "i1", "stuff"} + + assert {:ok, %{app_data: %{status: %{execution: :executing}}}, _actions} = Data.apply_operation(data, operation) end end diff --git a/test/livebook/session_test.exs b/test/livebook/session_test.exs index 9874d5db116..f1c85d69f6c 100644 --- a/test/livebook/session_test.exs +++ b/test/livebook/session_test.exs @@ -10,6 +10,7 @@ defmodule Livebook.SessionTest do @eval_meta %{ errored: false, + interrupted: false, evaluation_time_ms: 10, identifiers_used: [], identifiers_defined: %{} @@ -1223,13 +1224,20 @@ defmodule Livebook.SessionTest do {:ok, app_pid} = Apps.deploy(notebook) assert_receive {:app_created, %{pid: ^app_pid} = app} - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :executed}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :executed}}]}} Process.exit(Process.whereis(test), :shutdown) - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :error}]}} - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :executing}]}} - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :executed}]}} + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :error}}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :executing}}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :executed}}]}} App.close(app.pid) end diff --git a/test/livebook_web/live/app_live_test.exs b/test/livebook_web/live/app_live_test.exs index bb9498cd70e..d534fdfc981 100644 --- a/test/livebook_web/live/app_live_test.exs +++ b/test/livebook_web/live/app_live_test.exs @@ -59,7 +59,10 @@ defmodule LivebookWeb.AppLiveTest do Livebook.Session.app_deactivate(session_pid3) assert_receive {:app_updated, - %{pid: ^app_pid, sessions: [%{app_status: :deactivated}, _, _]}} + %{ + pid: ^app_pid, + sessions: [%{app_status: %{lifecycle: :deactivated}}, _, _] + }} refute render(view) =~ ~p"/apps/#{slug}/#{session_id3}" diff --git a/test/livebook_web/live/app_session_live_test.exs b/test/livebook_web/live/app_session_live_test.exs index fa064db7150..92f4937d3ef 100644 --- a/test/livebook_web/live/app_session_live_test.exs +++ b/test/livebook_web/live/app_session_live_test.exs @@ -34,7 +34,9 @@ defmodule LivebookWeb.AppSessionLiveTest do %{pid: ^app_pid, sessions: [%{id: session_id, pid: session_pid}]}} Livebook.Session.app_deactivate(session_pid) - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :deactivated}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{lifecycle: :deactivated}}]}} {:ok, view, _} = live(conn, ~p"/apps/#{slug}/#{session_id}") assert render(view) =~ "This app session does not exist" @@ -94,7 +96,9 @@ defmodule LivebookWeb.AppSessionLiveTest do {:ok, app_pid} = Apps.deploy(notebook) assert_receive {:app_created, %{pid: ^app_pid} = app} - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :executed}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :executed}}]}} {:ok, view, _} = conn |> live(~p"/apps/#{slug}") |> follow_redirect(conn) @@ -103,4 +107,44 @@ defmodule LivebookWeb.AppSessionLiveTest do Livebook.App.close(app.pid) end + + test "only shows an error message when session errors", %{conn: conn} do + slug = Livebook.Utils.random_short_id() + app_settings = %{Livebook.Notebook.AppSettings.new() | slug: slug} + + notebook = %{ + Livebook.Notebook.new() + | app_settings: app_settings, + sections: [ + %{ + Livebook.Notebook.Section.new() + | cells: [ + %{ + Livebook.Notebook.Cell.new(:code) + | source: source_for_output({:stdout, "Printed output"}) + }, + %{ + Livebook.Notebook.Cell.new(:code) + | source: ~s/raise "oops"/ + } + ] + } + ] + } + + Livebook.Apps.subscribe() + {:ok, app_pid} = Apps.deploy(notebook) + + assert_receive {:app_created, %{pid: ^app_pid} = app} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :error}}]}} + + {:ok, view, _} = conn |> live(~p"/apps/#{slug}") |> follow_redirect(conn) + + assert render(view) =~ "Something went wrong" + refute render(view) =~ "Printed output" + + Livebook.App.close(app.pid) + end end diff --git a/test/livebook_web/live/apps_live_test.exs b/test/livebook_web/live/apps_live_test.exs index 6dc6bea80d1..02bfc8a5976 100644 --- a/test/livebook_web/live/apps_live_test.exs +++ b/test/livebook_web/live/apps_live_test.exs @@ -59,13 +59,16 @@ defmodule LivebookWeb.AppsLiveTest do {:ok, app_pid} = Apps.deploy(notebook) assert_receive {:app_created, %{pid: ^app_pid}} - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :executed}]}} + + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{execution: :executed}}]}} view |> element(~s/[data-app-slug="#{slug}"] button[aria-label="deactivate app session"]/) |> render_click() - assert_receive {:app_updated, %{pid: ^app_pid, sessions: [%{app_status: :deactivated}]}} + assert_receive {:app_updated, + %{pid: ^app_pid, sessions: [%{app_status: %{lifecycle: :deactivated}}]}} view |> element(~s/[data-app-slug="#{slug}"] button[aria-label="terminate app session"]/) diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs index 3ae56da6f6e..945a99cc665 100644 --- a/test/livebook_web/live/session_live_test.exs +++ b/test/livebook_web/live/session_live_test.exs @@ -1449,7 +1449,10 @@ defmodule LivebookWeb.SessionLiveTest do assert_receive {:app_created, %{slug: ^slug} = app} assert_receive {:app_updated, - %{slug: ^slug, sessions: [%{app_status: :executed} = app_session]}} + %{ + slug: ^slug, + sessions: [%{app_status: %{execution: :executed}} = app_session] + }} {:ok, view, _} = live(conn, ~p"/sessions/#{session.id}") @@ -1457,7 +1460,8 @@ defmodule LivebookWeb.SessionLiveTest do |> element(~s/[data-el-app-info] button[aria-label="deactivate app session"]/) |> render_click() - assert_receive {:app_updated, %{slug: ^slug, sessions: [%{app_status: :deactivated}]}} + assert_receive {:app_updated, + %{slug: ^slug, sessions: [%{app_status: %{lifecycle: :deactivated}}]}} assert render(view) =~ "/apps/#{slug}/#{app_session.id}" From 218a479075ac5fc9469295d6f7989dbf8eac9c7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 25 May 2023 21:35:34 +0200 Subject: [PATCH 2/2] Up --- lib/livebook/app.ex | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/livebook/app.ex b/lib/livebook/app.ex index 04b4756bb0e..ca95e82d8af 100644 --- a/lib/livebook/app.ex +++ b/lib/livebook/app.ex @@ -237,12 +237,15 @@ defmodule Livebook.App do app_session = Enum.find(state.sessions, &(&1.version == state.version)) if app_session do - if state.notebook.app_settings.zero_downtime and app_session.app_status != :executed do - Enum.find(state.sessions, &(&1.app_status == :executed)) + if state.notebook.app_settings.zero_downtime and not status_ready?(app_session.app_status) do + Enum.find(state.sessions, &status_ready?(&1.app_status)) end || app_session end end + defp status_ready?(%{execution: :executed, lifecycle: :active}), do: true + defp status_ready?(_status), do: false + defp start_eagerly(state) when state.notebook.app_settings.multi_session, do: state defp start_eagerly(state) do