Move check_* utils to own file #8764

raethlein · 2024-05-24T13:40:19Z

Describe your changes

In streamlit/elements/utils.py we have some check_* functions that are used to enforce certain rules. This PR moves them to their own policies.py file for cleaner abstraction.
Also, tests for check_cache_replay_rules were missing that are added now.

This is in preparation of adding a new check_* policy function in this PR: https://github.com/streamlit/streamlit/pull/8756/files

GitHub Issue Link (if applicable)

Testing Plan

Unit Tests (JS and/or Python)
- moved unittests and added new ones

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

lib/streamlit/elements/policies.py

lib/streamlit/elements/widgets/camera_input.py

lib/streamlit/elements/widgets/checkbox.py

lib/streamlit/elements/widgets/color_picker.py

lib/streamlit/elements/widgets/file_uploader.py

lib/streamlit/elements/widgets/radio.py

lib/streamlit/elements/widgets/select_slider.py

lib/streamlit/elements/widgets/slider.py

lib/streamlit/elements/widgets/text_widgets.py

lib/streamlit/elements/widgets/time_widgets.py

lukasmasuch

It seems like that the methods are not yet removed from utils.py or am I missing something. Also, it seems to not yet cover all relevant elements (e.g data editor is missing?)

lukasmasuch · 2024-05-24T17:09:53Z

lib/streamlit/elements/policies.py

I'm a bit annoyed by all the import cycle warnings. I believe this import might be the issue. Can we move this import down into the function from where it is used?

Yeah the circular imports at some places in the code base are pretty annoying. I have spent some time today looking into refactoring it and refactoring everything including the delta generator are on top of my list 😄 moving the imports down - if it works at all as it opened a rabbit hole earlier - just hides the original problem that the code is not pretty well abstracted. I guess eventually we have to start cleaning it up top down.

moving the imports down - if it works at all as it opened a rabbit hole earlier - just hides the original problem that the code is not pretty well abstracted

Yeah, I think the better solution would be to have some internal import for exception and warning, but I'm not sure if that is viable without bigger refactoring. But just having a blank streamlit import here on top doesn't feel super good. Overall, it's better to move this import down and all the other check_ imports to the top level.

Since its a delta generator function, the best way would likely be to have to pass the deltagenerator to the function instead of importing a global object. But this surfaces other import issues at other places; so I think the first step has to be abstracting the delta generator and probably move out the main_dg and other *_dg variables and dg_stack out and work with a singleton factory or so. But that requires a little bit more time

lib/streamlit/elements/policies.py

lib/streamlit/elements/utils.py

lib/streamlit/elements/arrow.py

lib/streamlit/elements/widgets/button.py

lib/streamlit/elements/plotly_chart.py

lib/streamlit/elements/vega_charts.py

raethlein · 2024-05-27T19:52:34Z

@lukasmasuch could you please have another look? The CodeScan things look like a lot, but I have moved the imports within the policies.py file so that we can move the inner-imports in the different element files to the top. So the situation should be better than before 🙂

lukasmasuch

LGTM 👍 I think it would be good to move policies and/or utils into the element/lib folder. These are the only two non-element modules within this folders, and it feels like element/lib would be a better place.

raethlein · 2024-05-28T05:56:12Z

LGTM 👍 I think it would be good to move policies and/or utils into the element/lib folder. These are the only two non-element modules within this folders, and it feels like element/lib would be a better place.

Great point, I have moved both utils files

lib/streamlit/elements/lib/policies.py

+            f'The widget with key "{key}" was created with a default value but'
+            " also had its value set via the Session State API."
+        )
+        _shown_default_value_warning = True


lib/streamlit/elements/arrow.py

+from streamlit.elements.lib.policies import (
+    check_cache_replay_rules,
+    check_callback_rules,
+    check_session_state_rules,
+)


lib/streamlit/elements/widgets/button.py

+from streamlit.elements.lib.policies import (
+    check_cache_replay_rules,
+    check_callback_rules,
+    check_session_state_rules,
+)


lib/streamlit/elements/widgets/data_editor.py

+from streamlit.elements.lib.policies import (
+    check_cache_replay_rules,
+    check_callback_rules,
+    check_session_state_rules,
+)


lib/streamlit/elements/widgets/multiselect.py

+from streamlit.elements.lib.policies import (
    check_cache_replay_rules,
    check_callback_rules,
    check_session_state_rules,
+)


lib/streamlit/elements/lib/policies.py

+from typing import TYPE_CHECKING, Any
+
+from streamlit import config, runtime
+from streamlit.elements.form import is_in_form


