ensure kwargs are always in sync across the whole application #7963
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/cdc6cf6b6bcb11f3348969795fb52912ed31ec92/gradio-4.25.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@cdc6cf6b6bcb11f3348969795fb52912ed31ec92#subdirectory=client/python" |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
There was a problem hiding this comment.
Can confirm this fixes the bug @pngwn ! Can this demo be added to 2e2 tests?
| @@ -28,9 +28,7 @@ | |||
| construct(_target, args: Record<string, any>[]) { | |||
| //@ts-ignore | |||
| const instance = new _target(...args); | |||
| const props = Object.getOwnPropertyNames(instance).filter( | |||
| (s) => !s.startsWith("$") | |||
There was a problem hiding this comment.
What was the purpose of the $ guard?
There was a problem hiding this comment.
In addition to the prop names, there are also some internal svelte things (methods mostly) that start with $. Was always a little hacky.
|
@freddyaboulton Which demo? The blocks flipper is the original test case here and is in the e2e tests, or do you mean the streaming one? |
|
Oh weird I didn't know we already had a test! I guess |
|
Good to merge, we can take a look at the test later. |
|
I updated that test so that it does / would fail consistently on main. The assertion was okay, its just that the test was running faster than the UI could update. This is why it was flakey. Funnily enough the flake was the 'correct' behaviour.
|
|
Nice thanks @pngwn ! |
* ensure kwargs are always in sync across the whole application * add changeset * fix test * update accordion test --------- Co-authored-by: gradio-pr-bot <gradio-pr-bot@users.noreply.github.com>
Description
Closes #7786. Closes #7895.
The issue was not present in dev mode but was present in the built files. So we weren't noticing this behaviour day to day in development.
The root cause, seems to be that the list of props that we were using to listen for changes wasn't the same in dev + build. I'm not 100% why really, its a little confusing but I have confirmed that the approach in this PR works for all modes.
I still need to look at the test, but I need to create a failing test from main first. Will update shortly but the code is good for review.
To test you can use the
blocks_flipperdemo. This does not behave itself on main but works correctly in dev. You can also use the repro in #7895 as it is the same issue.Be sure to build the frontend locally and visit port
7860and not the dev port (9876).Update
I've update the test. The test was opening the accordion, clicking the slider and then checking that accordion text was on the page. The issue here is that for a brief moment after changing the slider value this is true, so sometimes this was passing (sometime it was failing too but we have retries on).
I've switched it out for a test that opens the accordion, changes the slider value then enters text into the textbox and waits for the server response + state update. This guarantees that the UI state has fully settled before asserting on the accordion contents.
This test passes on this branch but it fails consistently on main.