wave icon indicating copy to clipboard operation
wave copied to clipboard

Don't change state of unchanged cards / allow "no ui-changes" serves

Open Far0n opened this issue 2 years ago • 7 comments

Wave SDK Version, OS

wave v0.17, ubuntu

Actual behavior

Given the following app, the textboxes get cleared even though the card is not changed after submit:

@app('/')
async def serve(q: Q):
    print("q.args", q.args)

    if q.args.submit:
        q.page['NEW_CARD'] = ui.form_card(box='1 5 4 4', items=[])
    else:
        q.page['example'] = ui.form_card(box='1 1 4 4', items=[
            ui.textbox(name='textbox_01', label='textbox_01'),
            ui.textbox(name='textbox_02', label='textbox_02'),
            ui.button(name='submit', label='Submit', primary=True),
        ])
    await q.page.save()

https://user-images.githubusercontent.com/6949295/132977337-59afb335-8aac-42fc-845f-7bcc23b6ad45.mp4

We also have been discussing the issue here: https://h2oai.slack.com/archives/CPA21SSJH/p1631116412031900

Expected behavior

Citing @lo5 (from slack thread above)

it shouldn't clear the textboxes. the 'example' form should continue to exist, and you should see a new form somewhere on the page.

Additional Context and related issues with using JS

A closely related issue appears when we use JS. Since we can't just return from the serve function without any changes to q.page (resulting in spinning wheel otherwise) dealing with JS can easily yield unintended behavior.

Example: Let's say we have a simple html with a <div id="container></div> whose content is controlled via some JS. If we change it via q.['meta'].inline_script(."setContainerContent(..)") the issue above occurs. It's also problematic the other way around: Whenever the serve function is invoked, we currently need to update the JS-container. If we don't "maintain" the card containing the JS-container it gets refreshed and we loose our JS-container content. Because the submit can be very unrelated to the "JS-card" (like switching the theme) the current best solution seems to be to always rebuild everything each time serve gets called.

But this yields another issue: Since wave doesn't know about the dynmaic nature of our container we need to somehow make sure to have the actual state if we rebuild that card. One of many possibly approaches would be to do the following at the top of serve: q.['meta'].inline_script(."getContainerContent()") -> wave.emit('container', 'content', container.value)

But since we have the OP-issue that wont quite work. So we would need to check for changes between q.args and q.client|app first in order to be able to rebuild every card properly. This becomes easily a mess.

Okay, plan B: why not telling wave when the container content changes? That is, calling wave.emit on content change. Now this becomes a use-case for which we would like to return from serve without any UI changes. Something like:

"""
btw is there a better way to write the following check? 
Like checking q.events.container.changed_content directly is prone to run into NoneType not subscriptable,
but Expando doesn't seem to have a convenience method to scan for "2nd order" keys.
"""
if q.events.container and q.events.container.changed_content:
   q.app.container_content = q.events.container.changed_content
   return

=> spinning wheel

So I tried this hack:

if q.events.container and q.events.container.changed_content:
   q.app.container_content = q.events.container.content_changed
   q.page['meta'].redirect = None # or ''
   await q.page.save()
   return

=> runs into OP issue, similar to this example app:

from h2o_wave import main, app, Q, ui

@app('/')
async def serve(q: Q):
    print("q.args", q.args)

    if q.args.submit:
        q.page['meta'].redirect = ""
    else:
        q.page['example'] = ui.form_card(box='1 1 4 4', items=[
            ui.textbox(name='textbox_01', label='textbox_01'),
            ui.textbox(name='textbox_02', label='textbox_02'),
            ui.button(name='submit', label='Submit', primary=True),
        ])
    await q.page.save()

https://user-images.githubusercontent.com/6949295/132979170-a7d8fd38-fad2-40ef-aa7c-a9fdd684cc34.mp4

My expectation: q.args always contains the values for textbox_01 and textbox_02 Actual behavior: q.args only contains values of changed textbox values.

(also from slack thread above)

Far0n avatar Sep 12 '21 08:09 Far0n

@lo5 this behavior seems to be intentional, see https://github.com/h2oai/wave/blob/31c349df3527569e8e0de0615679a57577fee654/ui/src/ui.ts#L142

Can you clarify or is the line ^^ good to be removed?

mturoci avatar Sep 16 '21 07:09 mturoci

@mturoci if the args are not cleared, it will grow over the lifetime of the front end, especially if the app has multiple forms across multiple screens.

lo5 avatar Sep 17 '21 02:09 lo5

Makes sense, what about clearing this only on route (hash) change? This would submit only values related to the currently displayed page.

@Far0n the original problem:

Given the following app, the textboxes get cleared even though the card is not changed after submit:

is already fixed FYI

A closely related issue appears when we use JS. Since we can't just return from the serve function without any changes to q.page (resulting in spinning wheel otherwise) dealing with JS can easily yield unintended behavior.

