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

Added automatic cell evaluation #637

Merged
merged 11 commits into from
Oct 26, 2021
Merged

Added automatic cell evaluation #637

merged 11 commits into from
Oct 26, 2021

Conversation

gpopides
Copy link
Contributor

this refs #614
hope this time i got it right.

notes:
i wanted to enqueue the cell evaluation using push_event/3 but it didn't work. My phoenix experience is not big, especially live view. If there is a way to do it, please let me know.

@@ -1,6 +1,22 @@
defmodule LivebookWeb.SessionLive.CellComponent do
use LivebookWeb, :live_component

def update(assigns, socket) do
if should_enqueue_stale_component?(assigns.cell_view) do
Copy link
Member

@jonatanklosko jonatanklosko Oct 24, 2021

Choose a reason for hiding this comment

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

As I mentioned this is the kind of core logic that we handle on "data level", so it shouldn't be visible on LiveView side :)

If you look at Session.Data, there's apply_operation that takes data (essentially the session state) and an operation, it updates the data and handles all the implications of the operation. For example when the user clicks "Evaluate" we end up applying {:queue_cell_evaluation, client_pid, cell_id} to the data, which adds the cell to the queue, and then if nothing is evaluating we immediately mark it as evaluating. LiveView only gets the new state and renders accordingly.

Changes to Data.apply_operation need to consider all operations/scenarios, but I think in this case it may be a simple change. Whenever an operation affects cells validity (evaluated/stale/aborted/fresh), we call this function:

# Computes cell snapshots and updates validity based on the new values.
defp compute_snapshots_and_validity(data_actions) do
data_actions
|> compute_snapshots()
|> update_validity()
end

In update_validity a cell may transition into the :stale state. So I think what we need is to pipe this into another function like maybe_queue_reevaluating_cells, which looks up all automatically reevaluating cells that are :stale and queues these cells (similarly to this).

Feel free to poke around and let me know if you have any questions :)

@jonatanklosko
Copy link
Member

There's a number of things we also need to do (mostly a note to myself :)):

  • add relevant tests for the data transitions
  • persist the reevaluation setting as we do with formatting
  • remove reactive input

@gpopides
Copy link
Contributor Author

Hey, i made the changes and have some questions about the second part.

  • I could not find the place that we persist the formatting place so i can do the same for reevaluation setting.
  • By removing the reactive input, does it mean removing the phx-click from the input when the input has the reevaluation setting on?

Thanks

@jonatanklosko
Copy link
Member

Sweet!

I could not find the place that we persist the formatting place so i can do the same for reevaluation setting.

We handle this in Livebook.LiveMarkdown.Export and Livebook.LiveMarkdown.Import (parsing and storing the notebook content respectively).

By removing the reactive input, does it mean removing the phx-click from the input when the input has the reevaluation setting on?

Nah, with the automatically reevaluating cells there is no need for reactive inputs at all, so we will just remove that feature. There's a number of places involved, so it will be easier if I do it myself :)

@jonatanklosko
Copy link
Member

@gpopides thanks! I've just pushed some more tests and adjusted the UI a bit :)

@jonatanklosko
Copy link
Member

Here's how it looks:

autoeval.mp4

@josevalim let me know what you think.

I will remove reactive inputs in a separate PR :)

@jonatanklosko jonatanklosko merged commit 5c0267b into livebook-dev:main Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants