Skip to content

Support ttl and max_entries kwargs #6449

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.

Sign up for GitHub

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

Jump to bottom
Merged
vdonato merged 3 commits into from
Apr 11, 2023
Merged

Support ttl and max_entries kwargs #6449

vdonato merged 3 commits into from
Apr 11, 2023

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented Apr 7, 2023

📚 Context

Not too much to describe here -- a mostly mechanical change that does what the title says.

@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label Apr 7, 2023
@vdonato vdonato requested a review from kajarenc April 7, 2023 00:18
@kajarenc
Copy link
Collaborator

kajarenc commented Apr 7, 2023

Hey @vdonato!

it is very not obvious, but these changes could lead to a strange buggy behaviour:

let's say we have two connections with different max_entries.

sql_con = st.experimental_conneciton("sql", ..., max_entries=3)
snow_con = st.experimental_connection("snowpark", ..., max_entries=5)

since this st.experimental_connection call under the hood will decorate the same __create_connection function, but with different max_entries, based on our st.cache_recource implementation

streamlit/lib/streamlit/runtime/caching/cache_resource_api.py

Line 98 in 61830a3

if (
, this two connections will share the same key in self._function_caches global dict, but since they have different max_entries or ttl, ResourceCache corresponding to the first connection will be erased and replaced with second ResourceCache within the same key in self.function_caches.

I will be happy to have a call and help to debug this edge case with you.

@vdonato
Copy link
Collaborator Author

vdonato commented Apr 7, 2023

I will be happy to have a call and help to debug this edge case with you.

@kajarenc thanks! Just pinged you internally to schedule

@vdonato vdonato added the do-not-merge PR is blocked from merging label Apr 7, 2023
@vdonato vdonato force-pushed the feature/st.experimental_connection branch from f44239f to 044e1ff Compare April 10, 2023 21:08
@vdonato vdonato force-pushed the vdonato/ttl-and-more branch from aab39eb to 197535a Compare April 10, 2023 21:09
@vdonato vdonato removed the do-not-merge PR is blocked from merging label Apr 10, 2023
@vdonato vdonato force-pushed the feature/st.experimental_connection branch from 631a509 to ccf92e6 Compare April 11, 2023 19:08
@vdonato vdonato force-pushed the vdonato/ttl-and-more branch from adf81a1 to 0ba7882 Compare April 11, 2023 19:16
@vdonato
Copy link
Collaborator Author

vdonato commented Apr 11, 2023

Update on this: this sounds like something we'll want to address eventually, but it doesn't seem feasible to do so in the timeframe that we have left before we want to merge this feature.

Since st.connection is initially being released as st.experimental_connection, we're okay with noting the ttl and max_entries behavior as a limitation and fixing it before we "de-experimentalize" the feature.

@vdonato vdonato merged commit 2d0363c into feature/st.experimental_connection Apr 11, 2023
@vdonato vdonato deleted the vdonato/ttl-and-more branch April 11, 2023 22:33
vdonato added a commit that referenced this pull request Apr 15, 2023
@vdonato
Support ttl and max_entries kwargs (#6449)
97de4c5 
* Support ttl and max_entries kwargs

* Don't forget to pass kwargs to internal function

* Fix test failing due to rebase
vdonato added a commit that referenced this pull request Apr 17, 2023
@vdonato
Support ttl and max_entries kwargs (#6449)
e1121ea 
* Support ttl and max_entries kwargs

* Don't forget to pass kwargs to internal function

* Fix test failing due to rebase
vdonato added a commit that referenced this pull request Apr 19, 2023
@vdonato
Support ttl and max_entries kwargs (#6449)
fbeff72 
* Support ttl and max_entries kwargs

* Don't forget to pass kwargs to internal function

* Fix test failing due to rebase
vdonato added a commit that referenced this pull request Apr 20, 2023
@vdonato
Support ttl and max_entries kwargs (#6449)
2b70621 
* Support ttl and max_entries kwargs

* Don't forget to pass kwargs to internal function

* Fix test failing due to rebase
vdonato added a commit that referenced this pull request Apr 21, 2023
@vdonato
Support ttl and max_entries kwargs (#6449)
3ef3c26 
* Support ttl and max_entries kwargs

* Don't forget to pass kwargs to internal function

* Fix test failing due to rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kajarenc kajarenc kajarenc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vdonato @kajarenc