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

Use more precise error tags when communicating with IO proxy #413

Merged
merged 1 commit into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion lib/kino.ex
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ defmodule Kino do
def tmp_dir() do
case Kino.Bridge.get_tmp_dir() do
{:ok, path} -> path
{:error, _} -> nil
_ -> nil
end
end

Expand Down
46 changes: 24 additions & 22 deletions lib/kino/bridge.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defmodule Kino.Bridge do
# achieved via the group leader. For the implementation of
# that group leader see Livebook.Evaluator.IOProxy

@type request_error :: :unsupported | :terminated
@type request_error :: {:request_error, :unsupported | :terminated}

@doc """
Generates a unique, reevaluation-safe token.
Expand All @@ -19,14 +19,14 @@ defmodule Kino.Bridge do
def generate_token() do
case io_request(:livebook_generate_token) do
{:ok, token} -> token
{:error, _} -> System.unique_integer()
{:request_error, _} -> System.unique_integer()
end
end

@doc """
Sends the given output as intermediate evaluation result.
"""
@spec put_output(Kino.Output.t()) :: :ok | {:error, request_error()}
@spec put_output(Kino.Output.t()) :: :ok | request_error()
def put_output(output) do
with {:ok, reply} <- io_request({:livebook_put_output, output}), do: reply
end
Expand All @@ -35,7 +35,7 @@ defmodule Kino.Bridge do
Sends the given output as intermediate evaluation result directly
to a specific client.
"""
@spec put_output_to(term(), Kino.Output.t()) :: :ok | {:error, request_error()}
@spec put_output_to(term(), Kino.Output.t()) :: :ok | request_error()
def put_output_to(client_id, output) do
with {:ok, reply} <- io_request({:livebook_put_output_to, client_id, output}), do: reply
end
Expand All @@ -44,10 +44,10 @@ defmodule Kino.Bridge do
Sends the given output as intermediate evaluation result directly
to all connected client.
"""
@spec put_output_to_clients(Kino.Output.t()) :: :ok | {:error, request_error()}
@spec put_output_to_clients(Kino.Output.t()) :: :ok | request_error()
def put_output_to_clients(output) do
io_request_result =
with {:error, :unsupported} <-
with {:request_error, :unsupported} <-
io_request({:livebook_put_output_to_clients, output}),
# Livebook v0.8.0 doesn't support direct clients output,
# so we fallback to a regular one
Expand All @@ -62,7 +62,7 @@ defmodule Kino.Bridge do
Note that the input must be known to Livebook, otherwise
an error is returned.
"""
@spec get_input_value(String.t()) :: {:ok, term()} | {:error, request_error() | :not_found}
@spec get_input_value(String.t()) :: {:ok, term()} | {:error, :not_found} | request_error()
def get_input_value(input_id) do
with {:ok, reply} <- io_request({:livebook_get_input_value, input_id}), do: reply
end
Expand All @@ -71,7 +71,7 @@ defmodule Kino.Bridge do
Requests the file path for the given file id.
"""
@spec get_file_path({:file, String.t()}) ::
{:ok, term()} | {:error, request_error() | :not_found}
{:ok, term()} | {:error, :not_found} | request_error()
def get_file_path(file_ref) do
with {:ok, reply} <- io_request({:livebook_get_file_path, file_ref}), do: reply
end
Expand All @@ -80,7 +80,7 @@ defmodule Kino.Bridge do
Requests the file path for notebook file with the given name.
"""
@spec get_file_entry_path(String.t()) ::
{:ok, term()} | {:error, request_error() | :forbidden | String.t()}
{:ok, term()} | {:error, :forbidden} | {:error, String.t()} | request_error()
def get_file_entry_path(name) do
with {:ok, reply} <- io_request({:livebook_get_file_entry_path, name}), do: reply
end
Expand All @@ -89,7 +89,7 @@ defmodule Kino.Bridge do
Requests the file spec for notebook file with the given name.
"""
@spec get_file_entry_spec(String.t()) ::
{:ok, term()} | {:error, request_error() | :forbidden | String.t()}
{:ok, term()} | {:error, :forbidden} | {:error, String.t()} | request_error()
def get_file_entry_spec(name) do
with {:ok, reply} <- io_request({:livebook_get_file_entry_spec, name}), do: reply
end
Expand All @@ -103,7 +103,7 @@ defmodule Kino.Bridge do

See `monitor_object/3` to add a monitoring.
"""
@spec reference_object(term(), pid()) :: :ok | {:error, request_error()}
@spec reference_object(term(), pid()) :: :ok | request_error()
def reference_object(object, pid) do
with {:ok, reply} <- io_request({:livebook_reference_object, object, pid}), do: reply
end
Expand All @@ -129,12 +129,12 @@ defmodule Kino.Bridge do

