Skip to content

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented Apr 12, 2023

📚 Context

This is the big merge of the feature/st.experimental_connection branch into develop!

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_connection is a new feature designed to help with the currently tedious process of configuring
and using connections to data sources inside of a Streamlit app. It provides a few things as part of its
public-facing API:

  • The st.experimental_connection factory function, which helps developers with initializing a connection
    while 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.
  • Two first-party connections: SQLConnection and SnowparkConnection. These connections can easily
    be created from the st.experimental_connection factory function. They provide ways to work with the
    SQLAlchemy and Snowpark for Python libraries, respectively, with built-in support for Streamlit's secrets
    management mechanisms.
    • We'll be releasing additional connections that aren't shipped as part of the core library but are maintained
      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.
  • For developers who want to create their own connections, we provide an ExperimentalBaseConnection
    abstract class to simplify the process of writing an st.experimental_connection-compatible connection
    implementation.

@vdonato vdonato added the do-not-merge PR is blocked from merging label Apr 12, 2023
@vdonato vdonato force-pushed the feature/st.experimental_connection branch 2 times, most recently from 7883f66 to 412613d Compare April 17, 2023 23:22
@vdonato vdonato force-pushed the feature/st.experimental_connection branch from 2f2aede to 81c8c17 Compare April 19, 2023 19:58
@vdonato vdonato removed the do-not-merge PR is blocked from merging label Apr 19, 2023
return _query(sql)

@contextmanager
def session(self) -> Iterator["Session"]:
Copy link
Contributor

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? 🤔

Copy link
Contributor

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.

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"]:

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.

@vdonato vdonato force-pushed the feature/st.experimental_connection branch from 23900ac to 5cc7cd0 Compare April 20, 2023 23:55
@vdonato vdonato requested a review from lukasmasuch April 21, 2023 01:05
@vdonato vdonato marked this pull request as ready for review April 21, 2023 01:20
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@vdonato vdonato added security-assessment-completed Security assessment has been completed for PR dev:full-matrix labels Apr 21, 2023
vdonato and others added 21 commits April 21, 2023 16:04
* 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>
@vdonato vdonato force-pushed the feature/st.experimental_connection branch from 42b2470 to 4bafabd Compare April 21, 2023 23:05
@sfc-gh-jcarroll
Copy link
Contributor

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 session._conn._conn.is_closed() in Snowpark, so I think SnowparkConnection.session can potentially still get stale. We can follow up with Yijun & Snowpark team to get a solution for that in the next release, particularly if users are running into this issue (and seems decently likely they will).

@vdonato
Copy link
Collaborator Author

vdonato commented Apr 22, 2023

Just wrapped up a last testing pass myself -- going to click the submit button!

@vdonato vdonato merged commit abef3a3 into develop Apr 22, 2023
@vdonato vdonato deleted the feature/st.experimental_connection branch April 22, 2023 01:21
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 24, 2023
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants