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

Add append mode for reactive generator output #5129

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 16, 2023

Allows generator output to append to output rather than simply replacing it:

import random
import time
import panel as pn

pn.extension(sizing_mode="stretch_width")

def model():
    time.sleep(1)
    return random.randint(0, 100)

def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        yield f"Result {i}: {model()}"

run_input = pn.widgets.Button(name="Run model")
pn.Column(
    run_input,
    pn.panel(pn.bind(results, run_input), loading_indicator=True, generator_mode='append'),
).servable()

gen_append

@philippjfr philippjfr mentioned this pull request Jun 16, 2023
17 tasks
@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 16, 2023

One issue with your example. The appended results are disabled until all results are ready.

If for example my generative AI model streams multiple intermediate video files, audio files, tables or similar back, I want to be able to interact with them as soon as they are ready. Not wait several minutes until the final result is returned 😄

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #5129 (2b5293a) into main (b0ba5e3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5129      +/-   ##
==========================================
+ Coverage   83.78%   83.80%   +0.02%     
==========================================
  Files         274      274              
  Lines       39391    39449      +58     
==========================================
+ Hits        33003    33061      +58     
  Misses       6388     6388              
Flag Coverage Δ
ui-tests 40.42% <8.19%> (-0.07%) ⬇️
unitexamples-tests 73.84% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
panel/pane/base.py 85.82% <100.00%> (ø)
panel/param.py 88.62% <100.00%> (+0.22%) ⬆️
panel/tests/test_param.py 99.67% <100.00%> (+0.01%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 16, 2023

Please also consider if generator_mode should be inside pn.bind/ pn.depends instead? This would save users the friction of having to put it in a pn.panel.

(you could probably say the same for loading_indicator).

@philippjfr
Copy link
Member Author

import random
import time
import panel as pn

pn.extension(sizing_mode="stretch_width")

def model():
    time.sleep(1)
    return random.randint(0, 100)

def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        result = pn.panel('Running model...', loading=True)
        yield result
        result.param.update(object=f"Result {i}: {model()}", loading=False)

run_input = pn.widgets.Button(name="Run model")
pn.Column(
    run_input,
    pn.panel(pn.bind(results, run_input), generator_mode='append'),
).servable()

gen_load

@MarcSkovMadsen
Copy link
Collaborator

A Column is probably the most used layout. But consider if users would like to use other layouts like Row or some grid. Maybe this could be solved by a `generator_layout´ that accepts some list like interface.

@philippjfr
Copy link
Member Author

Please also consider if generator_mode should be inside pn.bind/ pn.depends instead? This would save users the friction of having to put it in a pn.panel.

I've been thinking about this a lot but haven't yet come to any good solutions. In hvplot.bind() we have allowed using hvplot.bind(...).interactive(), wonder if we should consider something like that here or whether there is some other solution. I'm not super excited about adding special keywords to the pn.bind signature since it's meant to be a generic thing like functools.partial and adding reserved keyword feels dirty.

@philippjfr
Copy link
Member Author

I do find pn.panel(pn.bind(...), ...) really ugly so I'm very motivated to find something better.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 16, 2023

result = pn.panel('Running model...', loading=True)
yield result
result.param.update(object=f"Result {i}: {model()}", loading=False)

Please consider whether this can be simplified to the first two lines. For example you can temporarily append and then pop a loading spinner. Or Spacer with a loading_indicator.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 16, 2023

I do find pn.panel(pn.bind(...), ...) really ugly so I'm very motivated to find something better.

Maybe

pn.bind(...).panel(loading_indicator=True)
pn.bind(...).configure(loading_indicator=True)
pn.bind(...).update(loading_indicator=True)
pn.bind(...).display_options(loading_indicator=True)

@philippjfr
Copy link
Member Author

Please consider whether this can be simplified to two lines. For example you can temporarily append and the pop a loading spinner. Or Spacer with a loading_indicator.

That approach just falls naturally out of the functionality and is pretty powerful. Anything else like you're suggesting would require injecting arguments into the generator or set up some special control flow signals, which seems overly complex. It would be neat if there was a clean way to temporarily switch between append and replace modes, so you can yield something that is appended and then subsequently yield a replacement.

@philippjfr
Copy link
Member Author

Only thing I can imagine is some sentinel value on the replacement:

def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        yield pn.panel('Running model...', loading=True)
        yield pn.panel(f"Result {i}: {model()}", tags=['replace'])

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jun 16, 2023

Only thing I can imagine is some sentinel value on the replacement:

def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        yield pn.panel('Running model...', loading=True)
        yield pn.panel(f"Result {i}: {model()}", tags=['replace'])

At a first glance it does not "feel" right. Its like an "agreed", "convention" that you need to remember. Then I probably prefer your 3 lines and then clear documentation.

@philippjfr
Copy link
Member Author

Indeed, the tags approach definitely feels too much like a very ad-hoc convention but it was really mostly to demonstrate the idea. That said I can't think of anything that's actually better.

@philippjfr
Copy link
Member Author

philippjfr commented Jun 16, 2023

Only other suggestion I have and it's definitely in weird convention territory again:

def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        yield pn.panel('Running model...', loading=True), ...
        yield f"Result {i}: {model()}"

i.e. the ... Ellipsis indicates that the output is just a placeholder and there's a replacement to come.

@hoxbro
Copy link
Member

hoxbro commented Jun 16, 2023

I don't think there is a great solutions. The convention seems non-ideal, I don't like adding the tag, and the pn.bind(...).panel in my mind is a small rewrite and not a big enough change.

I would not say the solution I have thought of is great either. But the thinking is making custom bind functions where you set the panel arguments, so you define them once and reuse that custom bind as needed.

import random
import time

import panel as pn

pn.extension(sizing_mode="stretch_width")


def model():
    time.sleep(1)
    return random.randint(0, 100)


def results(running):
    if not running:
        yield "The model has not run yet"
        return

    for i in range(0, 10):
        yield f"Result {i}: {model()}"


# Only reason for two buttons is so they don't block each other
run_input1 = pn.widgets.Button(name="Run model")
run_input2 = pn.widgets.Button(name="Run model")

custom_bind = pn.bind.create(loading_indicator=True, generator_mode="append")

pn.Column(
    pn.Row(run_input1, run_input2),
    pn.Row(
        custom_bind(results, run_input1),
        pn.bind(results, run_input2),
    ),
).servable()
screenrecord-2023-06-16_20.43.04.mp4
The hack to get it to work 🙃
import panel as pn
from panel.pane import PaneBase


def create(**pn_kwargs):
    def func(*args, **kwargs):
        bound = pn.bind(*args, **kwargs)
        bound.__panel__ = lambda: PaneBase.get_pane_type(bound, **pn_kwargs)(
            bound, **pn_kwargs
        )
        return bound

    return func


pn.bind.create = create

@jbednar
Copy link
Member

jbednar commented Jun 16, 2023

I'm having trouble really wrapping my head around this enough to have a strong opinion, but it seems like pn.bind.create(loading_indicator=True, generator_mode="append") is precisely what a param.ParameterizedFunction .instance() call does, i.e. allows you to configure a "function" with parameter settings that then gets called as normal. I agree with Philipp's reluctance to pollute the pn.bind argument namespace, and maybe just .instance vs. a regular call would do it?

@philippjfr
Copy link
Member Author

Merging this, adding an API like pn.bind.create or pn.bind(...).panel() really is a separate discussion.

@philippjfr philippjfr merged commit 6ce21fc into main Jun 29, 2023
@philippjfr philippjfr deleted the generator_mode branch June 29, 2023 11:02
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.

4 participants