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

Behavior of display() targets #769

Closed
antocuni opened this issue Sep 14, 2022 · 17 comments
Closed

Behavior of display() targets #769

antocuni opened this issue Sep 14, 2022 · 17 comments
Labels
backlog issue has been triaged but has not been earmarked for any upcoming release sprint issue has been pulled into current sprint and is actively being worked on

Comments

@antocuni
Copy link
Contributor

Related issues/PRs: #622 #635, #749

After lot of discussion, I think we agreed about what we want, which is:

<div id="mydiv"></div>
...
<py-script>
    print('this goes to stdout')
    display('this goes to the DOM')
    display('this goes to the DOM with an explicit target', target='mydiv')
</py-script>

Sidenote 1: here I'm using strings but display() will be able to render rich objects as well, e.g. images, tables, charts, etc. -- but this is not pertinent to this particular discussion.

Sidenote 2: it's unclear whether display('somestring') should wrap the string into an HTML tag or not. For these examples, I'm wrapping it into a <div> but please don't comment on that: again, this belongs to a different discussion.

The general consensus is that the default target is "here", i.e. we want to insert a sibling of the <py-script> tag. So the code above will produce the following DOM:

<div id="mydiv">
    <div>this goes to the DOM with an explicit target</div>
</div>
...
<py-script>...</py-script>
<div id="automatically-generated">
   <div>this goes to the DOM</div>
</div>

I think that in this example the semantics is clear, straightforward and intuitive. However, things become less clear in more complicated examples. I'll try to add names to each example so that it's easier to refer to them in the further discussion.


Example 1: display-inside-def

<py-script id="py1">
def say_hello():
    display('hello')
</py-script>
<div>world</div>
<py-script id="py2">
say_hello()
</py-script>

What is the implicit target of display here? There are at least two options:

  • lexical scoping: the target is determined by <py-script> tag in which the code is written. The target for say_hello() is py1 and the example above displays hello world
  • dynamic scoping: we have the notion of "current target" which varies during the execution of the page. During the definition of say_hello the default target is py1, but when we call it, the default target is py2. Thus, the example above displays world hello.

The concept of "current target" might seem tempting at first, but I think it introduces a lot of complexity and corner cases, as shown by the next examples.


Example 2: display-from-event-handlers

<py-script id=`py1`>
def say_hello():
    display('hello')
</py-script>
<div>world</div>
<button py-onclick="say_hello()">Click me</button>

I think that the concept of "current target" is ill-defined in a case like this. You could say that it's button, but it seems very counter-intuitive to me to display hello next to Click me.
Also, there might be events which doesn't have a clear tag where they are originated from.


Example 3: display-and-async
Let's forget about events for a while and let's stay focused on <py-script> only. Even in this case, there are cases in which the concept of "current target" is vague, ill-defined or simply unexpected. I can imagine to implement it like this:

for(s of list_of_pyscript_tags) {
    set_global_current_target(s);
    s.evaluate();
}

But then, consider this example:

<py-script id="A">
for i in range(3):
    display(f'A{i}')
    asyncio.sleep(0.1)
</py-script>
<div>hello</div>
<py-script id="B">
for i in range(3):
    display(f'B{i}')
    asyncio.sleep(0.1)
</py-script>

I would expect this to display A0 A1 A2 hello B0 B1 B2, but with the logic above it doesn't work, because we set current_target=B when the first loop is still executing. So, my naive implementation above would display A0 hello B0 A1 B2 A2 B2 which is clearly wrong.
The only way to make the "current target" working is to keep track of it at each async swtich. I don't even know if this is technically possible.


A middle ground?

One possible compromise could be to declare this rule:

display() without a target can be called only during the execution of <py-script> tags. Inside event handlers and async blocks, you must always provide an explicit target

This might technically work, although it has still weird corner cases. Example sync-and-async:

<py-script id="A">
for i in range(3):
    display(f'A{i}')  # this is allowrd
</py-script>
<div>hello</div>
<py-script id="B">
for i in range(3):
    display(f'B{i}')  # this is forbidden because of the async sleep?
    asyncio.sleep(0.1)
</py-script>

It sounds very weird that the A case is allowed and the B case is not.


Implementation problems

From the wall of text above, it should be clear that I am more favorable to use "lexical scoping" and declare that the default target of dsiplay() depends on the tag where the code is defined, not where it's run.
However, this poses another problem: I don't know how to implement it :)

The naive approach is create a different display for each tag, something like this:

def global_display(obj, target):
    ...

def make_display_with_a_default_target(target):
    def local_display(obj, target=target):
        return global_display(obj, target)
    return local_display

And then, for each <py-script> tag we create a local_display and inject it into its namespace.
The biggest problem is that it mixes very badly with the fact that all <py-script> tags share the same global namespace. Example same-global-namespace:

<py-script>
a = 42
</py-script>
<py-script>
print(a)
</py-script>

In order to make this working, the only reasonable approach is to ensure that all the tags share the same __globals__. But then it means that also display must be unique, i.e.:

<py-script>
a = display
</py-script>
<py-script>
assert a is display
</py-script>

But if display must be identical across tags, we can no longer have an unique local_display for each of them.
I don't really know how to solve this cleanly


Horror story: a non-clean solution

I'm writing it here just for the sake of completeness, but please don't even consider to use it :).
For each <py-script> tag, we could ast.parse() the code, detect the usage of a global display name and substitute it with something else which is unique per each block.


Always require explicit targets

One possibility is to decide that implicit targets are too hard and that we always require an explicit one; e.g.:

<py-script>
display('hello', target=parent)
# or: parent.display('hello')
</py-script>

Where parent is automagically "the current tag". But this has the very same problems that I explained above in the section "Implementation problems": we cannot have a per-tag parent if we share the same __globals__.


Kill the global namespace

Another "obvious" solution is to declare that each <py-script> tag has its own local namespace, and you have to be explicit to access their properties

<py-script id="py1">
a = 42
</py-script>
<py-script>
print(document.py1.a)
</py-script>

This solves all the problems explained above. I don't know if we want to go in that direction though.


Wrap up

I think this is a fundamental issue in our desired semantics. We need to consider it very seriously because a mistake here have probably big consequences on the usability of pyscript, especially if it leads to weird corner cases.
Somehow, my gut feeling is that the following three properties don't mix well together:

  • python semantics
  • implicit global namespace
  • implicit per-tag local state ("the default target")
@philippjfr
Copy link
Contributor

Thanks for the great writeup, overall, despite the complexity, I still think the dynamic scoping behavior is what I'd intuitively expect. If we decide the complexity is too much my second vote would go for being fully explicit. Everything else is, imo, a lot of mental overhead for users or simply non-intuitive.

The only way to make the "current target" working is to keep track of it at each async swtich. I don't even know if this is technically possible.

I think this should be possible using ContextVar. I've recently been dealing with a similar issue and ended up rolling my own solution by adding an asyncio task factory that allows me to keep track of task switches but the ContextVar solution, should it work, seems a lot cleaner.

@antocuni
Copy link
Contributor Author

Thanks for the great writeup, overall, despite the complexity, I still think the dynamic scoping behavior is what I'd intuitively expect

that's interesting because it's exactly what I would not expect :).
But the big question here is: what is the exact semantics which define what is the "current target"?

The only way to make the "current target" working is to keep track of it at each async swtich. I don't even know if this is technically possible.

I think this should be possible using ContextVar.

uhm, this is interesting.
I might be happy with the following semantics:

  • the "current target" is defined only when you are directly executing a <py-script>
  • the "current target" is stored in a ContextVar so that is works seamlessly for async cases
  • if you call display from e.g. an event handler, you need to specify the target explicitly

I'm still not fully convinced that there are no weird cases though.

@tedpatrick
Copy link
Contributor

It feels like display() has overlap with the Element api.

Element("#foo").write("<h1>My Grand Title</h1>")
#vs
display( "<h1>My Grand Title</h1>", "#foo" )

It also feels like the output target here should be configurable.

<py-script> configured

<py-script stderr="console.error" stdout="#mydiv">
print('hello') # to #mydiv
</py-script>
<py-script stderr="console.error" stdout="console.log">
print('hello') # to console.log
import foo&bar # import error to console.error
</py-script>

<py-config> configured

<py-config>
stderr="console.error"
stdout="console.log"
</py-config>

<py-script>
print('hello') # to console.log
</py-script>

I do not believe we need to wrap text in display(string) as TextNode is a valid DOM type and is generated purely from a string in the DOM api.