lib/streamlit/elements/widgets/radio.py

+from streamlit.elements.lib.policies import (
    check_cache_replay_rules,
    check_callback_rules,
    check_session_state_rules,
+)


lib/streamlit/elements/widgets/select_slider.py

+from streamlit.elements.lib.policies import (
    check_cache_replay_rules,
    check_callback_rules,
    check_session_state_rules,
+)


lib/streamlit/elements/widgets/selectbox.py

+from streamlit.elements.lib.policies import (
    check_cache_replay_rules,
    check_callback_rules,
    check_session_state_rules,
+)


lib/streamlit/elements/vega_charts.py

+from streamlit.elements.lib.policies import (
+    check_cache_replay_rules,
+    check_callback_rules,
+    check_session_state_rules,
+)


## Describe your changes In https://github.com/streamlit/streamlit/blob/develop/lib/streamlit/elements/utils.py we have some check_* functions that are used to enforce certain rules. This PR moves them to their own policies.py file for cleaner abstraction. Also, tests for check_cache_replay_rules were missing that are added now. This is in preparation of adding a new check_* policy function in this PR: https://github.com/streamlit/streamlit/pull/8756/files ## GitHub Issue Link (if applicable) ## Testing Plan - Unit Tests (JS and/or Python) - moved unittests and added new ones --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Replace the TTLCache in ResourceCache with a TTLCleanupCache. Add on_release to the st.cache_resource API, and plumb it through to the TTLCleanupCache. Update st.cache_resource.clear_all to clear the cache directly instead of just GCing it to ensure release functions are called. Implements feature request in #8764 .

## Describe your changes Replace the TTLCache in ResourceCache with a TTLCleanupCache. Add on_release to the st.cache_resource API, and plumb it through to the TTLCleanupCache. Update st.cache_resource.clear_all to clear the cache directly instead of just GCing it to ensure release functions are called. Implements feature request in #8764 . ## GitHub Issue Link (if applicable) - Closes #8674 ## Testing Plan - Unit Tests (JS and/or Python) Implemented. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

raethlein requested a review from lukasmasuch May 24, 2024 13:41

raethlein added security-assessment-completed Security assessment has been completed for PR impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change labels May 24, 2024

github-advanced-security bot found potential problems May 24, 2024

View reviewed changes

lukasmasuch reviewed May 24, 2024

View reviewed changes

raethlein force-pushed the refactor-widget-checks-util branch from 73b59b9 to b5aad90 Compare May 26, 2024 10:59

github-advanced-security bot found potential problems May 26, 2024

View reviewed changes

lib/streamlit/elements/arrow.py Fixed Show fixed Hide fixed

lib/streamlit/elements/widgets/button.py Fixed Show fixed Hide fixed

lib/streamlit/elements/plotly_chart.py Fixed Show fixed Hide fixed

lib/streamlit/elements/vega_charts.py Fixed Show fixed Hide fixed

raethlein force-pushed the refactor-widget-checks-util branch from 94e6571 to 12ec5c3 Compare May 27, 2024 19:27

lukasmasuch approved these changes May 27, 2024

View reviewed changes

raethlein added 7 commits May 28, 2024 07:43

Move check_* utils to own file

5b6ef37

Add docstrings

631f581

Fix tests

a91dd09

Centralize circular-import workaround in policies.py

ea77118

Update import

a61684d

Fix formatting

f90ca61

Move utils functions into lib folder

18888e0

raethlein force-pushed the refactor-widget-checks-util branch from 12ec5c3 to 18888e0 Compare May 28, 2024 05:55

github-advanced-security bot found potential problems May 28, 2024

View reviewed changes

raethlein merged commit 405be2d into develop May 28, 2024

raethlein deleted the refactor-widget-checks-util branch August 8, 2024 14:07

sfc-gh-jkinkead mentioned this pull request Dec 22, 2025

Add on_release to st.cache_resource. #13439

Merged

Move check_* utils to own file #8764

Move check_* utils to own file #8764

Uh oh!

Conversation

raethlein commented May 24, 2024

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukasmasuch left a comment

Choose a reason for hiding this comment

Uh oh!

lukasmasuch May 24, 2024

Choose a reason for hiding this comment

Uh oh!

raethlein May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasmasuch May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raethlein May 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raethlein commented May 27, 2024

Uh oh!

lukasmasuch left a comment

Choose a reason for hiding this comment

Uh oh!

raethlein commented May 28, 2024

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raethlein May 24, 2024 •

edited

Loading

lukasmasuch May 24, 2024 •

edited

Loading