"""
@spec monitor_object(term(), Process.dest(), payload :: term(), keyword()) ::
:ok | {:error, request_error()}
:ok | request_error()
def monitor_object(object, destination, payload, opts \\ []) do
ack? = Keyword.get(opts, :ack?, false)

io_request_result =
with {:error, :unsupported} <-
with {:request_error, :unsupported} <-
io_request({:livebook_monitor_object, object, destination, payload, ack?}),
# Used until Livebook v0.7
do: io_request({:livebook_monitor_object, object, destination, payload})
Expand All @@ -154,7 +154,7 @@ defmodule Kino.Bridge do
@doc """
Broadcasts the given message in Livebook to interested parties.
"""
@spec broadcast(String.t(), String.t(), term()) :: :ok | {:error, request_error()}
@spec broadcast(String.t(), String.t(), term()) :: :ok | request_error()
def broadcast(topic, subtopic, message) do
with {:ok, reply} <- io_request(:livebook_get_broadcast_target),
{:ok, pid} <- reply do
Expand Down Expand Up @@ -190,22 +190,22 @@ defmodule Kino.Bridge do
def get_evaluation_file() do
case io_request(:livebook_get_evaluation_file) do
{:ok, file} -> file
{:error, _} -> "nofile"
{:request_error, _} -> "nofile"
end
end

@doc """
Returns information about the running app.
"""
@spec get_app_info() :: {:ok, map()} | {:error, request_error()}
@spec get_app_info() :: {:ok, map()} | request_error()
def get_app_info() do
with {:ok, reply} <- io_request(:livebook_get_app_info), do: reply
end

@doc """
Returns a temporary directory tied to the current runtime.
"""
@spec get_tmp_dir() :: {:ok, String.t()} | {:error, request_error() | :not_available}
@spec get_tmp_dir() :: {:ok, String.t()} | {:error, :not_available} | request_error()
def get_tmp_dir() do
with {:ok, reply} <- io_request(:livebook_get_tmp_dir), do: reply
end
Expand All @@ -221,7 +221,7 @@ defmodule Kino.Bridge do

Returns a list of client ids that are already joined.
"""
@spec monitor_clients(pid()) :: {:ok, list(String.t())} | {:error, request_error()}
@spec monitor_clients(pid()) :: {:ok, list(String.t())} | request_error()
def monitor_clients(pid) do
with {:ok, reply} <- io_request({:livebook_monitor_clients, pid}), do: reply
end
Expand All @@ -234,7 +234,8 @@ defmodule Kino.Bridge do
"""
@spec get_user_info(String.t()) ::
{:ok, Kino.Hub.user_info()}
| {:error, :not_available | :not_found | request_error()}
| {:error, :not_available | :not_found}
| request_error()
def get_user_info(client_id) do
with {:ok, reply} <- io_request({:livebook_get_user_info, client_id}), do: reply
end
Expand All @@ -257,10 +258,11 @@ defmodule Kino.Bridge do

result =
receive do
{:io_reply, ^ref, {:error, {:request, _}}} -> {:error, :unsupported}
{:io_reply, ^ref, {:error, :request}} -> {:error, :unsupported}
{:io_reply, ^ref, {:error, {:request, _}}} -> {:request_error, :unsupported}
{:io_reply, ^ref, {:error, :request}} -> {:request_error, :unsupported}
{:io_reply, ^ref, {:error, :terminated}} -> {:request_error, :terminated}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this we don't actually need to change anything on Livebook side.

In retrospect, we should probably have a nested ok/error tuple to more clearly distinguish communication errors from expected errors, but it's fine.

{:io_reply, ^ref, reply} -> {:ok, reply}
{:DOWN, ^ref, :process, _object, _reason} -> {:error, :terminated}
{:DOWN, ^ref, :process, _object, _reason} -> {:request_error, :terminated}
end

Process.demonitor(ref, [:flush])
Expand Down
8 changes: 4 additions & 4 deletions lib/kino/fs.ex
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ defmodule Kino.FS do
{:error, :forbidden} ->
raise ForbiddenError, name: name

{:error, message} when is_binary(message) ->
{:error, message} ->
raise message

{:error, reason} when is_atom(reason) ->
{:request_error, reason} ->
raise "failed to access file path, reason: #{inspect(reason)}"
end
end
Expand All @@ -65,10 +65,10 @@ defmodule Kino.FS do
{:error, :forbidden} ->
raise ForbiddenError, name: name

{:error, message} when is_binary(message) ->
{:error, message} ->
raise message

{:error, reason} when is_atom(reason) ->
{:request_error, reason} ->
raise "failed to access file spec, reason: #{inspect(reason)}"
end
end
Expand Down
11 changes: 7 additions & 4 deletions lib/kino/hub.ex
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ defmodule Kino.Hub do
@spec app_info() :: app_info()
def app_info() do
case Kino.Bridge.get_app_info() do
{:ok, app_info} -> app_info
{:error, _} -> %{type: :none}
{:ok, app_info} ->
app_info

{:request_error, reason} ->
raise "failed to access app info, reason: #{inspect(reason)}"
end
end

Expand All @@ -46,10 +49,10 @@ defmodule Kino.Hub do
{:ok, user_info} ->
{:ok, user_info}

{:error, reason} when reason in [:not_found, :not_available] ->
{:error, reason} ->
{:error, reason}

{:error, reason} ->
{:request_error, reason} ->
raise "failed to access user info, reason: #{inspect(reason)}"
end
end
Expand Down
8 changes: 6 additions & 2 deletions lib/kino/input.ex
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,11 @@ defmodule Kino.Input do
{:ok, value} ->
value

{:error, reason} ->
{:error, :not_found} ->
raise "failed to read input value, input not found." <>
" Make sure to render the input before reading its value"

{:request_error, reason} ->
raise "failed to read input value, reason: #{inspect(reason)}"
end
end
Expand All @@ -756,7 +760,7 @@ defmodule Kino.Input do
{:ok, path} ->
path

{:error, _reason} ->
_ ->
# Return a non-existing path for consistency
Path.join([System.tmp_dir!(), "nonexistent", file_id])
end
Expand Down
Loading