There are some important aspects to writing to the DOM and these "modes" need to be taken into account:

  • Append - Append a node into the existing element.children
  • Overwrite - write to innerHTML and overwrite everything
  • Diff/Patch - elegantly patch DOM differences and preserve end-user DOM state (form data entered, scroll positions, textnodes, element order).

See https://github.com/patrick-steele-idem/morphdom for a Diff/Patch API without Virtual DOM

@fpliger
Copy link
Contributor

fpliger commented Sep 14, 2022

Thank you @antocuni ! This is a great summary!

Thanks for the great writeup, overall, despite the complexity, I still think the dynamic scoping behavior is what I'd intuitively expect

that's interesting because it's exactly what I would not expect :).
But the big question here is: what is the exact semantics which define what is the "current target"?

I'll say dynamic scoping behavior is exactly what I'd expect as well. I will risk here and say that @pzwang vote goes there too. In general I think this is very familiar to anyone used to Notebooks. You get your output in the context of the cell that is executing code (not defining!).

In that sense, actually most examples above don't really seem an edge case (at least to me).

I do like @philippjfr idea of using ContextVar. or something along these lines where the target lives within that execution block.

I'll give you that this write you actually adds one use case that I really consider edge, and that is example 2. In that case I think there's no good default target in general because the context doesn't have enough information for a good guess. In that case I think we should implement a way for the user to pass the target in the context of "I'm passing an event handler to an action and I EXPLICITLY want to pass the target because any default isn't good enough". Either it being a decorator and some syntatic sugar..., we can figure that out but I agree, that is a edge case. (and fwiw, I think it's fair to come with "some" solution not and note it in the docs for now, while we work that out, rather than this cause for us to say "display should always be explicit". That, imho, is a bad solution.

In addition to that default, IF WE CAN, I think we should also allow users to set targets explicitly at different levels. For instance:

  1. Allow users to set a default target for all pyscript tags in a page, in the py-config
  2. Allow users to set a default target for all displaycalls within a py-script tag (i.e. <py-script target=bla>)
  3. Allow users to explicitly set a target as an argument of display (i.e. display(obj, target=bla))

@antocuni
Copy link
Contributor Author

In general I think this is very familiar to anyone used to Notebooks.

this is something which came up many times already, and I don't really understand it. From my POV notebooks cells and <py-script> tags are something completely different: one is interactive and REPL-style, the other is non-interactive. Notebooks have lots in common with <py-repl>, not <py-script>, IMHO.
That said, I think that one outcome of this conversation is that there is no behavior which is "obvious", because it's clear that people who are all with lot of experience but with different backgrounds have a different intuition of how things should behave, including @tedpatrick who comes from a JS/Web background.
Because of this, I think we should be even more cautious than usual in finding the "best" semantics for this.

I do like @philippjfr idea of using ContextVar. or something along these lines where the target lives within that execution block.

I'm still not 100% convinced that it works in all cases, especially in combination with the JS async loop. I need to think more.

I'll give you that this write you actually adds one use case that I really consider edge, and that is example 2. In that case I think there's no good default target in general because the context doesn't have enough information for a good guess

ok, it's good that we are converging somewhere :).
I think I could live with the following semantics:

  1. display(obj, target=...) is always possible
  2. display(obj) is possible only when the concept of "current target" is clearly defined
  3. display(obj) when "current target" is not defined raises an error
  4. as a first approximation, we declare that "current target" is defined only during the execution of a <py-script> or <py-repl> tag. Basically, something like this:
class BaseEvalElementWithImplicitTarger extends HTMLElement {
    async evaluate(): Promise<void> {
        magically_set_current_target(this);
        try {
            ... execute the python code ...
        } finally {
            magically_set_current_target(undefined);
        }
}
  1. for now, we declare that async py-script blocks do not define an implicit target. Later we can think more and see whether we can come up with a reasonable solution based on ContextVar

In addition to that default, IF WE CAN, I think we should also allow users to set targets explicitly at different levels. For instance:

  1. Allow users to set a default target for all pyscript tags in a page, in the py-config

+1

