Support dependent bound variables for SliderServer

[lungben]

My first try to implement #1695

This should allow PlutoSliderServer to work with dependent bind values, i.e. a UI element dependent on another UI element.

ToDo:

• Add unit tests
• Make sure that this change does not break anything ;-)

[github-actions]

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="/~https://github.com/lungben/Pluto.jl", rev="dependend_bound_variables")
julia> using Pluto

[Pangoraw]

You need to apply the value transformation somewhere:

expr = :($bond_var = Main.PlutoRunner.transform_bond_value($(QuoteNode(bond_var)), $bond_val))

This API allows widgets to return julia native values by converting them depending on the widget (the second argument to @bind). See JuliaPluto/PlutoUI.jl#148 for more details.

Note that this suggestion causes a problem because the transformation will happen with the new widget:

# Cell 1
@bind x Slider(1:10)

# Cell 2
@bind y Slider(0:x:100)

I think in this context, the widgets send as indices in the respective slider ranges x_idx = 2 and y_idx = 2, and after transformation x = (1:10)[x_idx] = 2, y = (0:x:100)[y_idx] = 2 but it should be y = (0:old_x:100)[y_idx] = 1.

So the transformation has already been done before the cell is run but its value has been overriden with the new default widget value and we either need to restore that value or transform again:

Pluto.jl/src/evaluation/RunBonds.jl

Line 44 in 15a94f4

WorkspaceManager.eval_in_workspace((session, notebook), :($(bound_sym) = Main.PlutoRunner.transform_bond_value($(QuoteNode(bound_sym)), $(new_value))))

[lungben]

Thanks for pointing out the value transformations!

It is a general issue for dependent sliders that y may contain an invalid value after changing x, either by being not allowed anymore (out of range for sliders, invalid selections for Select, etc.) or by the value transform issue you described.
However, after the user updates the element bound to y it should have a valid value again. Thus, maybe this is an acceptable behavior?

As far as I know, PlutoSliderServer does not distinguish in its request which variable has been explicitly changed by the user and which is only co-dependent. Thus, we cannot check if the latter still has a valid value with the updated UI element (and reset it to the default if not). Or do I miss something?

[Pangoraw]