Although I agree with general intention, the current implementation sends response only when something changed in the UI, which is what troubles most of the people. IMO a better solution would be to always respond once the q.page.save or serve function finishes. That way people could freely remove all their hacks, if they are not blocking main python thread. If main thread is blocked (due to a long-running job), the app will display the loading screen. @lo5 wdyt? Or would you rather special-casing the JS somehow and keep the original rule that an action in event should always result in an update.

mturoci avatar Sep 17 '21 06:09 mturoci

Just offering an unsolicited opinion, having read several threads on this topic... I think this idea that "not updating the UI is a bad practice" is itself wrong.

In functional programming (which Wave does largely embrace despite having the appearance of an imperative approach), rendered components are considered "pure functions of state". The framework (React, whatever) takes responsibility for calling the components with the State at appropriate times, and usually also takes responsibility for 'detecting' changes to the state. Wave does both of these things.

In math, a function f takes an input i and returns an output o. Crucially - it is mathematically required that if the output o of f(_) changes, that is because the input i changed - you should always get the exact same result when calling f(i) no matter how many times you call it. So far so good - Wave does this (modulo possible bugs). But the converse is not true - it is not expected that a function f should always return a different output for each input. In fact, f(i) may equal f(i2) quite frequently - case in point, the famous fibonacci sequence, where f(1) and f(2) both equal 1.

If Wave is to be a functional UI framework, then it must be willing to render the exact same UI based on two different state inputs, and this should be considered a well-formed request to the purely functional backend.

An obvious example of this is a validation handler that receives all updates to a text box, and switches the Submit button from enabled to disabled whenever the text becomes something that the server will not be willing to accept. In many states, the button should be disabled; in many other states, the button should remain enabled. There is no need for the backend to pretend that anything has fundamentally changed when the input changes from 90 to 901 - but if the input then changes from 901 to 901T, this number is no longer parseable and the button should be disabled - the "UI value" has changed.

Y'all have built what in some ways is the most intuitive, Pythonic UI framework I've ever seen - "receive query state, return UI state" - but this is, in my humble but professional opinion, a gaping hole in your abstraction. It's fundamentally tearing down the very claim you make in https://wave.h2o.ai/docs/guide#who-is-this-for - Wave cannot be properly "functional in the functional programming sense" if it refuses to map a new input i2 to the same output as as previous input i1.

I would love to see you reconsider your approach here. :)

petergaultney avatar Dec 07 '23 04:12 petergaultney

Thanks for the input @petergaultney!

I think what you say makes sense here, but let me clear some incorrect assumptions.

In fact, f(i) may equal f(i2) quite frequently

True. However, in order to be fast, Wave (unlike others) is purely write-only. It doesn't store UI state on python server. All the info about what cards are rendered is stored client-side (browser).

That's the reason why you can't simply print q.page['my-card'], but get that weird Reference object instead.

This makes the whole conversation much simpler because Wave doesn't compare inputs and outputs. It only checks if there are any UI changes to apply (even if redundant).

Your validation case is a good example. The solution in this case would be to make your validate function always return True/False and set it to the submit button disabled state (even if that means going from False to False again).

Another hacky solution is to use q.page['non-existent'].items = [] fake update and get rid of the loading completely.

The why

Based on your recent posts (hugely appreciated) I can fairly confidently say you are a software engineer, seemingly a good/knowledgable one. However, Wave is used by 2 quite different types of people:

  • AI/ML/DS folks - focus more on the demo and the message it emits - code quality is less important.
  • Software engineers - focus on the code quality more.

The reason for the loading feature existence is to simply prevent people from the first group from writing suboptimal apps and to make them think modre about the UX. You are the first person to bring up form validation (although it should be standard).

mturoci avatar Dec 07 '23 09:12 mturoci

I think maybe I've missed how this actually works in practice. I've understood these conversations to be saying that a change to the state must occur; but it sounds like all that actually is required is that a message is sent to the client - i.e., that q.page.save() is called at some point. Is the latter interpretation correct? Or is it something in-between - perhaps the requirement is to assign some value into one of these references, such that Wave (intercepting __setitem__ or __setattr__) can see that you've done "something" even if that something results in no actual change to what the client will store locally?

If one of these latter options is true, perhaps the documentation would be clearer if this was said explicitly? Either "some sort of message must be sent to the client, and a message can be forced with page.save() -- or, "some attribute value must be assigned into the page state prior to save, even if that value is the same as the value that the client already has".

petergaultney avatar Dec 09 '23 20:12 petergaultney

q.page.save() is called at some point assign some value into one of these references, such that Wave (intercepting setitem or setattr) can see that you've done "something"

Correct. There must be "some" message sent back so both q.page.save and assignment needs to happen.

perhaps the documentation would be clearer if this was said explicitly

Makes sense. We have this, feel free to open a PR with suggested change that would have helped you while figuring this out.

mturoci avatar Dec 11 '23 07:12 mturoci