-
Notifications
You must be signed in to change notification settings - Fork 4k
st.experimental_connection: The big merge #6487
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
Conversation
7883f66 to
412613d
Compare
2f2aede to
81c8c17
Compare
| return _query(sql) | ||
|
|
||
| @contextmanager | ||
| def session(self) -> Iterator["Session"]: |
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.
If I'm only using session() and not query(), do I have any easy way to ensure the connection gets reset if it expires? Or do I need to implement that from scratch? 🤔
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.
Per @sfc-gh-bhess we can use session._conn._conn.is_closed() to check whether the connection has expired and reset in that case: "The session is keeping an HTTP session open a long time (long keep-alive) and is able to see if that session has been killed (by either end)."
If that will work, seems like for Snowpark that would be a faster and more reliable option than using tenacity - just make sure it gets checked for _instance (I think?) or at least in session() and query().
Also solves the tenacity import problem for SiS.
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.
Is there a JIRA for Snowpark to provide session.is_closed? If yes we'll look whether _conn.is_closed is reliable.
| return _query(sql) | ||
|
|
||
| @contextmanager | ||
| def session(self) -> Iterator["Session"]: |
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.
Is there a JIRA for Snowpark to provide session.is_closed? If yes we'll look whether _conn.is_closed is reliable.
23900ac to
5cc7cd0
Compare
lukasmasuch
left a comment
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 👍
* Reload the connection when its secrets section changes * Use json instead of pickled dict to construct hash
* Add implementation of SQL connection * Use sqlalchemy.text for 2.0 compatibility * Add unit tests for SQL connection * Add autocommit kwarg to connection_factory overload
* Add a first party Snowpark connection * Use the new AttrDict.to_dict() method * Add a unit test for Snowpark.read_sql
* Remove the notion of default connection names * Rework connection factory API * Name type variable more descriptively * Change from Union[str, None] to Optional[str]
* Prefix a bunch of our BaseConnection methods with an underscore * Add underscore in front of get_secrets * Rename _get_secrets() -> _secrets and make it a @Property * Tweak some st.experimental_connection-related names
* Rename connection_class -> type * Rename read_sql -> query for SQL and Snowpark connections * Plumb a few arguments through SQL.query explicitly * Get rid of now stale TODO
* Add something closer to what our final default _repr_html_ function will be * Also add implementation of SQL._repr_html_()
* Support ttl and max_entries kwargs * Don't forget to pass kwargs to internal function * Fix test failing due to rebase
* Add tenacity as an "optional dependency" for the library (but a dependency for connections) * Add retries to SQL.read_sql * Add retries to Snowpark.read_sql * Fix tests failing due to rebase
…ons (#6486) * Add some utility functions for manipulating dicts * Teach the SQL connection to merge params set via secrets.toml and kwargs * Merge secrets from various sources in Snowpark connection * Add a @pytest.mark.require_snowflake decorator * Remove "password" from required params * Use ChainMap instead of custom helper function
* Use the current active Snowpark session if one is available * Only run Snowpark tests in Snowflake CI tests * Add `type: ignore`s
#6511) * Don't always retry for Snowpark connection * Don't always retry for SQL connection
* Add missing default arg * Error when tenacity is not installed on connection creation * Add more missing module -> PyPI package mappings * Change spinner messages for .query() methods to hide internals * Complain more clearly if no SQL connection params at all are set * Support named snowsql config file sections * Remove unneeded f-string * Raise exception when no Snowpark config is set * Use correct mysql PyPI package name * Make query method spinner messages lowercase
* Remove "user" from required SnowparkConnection params * Remove SnowparkConnection locking in __init__ method * Remove handling for tenacity as an optional dependency * Rename SQL -> SQLConnection * Rename Snowpark -> SnowparkConnection * Add nicer `st.experimental_connection` spinner message * Support both session() and safe_session() in SnowparkConnection * Make session a @Property * Don't nest contextmanagers in SQLConnection * Make SQLConnection.session a property * Remove TODO
* Update docstrings for base connection and snowpark connection * Add SQL docstring updates * Add connection_factory docstrings * Fix reStructuredText issues in connection_factory docstrings * Fix reStructuredText issues in ExperimentalBaseConnection docstrings * Fix reStructuredText issues in Snowpark docstrings * Fix reStructuredText issues in SQL docstrings * Address PR feedback * Post-rebase cleanups * More cleanups from omnibus changes * Add explicit guidance to not initialize connections directly * Add example and fix note formatting on snowparkconnection.session * Formatting fixes --------- Co-authored-by: snehankekre <snehan.minerva@gmail.com>
42b2470 to
4bafabd
Compare
|
Just finished my final round of QA and updated https://st-connection-preview.streamlit.app/ with the latest whl from this PR. Everything looks good to merge IMO. ✅ We didn't resolve the issue to use |
|
Just wrapped up a last testing pass myself -- going to click the submit button! |
* develop: Removing viz-1.8.0.min.js (streamlit#6520) st.experimental_connection: The big merge (streamlit#6487) Add additional attributions (streamlit#6536) Fix code block font change (streamlit#6535) Fully remove email from new session message (streamlit#6516) Clarify what telemetry data our backend stores (streamlit#6463) Update emojis to latest state (streamlit#6532) Add support for pandas 2.0 (streamlit#6507) Fix regression in visibility of `st.code`'s copy-to-clipboard button (streamlit#6498) Fix E2E image (streamlit#6524) Add tenacity as dependency (streamlit#6529)
📚 Context
This is the big merge of the
feature/st.experimental_connectionbranch intodevelop!Note that this feature was built incrementally with small PRs being merged into the feature branch, and
each of the PRs was reviewed. Thus, it's not necessary to do an extremely thorough code review on this PR
before approving it, but it would still be good to do some sanity checks on the code before rubber-stamping
the PR.
st.experimental_connectionis a new feature designed to help with the currently tedious process of configuringand using connections to data sources inside of a Streamlit app. It provides a few things as part of its
public-facing API:
st.experimental_connectionfactory function, which helps developers with initializing a connectionwhile following best practices that are often easy to overlook. Most notably, it ensures that connections
created with it are cached so that they don't get reinitialized with every script run.
SQLConnectionandSnowparkConnection. These connections can easilybe created from the
st.experimental_connectionfactory function. They provide ways to work with theSQLAlchemy and Snowpark for Python libraries, respectively, with built-in support for Streamlit's secrets
management mechanisms.
by the Streamlit team. These connections may eventually be integrated into the core library after we're
more confident in their quality and assuming there aren't dependency-related reasons we're unable to
include them.
ExperimentalBaseConnectionabstract class to simplify the process of writing an
st.experimental_connection-compatible connectionimplementation.