  1. Allow users to set a default target for all displaycalls within a py-script tag (i.e. <py-script target=bla>)

-0. It probably doesn't hurt, but what is the use case for that? I mean:

<py-script display-target="foo">
display(obj)
</py-script>

<py-script>
display(obj, target="foo")
</py-script>

The two examples seem to have the same level of complexity to me: is it worth to introduce yet-another-construct which does the same work as one that we already have? I think it adds cognitive overhead for a very little gain. For example, as soon as you introduce display-target you have to introduce a rule which says who takes the precedence: it is "obvious" that the target=... python argument should take the precedence, but as a beginner it's still a rule that you have to understand and memorize.

The only case in which it saves typing is if you have multiple calls to display(), but IMHO "saving keystrokes" is not so important to add the cognitive overhead I described above.

  1. Allow users to explicitly set a target as an argument of display (i.e. display(obj, target=bla))

+1

@marimeireles
Copy link
Member

marimeireles commented Sep 15, 2022

I think that the concept of "current target" is ill-defined in a case like this. You could say that it's button, but it seems very counter-intuitive to me to display hello next to Click me.
Also, there might be events which doesn't have a clear tag where they are originated from.

The current behavior is: we only display by default on py-script tags. Too give more color to my explanation if you run:

<py-script id='py1'>
def say_hello():
    display('hello')
</py-script>
<div>world</div>
<button id='what' py-onclick="say_hello()">Click me</button>

You'll get the py1 id, instead of the what one. The reason for that is the only time we run the display code is when we run python code and not any other time. It's on my PR, base.ts:

