Skip to content

Re-run errored cells after soft_definitions become available #3513

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

Open
PatrickHaecker wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Re-run errored cells after soft_definitions become available #3513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 19 additions & 13 deletions src/evaluation/Run.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,28 +215,34 @@ function run_reactive_core!(
end

implicit_usings = collect_implicit_usings(new_topology, cell)
if !PlutoDependencyExplorer.is_resolved(new_topology) && can_help_resolve_cells(new_topology, cell)
notebook.topology = new_new_topology = resolve_topology(session, notebook, new_topology, old_workspace_name)
needs_resolve = !PlutoDependencyExplorer.is_resolved(new_topology) && can_help_resolve_cells(new_topology, cell)
has_implicit_usings = !isempty(implicit_usings)

if !isempty(implicit_usings)
if needs_resolve || has_implicit_usings
resolved_topology = needs_resolve ? resolve_topology(session, notebook, new_topology, old_workspace_name) : new_topology

if has_implicit_usings
new_soft_definitions = WorkspaceManager.collect_soft_definitions((session, notebook), implicit_usings)
notebook.topology = new_new_topology = with_new_soft_definitions(new_new_topology, cell, new_soft_definitions)
resolved_topology = with_new_soft_definitions(resolved_topology, cell, new_soft_definitions)
end

# update cache and save notebook because the dependencies might have changed after expanding macros
update_dependency_cache!(notebook)
save && save_notebook(session, notebook)

return run_reactive_core!(session, notebook, new_topology, new_new_topology, to_run; save, deletion_hook, user_requested_run, already_run = to_run[1:i])
elseif !isempty(implicit_usings)
new_soft_definitions = WorkspaceManager.collect_soft_definitions((session, notebook), implicit_usings)
notebook.topology = new_new_topology = with_new_soft_definitions(new_topology, cell, new_soft_definitions)
notebook.topology = resolved_topology

# update cache and save notebook because the dependencies might have changed after expanding macros
update_dependency_cache!(notebook)
save && save_notebook(session, notebook)

return run_reactive_core!(session, notebook, new_topology, new_new_topology, to_run; save, deletion_hook, user_requested_run, already_run = to_run[1:i])
# Cells that are now newly downstream via soft_definitions may have already run (and errored)
# before the using cell. Remove errored ones from already_run so they get re-executed.
already_run_for_restart = if has_implicit_usings
new_soft_defs = resolved_topology.nodes[cell].soft_definitions
newly_downstream = Set{Cell}(PlutoDependencyExplorer.where_referenced(resolved_topology, new_soft_defs))
filter(c -> !(c ∈ newly_downstream && c.errored), to_run[1:i])
else
to_run[1:i]
end

return run_reactive_core!(session, notebook, new_topology, resolved_topology, to_run; save, deletion_hook, user_requested_run, already_run = already_run_for_restart)
end
end

Expand Down
21 changes: 21 additions & 0 deletions test/React.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,27 @@ import Pluto.Configuration: Options, EvaluationOptions
WorkspaceManager.unmake_workspace((🍭, notebook); verbose=false)
end

@testset "Soft definitions re-run errored cells" begin
# When a cell that references a soft-exported symbol (e.g. `January` from Dates) also
# defines a variable used by the `using` cell, the dependency forces it to run BEFORE
# the using cell. It errors with UndefVarError because the soft symbol isn't available yet.
# After the using cell runs and soft_definitions become available, the errored upstream
# cell should be removed from `already_run` and re-executed.
notebook = Notebook([
Cell("x = January"),
Cell("""begin
using Dates
y = x
end"""),
Comment on lines +340 to +344

@Pangoraw Pangoraw Apr 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you have a more complete reproducer of that notebook that fails in 1.12 and not in 1.11? while you can run this notebook because of the cyclic definition bailing on these "using" definitions, it is essentially a cycle. And there is for example no way to store this notebook in a julia script without having an error

@PatrickHaecker PatrickHaecker Apr 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for having a look at the PR, @Pangoraw !

The document is proprietary and quite large. I was really glad that I was finally able to fix it, because the error depends on the evaluation order, which is not visible in the document itself. I lack the time to trace this for the real document.

I think the Pluto model inherently needs to be able to cover dependencies which could by cyclic. If you have

@x b
@y a

both macros could define only a, only b, a and b or neither one. So it's not clear which macro to evaluate first and this probably just needs to be tried, but it needs to be done if there was any progress in evaluation. Of course, as soon as there is no progress being made, another evaluation cycle of all failed expressions does not make any sense anymore and should be stopped.

I am not sure whether all these cases are fully covered, but I am pretty sure that the cyclic nature is the price of the Pluto model that cell order shall not matter in combination with a dynamic language. I don't think we should require that all Pluto scripts can be executed as a script without error. If this were the case, no dependency analysis would be needed in Pluto, because everything would need to be executed in order anyway.

This PR definitely fixes some cases. At least according to the test suite, it does not break anything. Do you have an example of what it breaks? Otherwise it seems to be a clear improvement.

@PatrickHaecker PatrickHaecker Apr 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

With some bisecting, I managed to come up with an example in finite time. I hope this is more complete without being excessively large.
real-world-example.zip

])

update_run!(🍭, notebook, notebook.cells)

@test notebook.cells[1] |> noerror # January should resolve after Dates provides soft_definitions

WorkspaceManager.unmake_workspace((🍭, notebook); verbose=false)
end

@testset "More challenging reactivity of extended function" begin
notebook = Notebook(Cell.([
"Base.inv(s::String) = s",
Expand Down
Loading