For the valid value problem, there is plan (see JuliaPluto/PlutoSliderServer.jl#39) to integrate AbstractPlutoDingetjes.validate_value inside PlutoSliderServer and prevent invalid value altogether (from calling the API directly).

You are right that the request always contains both bonds values, so actually the transformation should be done with respect to the new widget value (not previous as i said). Therefore, you should be okay by just transforming after the cell was run:

		        for bond_var ∈ redefined_external_bound_variables
				# set the redefined bound variables to their original value from the request
				bond_val = externally_updated_variables[bond_var]
-				expr = :($bond_var = $bond_val)
+				expr = :($bond_var = Main.PlutoRunner.transform_value($(QuoteNode(bond_var)), $bond_val))
				WorkspaceManager.eval_in_workspace((session, notebook), expr)
			end

[lungben]

In general this functionality seems to be working correctly also for larger / more complex notebooks.

If the first bound value (here max_x) is changed so that the choice of the 2nd bound value (here x) is no longer valid, an ArgumentError is currently raised. This can be fixed by updating the 2nd bound value in the GUI.

Maybe we should catch this error and show a more descriptive error to the user?

[lungben]

Wrapping Main.PlutoRunner.transform_bond_value() in a try-catch block did not work for me, neither locally in Run.jl nor if it is part of the expr to be executed in the notebook process.

Edit: I think a more descriptive error message is rather "nice-to-have" and should not prevent merging this PR. The core in this PR should anyhow only be executed in SliderServer requests, where this PR brings a significant improvement.

[fonsp]

Awesome! Very cool that this could be done with such a small PR!

We should add tests for this into Pluto.jl itself, right now there are only on PSS. It would be nice if the tests do not depend on PlutoUI, so you can do:

begin
    struct TransformSlider
        range::AbstractRange
    end
    
    Base.show(io::IO, m::MIME"text/html", os::TransformSlider) = write(io, "<input type=range value=$(minimum(os.range)) min=$(minimum(os.range)) max=$(maximum(os.range))>")
    
    Bonds.initial_value(os::TransformSlider) = Bonds.transform_value(os, minimum(os.range))
    Bonds.possible_values(os::TransformSlider) = os.range
    Bonds.transform_value(os::TransformSlider, from_js) = from_js * 2
end

which you can use like so:

@bind x TransformSlider(1:10)

@bind y TransformSlider(1:x)

(copy-paste into Pluto to see what it does)

See /~https://github.com/fonsp/Pluto.jl/blob/main/test/Bonds.jl for more examples

[lungben]

For testing this PR, plain html should be sufficient:

# ╔═╡ 600c3172-e83d-4f2c-a239-d130168d9fa0
@bind x html"<input type=range min=1 max=10>" 

# ╔═╡ 326db4f1-f996-49dc-a270-f6ae52e64e88
@bind y HTML("<input type=range min=1 max=$(x)>")

# ╔═╡ 35eec394-e841-491c-8bc8-7ac2de327736
x

# ╔═╡ 20f827a7-27f1-408f-85d1-4b87f45d3867
y

[lungben]

From my side, this PR is now ready for merging.

• The failure of the tests on MacOS is not related to my changes, as far as I can see (3 tests from the set "Reload from file" are failing, and only on MacOS)
• Unit Tests for the change in this PR have been added
• The behavior of dependent bound variables in Pluto itself is the same as without this PR as far as I can see
• With PlutoSliderServer, dependent bound variables now work correctly

[fonsp]

Thanks! Can you add a test with a transformed value as in #2014 (comment) ? Since this PR had some discussion and changes related to it, it makes sense to also add a test, right?

It should be easy to add, by adding three new cells to the test notebook, (defining a and b instead of x and y).

[lungben]

Done

Interestingly, the TransformSlider requires setting of 🍭.options.evaluation.workspace_use_distributed = true in the tests, otherwise the bound value is not transformed. But I think this is unrelated to my PR because the same setting is also in the testset "AbstractPlutoDingetjes.jl".

[fonsp]

Weird! But that's a good find! I made #2056 to track that separately from this PR

[fonsp]

Hey! I noticed that we might be able to clean this up a bit: we now have the old mechanism for setting bond values: custom_deletion_hook, and the new mechanism, externally_updated_variables.

In theory, externally_updated_variables should be a generalization of the trick in custom_deletion_hook, so we should be able to just delete that hook in set_bond_values_reactive. I didn't think this through 100% yet, but @lungben or @Pangoraw could you look into this? This new mechanism should also be very similar to the way #1052 injects values. If it's easy to implement, let's do it in this PR.

[lungben]

As far as I can see, both custom_deletion_hook and the new functionality loop over symbol/value pairs and eval them in the workspace with WorkspaceManager.eval_in_workspace((session, notebook), :($(bound_sym) = Main.PlutoRunner.transform_bond_value($(QuoteNode(bound_sym)), $(new_value)))).
We could put this functionality into a utility function.

custom_deletion_hook in addition has WorkspaceManager.move_vars(), which is not used in the new functionality of this PR.

[lungben]

@fonsp can this PR be merged?

[fonsp]

Thanks @lungben , and apologies for the delay!

I initially thought that this PR would be difficult to review, because I was worried about how it interplays with funky combinations of bonds. But in fact, this PR does not change behaviour when only a single bond is being set, because there is only one cell defining that bond, and that one is excluded from the reactive run (because we use the cells that reference a set bond as the root of the reactive run).

There is one exception: the following notebook behaves differently:

begin
    if @isdefined(x)
        # cool
    end
    
    bond = @bind x html"<input type=range>"
    
    x = 999
    
    bond
end

x

because the first cell both defines and references x, so it is a cell that references the bound symbol, and it will be part of the reactive run. This mechanism can be used for single-cell interactions like this one.

As it happens, the example above gets stuck in an infinite loop on the current Pluto (long story), and after this PR, it doesn't get stuck, and the x = 999 line is "ignored".

But! The single-cell interaction example that I mentioned above still works! So I take that as a sign that the behaviour is generally as expected, and can even contribute to preventing loops.

TLDR: 👍👍