            <string>await runtime.run(`set_current_display_target(element="${this.id}")`);
            <string>await runtime.run(source);

That's just building on top of previous behavior... And I imagine if we want to change this we'd have to undergo deeper refactoring of the code.

If I understand correctly that's your compromise design idea?

display() without a target can be called only during the execution of tags. Inside event handlers and async blocks, you must always provide an explicit target


Example 3 would be nicely fixed by @philippjfr's idea, I guess? I'm not familiar with this var but that was Fabio's opinion yesterday.


Implementation problems

This session seems so overly complicated.

I like these:

I might be happy with the following semantics:
the "current target" is defined only when you are directly executing a
the "current target" is stored in a ContextVar so that is works seamlessly for async cases
if you call display from e.g. an event handler, you need to specify the target explicitly


Kill the global namespace

I think this is a bad solution in an user perspective.

@marimeireles
Copy link
Member

marimeireles commented Sep 15, 2022

Sorry my page wasn't updated here are some more cents.

Because of this, I think we should be even more cautious than usual in finding the "best" semantics for this.

I hear you!
I also have this feeling I'd expect things from being dynamic, but I'm biased towards Jupyter notebooks.

I'm still not 100% convinced that it works in all cases, especially in combination with the JS async loop. I need to think more.

I will run some tests trying this once we merge the current PR.

I think I could live with the following semantics:

You're saying we should put rules in place that enforce this behavior (as in implemented in the code and throwing warnings or errors? Or just somewhere in the docs as what you should do?)

Allow users to explicitly set a target as an argument of display (i.e. display(obj, target=bla))

+1

This is already implemented.

@JeffersGlass
Copy link
Member

JeffersGlass commented Sep 15, 2022

I hate to add more cooks to this great kitchen, but:

Precedence

I'm hoping to clarify the precedence of the various function-arguments and tag-attributes as they've been discussed. I agree with @antocuni's desire not to overcomplicate what users need to learn, but I think the following order (from most-explicit to most-generic) is reasonable and not overcomplicated.

Per @antocuni's 'sidenote 2', we're focusing mainly on the question: "When the user calls display, to what location in the DOM does the content go?"

As @tedpatrick points out, there's also the questions of "What changes do we make to wrap/change/interpret that content before it gets put in the DOM? And how to do actual make the actual change (append, overwrite, patch)?" but I think those can be considered separately. Let's focus on the where and come back to the what and how.

Here's what I propose as the logic for how the DOM-placement-location is determined. They work from most-explicit to most-general, with "output as a sibling to the current tag" as a fallback:

if the call to `display()` has a 'target' argument?
    if the value of 'target' is the id of a tag on the current page:
        write output to that tag; return
    else: <Error?>

if the current tag (py-script, py-repl, tag with py-on* event, etc) has a 'display-target' attribute:
    ''' Per antocuni/fpliger, this may or may not be an option we want to have. JGlass wants it, see below'''
    if the value of 'display-target' is the id of a tag on the current page:
        write output to that tag; return
    else: <Error?>

if the current page has a 'default-display-target' set in Py-Config:
    if the value of 'default-display-target' is the id of a tag on the page:
        write output to that tag; return
    else: <Error?>

if we`re using lexical-scoping:
    *somehow* identify the context any nested calls to display() were defined in
        Repeat the above logic for that scope
        If the above-logic does not terminate in a result - error?

Or if we`re doing dynamic scoping:
    create new tag as sibling of calling tag; 
    write output to that tag; return

The <Error?> spots correspond to places where the user has provided an ID as a target that doesn't appear on the page. I think we'd be justified here in just raising an error.

To highlight: the reference to the 'current-tag' on line 6 is either the evaluatable tag(py-script, py-repl, etc) or the element whose py-on* event listener triggers the code. This is one solution to @fpliger's desire to avoid default behaviors for event handlers - the <button> itself would have an output-target attribute, which specifies the output location for the results of the event handler code.

Which brings us to...


Lexical vs Dynamic Binding (Opinion)

Personally, I'd advocate for 'dynamic scoping'. Having display() calls lexically scoped to the tag they're defined in limits their re-use across multiple tags on a single page.

In my experience, when creating projects with PyScript, this design pattern (with dynamic scoping) is what I'd like to use almost all of the time:

<py-script>
    def nice_output(raw_value):
        display("Some additional formatting around " + raw_value + " is nice")
</py-script>
...
<py-script output="example-a">
    result = some_action()
    display("We did something")
    nice_output(result) #results near the action on the page
</py-script>
...
<py-script output="example-b">
    another_result = some_entirely_different_thing()
    display("We did something else")
    nice_output(another_result) #results near the other action on the page
</py-script>

And sure, we could pass a target as an argument to nice_output and using that as the the target for display()... but then we're not relying on either kind of scoping, we're just being explicit.

It also brings the answer to "Where is my output going to go when I call display()?" closer to the call itself.

If a user-defined function is always supposed to output to a specific location, the place to specify that is as a argument to the display() function; using the targeting of the surrounding tag for that purpose is unnecessarily removed from its purpose.


Per-tag Targetting (Opinion)

Allow users to set a default target for all display calls within a py-script tag (i.e. <py-script target=bla>)

-0. It probably doesn't hurt, but what is the use case for that?

+3. It allows writing code that can be 'portable' between PyScript tags with different output destinations, or the changing of the output of all calls to display() within a tag without manually changing each and every one.

@antocuni
Copy link
Contributor Author

Wow, lots of very valuable discussion here, thanks to everyone who is contributing! As usual, let me try to answer some comments individually.

re @marimeireles

The current behavior is: we only display by default on py-script tags. Too give more color to my explanation if you run:

<py-script id='py1'>
def say_hello():
    display('hello')
</py-script>
<div>world</div>
<button id='what' py-onclick="say_hello()">Click me</button>

You'll get the py1 id, instead of the what one.

Do you mean that hello would be displayed as a sibling of py1, i.e. before world?
If so, this is what I called "lexical scoping" and it seems that everybody agrees that it's a bad idea, so in that case I fear you have to change your PR.

You're saying we should put rules in place that enforce this behavior (as in implemented in the code and throwing warnings or errors? Or just somewhere in the docs as what you should do?)

we should actively check for this and throw errors in case the "current target" is not defined.
And also have docs :)


re @JeffersGlass

Here's what I propose as the logic for how the DOM-placement-location is determined. They work from most-explicit to most-general, with "output as a sibling to the current tag" as a fallback:
[cut]

The proposed logic looks reasonable to me, but the hard part is to define what is "the current tag" and under which conditions it's well defined.

The <Error?> spots correspond to places where the user has provided an ID as a target that doesn't appear on the page. I think we'd be justified here in just raising an error.

agreed, errors should not pass silently. We should raise a big red error in that case.

To highlight: the reference to the 'current-tag' on line 6 is either the evaluatable tag(py-script, py-repl, etc) or the element whose py-on* event listener triggers the code. This is one solution to @fpliger's desire to avoid default behaviors for event handlers - the <button> itself would have an output-target attribute, which specifies the output location for the results of the event handler code.

I am +1 to define current-tag when we are executing a <py-script>.
I am -1 to define current-tag when we are executing an event handler. Moreover, it doesn't look very useful: if you are clicking a button, the chances that you want to display something next to it are very slim. And if you really want, you can just be explicit.

Which brings us to...

Lexical vs Dynamic Binding (Opinion)

