-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[App] Increased DeepDiff's verbose level to properly handle dict changes #13960
Conversation
Thank you @adam-lightning, this is indeed the intended use of Can you please open a separate issue related to the assignment? |
This issue is been partially solved in https://github.com/Lightning-AI/lightning/blob/4c35867b618b4c36bfac5428756b95223f7f526a/src/lightning_app/utilities/proxies.py#L173 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Do you know if anyone have tested that observer with streamlit ? Just curious. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
What does this PR do?
Problem
While building a streamlit UI we found that if we do the following:
And later trigger the status update the App will fail with something of the following error:
Solution
It turns out that
DeepDiff
on currentverbose_level = 1
(default) while creating diff only records what keys were added or changed, and not recording the new value. This diff is not applicable to the state with+=
operation (because it's missing a new value to set) so it fails with the error above. Increasingverbose_level=2
now includes the new item value -> solves the issue.Disclaimer: We know that just setting
state.my_dict["some_key"] = "some_value"
does not trigger a state update by itself (the value will not be visible outside). We need other assignment operation to do that (any use of=
on state variable would do that). This PR does not address this issue at all.Does your PR introduce any breaking changes? If yes, please list them.
Nothing I know of. But would be great if someone chimes in and verifies that changing
verbose_level
in all places is actually safe ;)Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda