Fix dataframe maximum update depth error #9490
Conversation
lukasmasuch
added
security-assessment-completed
change:bugfix
lukasmasuch
changed the title
lukasmasuch
changed the title
| """This test checks that the dataframe component renders without crashing | ||
| when used in different containers. |
There was a problem hiding this comment.
Is this test always failing without the fix? I assumed its pretty difficult to hit the issue. If it usually does not fail even without the fix, it might make sense to add a comment here.
There was a problem hiding this comment.
Interestingly, I was able to trigger the error in the st_popver case in CI run when configuring the one dataframe to use container width. But not consistently. Based on that, I believe it could happen in CI, probably also because the CI uses a resolution around ~1200x700. But I think the test is mostly about giving us some safeguarding around similar types of crashes for dataframe.
There was a problem hiding this comment.
I will add a comment.
| let maxWidth = availableWidth | ||
|
|
||
| if (element.useContainerWidth) { | ||
| // Always use the full container width | ||
| initialWidth = containerWidth | ||
| // If user has set use_container_width, | ||
| // use the full container width. | ||
| initialWidth = availableWidth | ||
| } else if (element.width) { | ||
| // User has explicitly configured a width | ||
| initialWidth = Math.min( | ||
| Math.max(element.width, MIN_TABLE_WIDTH), | ||
| containerWidth | ||
| availableWidth | ||
| ) | ||
| maxWidth = Math.min(Math.max(element.width, maxWidth), containerWidth) | ||
| maxWidth = Math.min(Math.max(element.width, maxWidth), availableWidth) | ||
| } |
There was a problem hiding this comment.
I think the multiple usage of nested min-max calls make it sufficiently hard to read and understand to warrant moving this into its own function and add some comments 😅 I think it would be good to have a list of the different widths in bullet points below each other giving a brief explanation why its needed and what the differences are and how it is calculated in a few words.
There was a problem hiding this comment.
I added a lot more comments 👍
raethlein
left a comment
raethlein
left a comment
There was a problem hiding this comment.
LGTM! Thanks for getting to the bottom of this very gnarly issue 🚀 And thanks for adding the comments 👍
## Describe your changes Fixes an issue with the dataframe component that causes random crashes:  The crash can happen on low-resolution monitors if dataframes are rendered with `use_container_width=True` are used in some of our containers (popover, tabs, sidebar, columns). It appears that our width calculation in the Block component causes this. If the width can't be determined yet by the resize observer, it can be -1 for some time. This seems to be a trigger for the maximum update depth. Unfortunately, I don't have a validated explanation of why it actually leads to the error :( ## GitHub Issue Link (if applicable) - Closes streamlit#7949 ## Testing Plan - Add e2e test to discover similar breakage. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
2 participants
Describe your changes
Fixes an issue with the dataframe component that causes random crashes:
The crash can happen on low-resolution monitors if dataframes are rendered with
use_container_width=Trueare used in some of our containers (popover, tabs, sidebar, columns). It appears that our width calculation in the Block component causes this. If the width can't be determined yet by the resize observer, it can be -1 for some time. This seems to be a trigger for the maximum update depth. Unfortunately, I don't have a validated explanation of why it actually leads to the error :(GitHub Issue Link (if applicable)
st.dataframecrashes with maximum update depth exceeded #7949Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.