Personally, I'd advocate for 'dynamic scoping'. Having display() calls lexically scoped to the tag they're defined in limits their re-use across multiple tags on a single page.

In my experience, when creating projects with PyScript, this design pattern (with dynamic scoping) is what I'd like to use almost all of the time:

<py-script>
    def nice_output(raw_value):
        display("Some additional formatting around " + raw_value + " is nice")
</py-script>
...
<py-script output="example-a">
    result = some_action()
    display("We did something")
    nice_output(result) #results near the action on the page
</py-script>
...
<py-script output="example-b">
    another_result = some_entirely_different_thing()
    display("We did something else")
    nice_output(another_result) #results near the other action on the page
</py-script>

ok, with this example you convinced me. +1 for dynamic scoping.
And since I think I was the only one advocating for lexical scoping, I think we can say that this particular aspect is settled 🎉.

Per-tag Targetting (Opinion)

Allow users to set a default target for all display calls within a py-script tag (i.e. <py-script target=bla>)
-0. It probably doesn't hurt, but what is the use case for that?

+3. It allows writing code that can be 'portable' between PyScript tags with different output destinations, or the changing of the output of all calls to display() within a tag without manually changing each and every one.

I don't understand what you mean. Could you please provide an example?

@antocuni
Copy link
Contributor Author

It's just "lexical" because in this specific example we're not running any py-script tag.

exactly. In that case, it should be an error but that's precisely what we are trying to decide with this discussion :)

@JeffersGlass
Copy link
Member

The proposed logic looks reasonable to me, but the hard part is to define what is "the current tag" and under which conditions it's well defined.

I think perhaps I confused things by using the term 'current tag' above, which I realize in #749 has special meaning as a global variable. A more accurate term for me, above, would have been "most-recently evaluate()'d py-script/py-repl/etc tag or the source tag of the most-recently trigged py-on* event".

Essentially, we can use ContextVars to create this behavior, and do away with the global CURRENT_PY_SCRIPT_TAG entirely. When PyScript Evaluate()s a tag, or triggers a py-on* event from a tag, if that tag has an output-target attribute, set()s a new value for that ContextVar, evaluates the appropriate Python code, then resets() the context.

I'm cleaning up a demo of this which I think will clarify my proposed approach and demonstrates how it works with various tags (and async). I was in the process of also describing it in more detail here, but honestly I think it will be easier to read the code than the Psuedocode.

I am -1 to define current-tag when we are executing an event handler. Moreover, it doesn't look very useful: if you are clicking a button, the chances that you want to display something next to it are very slim. And if you really want, you can just be explicit.

I think this is again a case where I've confused things. I'll try to clear up my meaning presently.


+3. It allows writing code that can be 'portable' between PyScript tags with different output destinations, or the changing of the output of all calls to display() within a tag without manually changing each and every one.

I don't understand what you mean. Could you please provide an example?

For sure, that wasn't super clear of me. To copy-paste from a current project, with a little reformatting to use the proposed display() syntax:

<py-script>
    import asyncio

    for i in range(1, 20+1):
        display(f"Counted to {i}")
        await asyncio.sleep(1)
    display("I counted to 20!", append=True)

    asyncio.sleep(2)
    
    for i in range(20, 0+1):
        display(f"Counted down to {i}")
        await asnycio.sleep(1)
    display("And we're done!", append=True)
</py-script>

When I'm writing a script like this, I know I want all of the output to go to the same place on the page, but it's not like its behavior is specifically tied to anything special about that place. And that place/tag is might change many times as the page itself evolves, for reasons that have nothing to do with the behavior of the Python code.

If we allow <py-script output-target-"some-div">, it's easy to adjust where the output from the entire script is going. Without it, if I undertand right, we would need to write

display(f"Counted to {i}", output-target="some-div")
...
display("I counted to 20!", append=True, output-target="some-div")
...
etc

And we would have to change the argument of each of those calls whenever a change in page organization requires changing the output target.

And yes, we could use a variable to store the value "some-div" and use that in each function call, but in my personal opinion having the ability set the output target at the tag level keeps the code cleaner and easier to read.

To return to the 'portable' comment of mine above - I just meant that if you wanted to have multiple identical (or similar) scripts running on the same page, being able to configure the output target a the tag level means the code in each of them looks more similar, and it's easier to reason about them together. (Or copy-paste between them.)

Not that this is what folks would universally want to do; I think it would be good practice to say that if your code is coupled to the behavior/layout/placement of the output on the page, then being explicit about the output location wherever possible is good! But for situations where the behavior of the code doesn't really depend (or care) about the output location, being able to configure that at the tag level, I think, is nice.

@fpliger
Copy link
Contributor

fpliger commented Sep 17, 2022

In general I think this is very familiar to anyone used to Notebooks.

this is something which came up many times already, and I don't really understand it. From my POV notebooks cells and tags are something completely different:

Ok. Remove the interaction part of it for a second and abstract it. From an execution standpoint, both are the same thing, just the execution of a block of code.

ok, it's good that we are converging somewhere :).
I think I could live with the following semantics:

display(obj, target=...) is always possible
display(obj) is possible only when the concept of "current target" is clearly defined
display(obj) when "current target" is not defined raises an error
as a first approximation, we declare that "current target" is defined only during the execution of a or tag. Basically, something like this:
class BaseEvalElementWithImplicitTarger extends HTMLElement {
async evaluate(): Promise {
magically_set_current_target(this);
try {
... execute the python code ...
} finally {
magically_set_current_target(undefined);
}
}

+1 on that

for now, we declare that async py-script blocks do not define an implicit target. Later we can think more and see whether we can come up with a reasonable solution based on ContextVar

+0 for right no but really think we should explore solutions right away

-0. It probably doesn't hurt, but what is the use case for that? I mean:

display(obj) display(obj, target="foo") The two examples seem to have the same level of complexity to me: is it worth to introduce yet-another-construct which does the same work as one that we already have? I think it adds cognitive overhead for a very little gain. For example, as soon as you introduce display-target you have to introduce a rule which says who takes the precedence: it is "obvious" that the target=... python argument should take the precedence, but as a beginner it's still a rule that you have to understand and memorize.

The only case in which it saves typing is if you have multiple calls to display(), but IMHO "saving keystrokes" is not so important to add the cognitive overhead I described above.

That's where we disagree. Typing and verbosity does matter imho. I pretty strongly think that it matters and makes things more accessible.

In general, glancing over this, I think I'm pretty much aligned with @JeffersGlass on this. I might be wrong since I'm on a rush but I think it's the case.

@JeffersGlass
Copy link
Member

JeffersGlass commented Sep 20, 2022

(Apologies for the delay on this, I had hoped to have more time over the weekend.)

As promised, I've build a little demo of how this could work using ContextVars, over at JeffersGlass/output-contextvar. It's built on top of @marimeireles work on #749 (much good stuff! And so much code removed!)

If you're interested, the easiest thing to do is check out that branch, build it, and load the outputtargetting.html and outputtargetting-async.html examples. (Not proposing that these be new included examples, they're mostly for illustration.)

EDIT: Here are online demos of outputtargeting.html and outputtargeting-async.html as well.

image

Each section of outputtargetting.html shows a way of using display(), using a <py-script> tag, a <py-repl> tag, a <button> with a py-on* event, and a <py-button>. For each tag:

  • If the display() call has a targetID argument, try to write to the tag with that ID
  • Else, if the most-recently evaluated tag, element with py-on* action, or py-button has an output-target attribute, try to write to the tag with that ID
  • Else, if an id has been set in Py-Config*, try to write to the tag with that ID
  • Else, create a new div as a child of the currently evaluting tag and write to that

(*) I wasn't actually sure the best way to check Py-Config from Python, so there's a hard-coded option at the top of PyScript.js:

appConfig_default_output_location = "default-location-div"
# appConfig_default_output_location = None #uncomment to simulate no default location set in Py-Config

The outputtargetting-async.html example also shows the 4 methods above working in Async threads. To make that work, I had to make the (tiny but significant) change we've been discussing in #751. That should almost certainly be its own PR eventually, but it was necessary to show this demo working.

Like I say, this is more of a demo than a ready-to-merge anything, just to explore what using ContextVars gets us. I'm not wedded to variable names, syntax, arguments, the conditional logic is functional but ugly and unoptimized, and there's definitely untested edge cases (especially with async...) but the bones of the functionality is there. No thought was put into changing/wrapping the content when it gets added, just to where on the page it goes.

In any case, hoping this is a helpful touchpoint for conversation, what aspects feel natural and what don't, what's missing and what's too much.

@marimeireles
Copy link
Member

ok, it's good that we are converging somewhere :).
I think I could live with the following semantics:

    display(obj, target=...) is always possible
    display(obj) is possible only when the concept of "current target" is clearly defined
    display(obj) when "current target" is not defined raises an error
    as a first approximation, we declare that "current target" is defined only during the execution of a or tag. Basically, something like this:
    class BaseEvalElementWithImplicitTarger extends HTMLElement {
    async evaluate(): Promise {
    magically_set_current_target(this);
    try {
    ... execute the python code ...
    } finally {
    magically_set_current_target(undefined);
    }
    }

+2

Typing and verbosity does matter imho. I pretty strongly think that it matters and makes things more accessible.

+2


@JeffersGlass thank you for putting this together!
The following example:

Outputs to location set in Py-config, or adjacent to element (C)

<py-script>display("C")</py-script>

Should throw an error as it's not allowed (as we've discussed in this issue. I think we converged to that?).

Why use ContextVars:

At least in my opinion the main reason for using ContextVars is to test corner case behaviors like the following:

                <py-script id='py1'>
                def say_world():
                    display('world')
                </py-script>
                <py-script id='py2'>
                print('hello')
                    
                </py-script>
                <button py-onclick="say_world()">Click me</button>

They were supposed to hold the information of "which tag invoked the display() function" and then proceed to display underneath the function. Correct?
So taking the example I just posted above, "Hello" should be printed underneath the py-script tag with the pw2 id and the world should be printed underneath the button tag.
I'm not sure... But that's what I got from talking with @antocuni about it (please correct me if I'm wrong).

@JeffersGlass
Copy link
Member

JeffersGlass commented Sep 26, 2022

HI @marimeireles!

They were supposed to hold the information of "which tag invoked the display() function" and then proceed to display underneath the function. Correct?

The ContextVar is holding "the ID of the tag from the currently running script should go, if the call to display() has no targetID argument and there's no global default location set". It also holds whether that ID was set by the tag having an 'output-targeting' attribute or the ID of the tag itself, to be used as a fallback. I made a couple of changes to the ContextVar that holds that state this morning, if you want to take a fresh look 🙂

I've updated the demo to highlight the behavior better, and added radio buttons that allow you to simulate:

  • The user has configured a default output location in <py-config> (all unspecified output goes there)**
  • The user hasn't set a default output location (outputs go below their respective tags)

With a default set in Py-config:
with-default

And without:
without-default

(**) Like I noted above, I didn't actually implement the logic to grab that data from <py-config> - it's just hardcoded at at the top of pyscript.py currently. The new toggle button in the demo just updates the value of the global variable created there.

The async example also works, but to see it, it's best to build the examples locally and change the value of appConfig_default_output_location in pyscript.py.


<py-script>display("C")</py-script>
Should throw an error as it's not allowed (as we've discussed in this issue. I think we converged to that?).

I think this is answered by the clarified answer above - if there's no output-target attribute in the tag and no targetID argument to display(), the output will either go to the default location from \py-config if one is set, or below the triggering tag if not. Since one of those will always be true, I don't think there's a need to error here.

Or at least, that's what I'm proposing. But happy to cut out options from what's presented here.

@marimeireles
Copy link
Member

I think this is answered by the clarified answer above - if there's no output-target attribute in the tag and no targetID argument to display(), the output will either go to the default location from \py-config if one is set, or below the triggering tag if not. Since one of those will always be true, I don't think there's a need to error here.

I thought this was a good solution too. But I remember having a discussion with @antocuni where he disagreed.
So, tagging him here for his opinion.

Other than that I think this is great :)
Excited to have it as a PR... But first we need to merge the display one 😅 and probably have a final discussion here to summarize everything that should be implemented in a more clear way.

@marimeireles marimeireles added backlog issue has been triaged but has not been earmarked for any upcoming release sprint issue has been pulled into current sprint and is actively being worked on labels Oct 4, 2022
@marimeireles
Copy link
Member

I'm closing this issue as things were mostly resolved here and seems like we're moving away from the contextVar implementation, see Antonio's experiments with it in the comment #879.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog issue has been triaged but has not been earmarked for any upcoming release sprint issue has been pulled into current sprint and is actively being worked on
Projects
Archived in project
Development

No branches or pull requests

6 participants