Skip to content

Add a skeleton implementation of st.experimental_connection #6332

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 1 commit into from
Mar 23, 2023
Merged

Add a skeleton implementation of st.experimental_connection #6332

vdonato merged 1 commit into from
Mar 23, 2023

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented Mar 17, 2023 •
edited
Loading

Note: This PR is going into feature/st.experimental_connection and not develop.

📚 Context

This PR kicks off the work to implement the real version of st.experimental_connection that was
prototyped in #6035. We start off by doing mostly uninteresting stuff:

  • Expose st.experimental_connection as part of the public API
  • Add a stub implementation of BaseConnection and use it to define types in connection_factory.py
  • Implement friendly error messages for when users are missing packages that a connection requires.
    • These are being added mostly for the benefit of the first-party connections we'll be providing.

What's currently implemented doesn't really do anything at the moment, but it does define the general
structure of the code we'll be adding for this feature.

The next PR will implement the BaseConnection class, and after that we'll add a few first-party connectors.

  • What kind of change does this PR introduce?

    • Feature

🧪 Testing Done

  • Added/Updated unit tests

@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from e785225 to e2217a3 Compare March 17, 2023 23:27
@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label Mar 17, 2023
@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from e2217a3 to 990c626 Compare March 17, 2023 23:43
vdonato
vdonato commented Mar 17, 2023
View reviewed changes
lib/streamlit/runtime/connection_factory.py Outdated
Copy link
Collaborator Author

@vdonato vdonato Mar 17, 2023

Choose a reason for hiding this comment

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

Note this intentionally differs from the prototype (where we pass a validate function to cache_resource) as I'm thinking about moving the underlying connection lifecycle to be managed within the connection class

lib/streamlit/runtime/connection_factory.py Outdated
Copy link
Collaborator Author

@vdonato vdonato Mar 17, 2023

Choose a reason for hiding this comment

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

This type: ignore and the one below are temporary and will be removed as more of this feature is implemented

@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch 2 times, most recently from 27f566e to 3d922c5 Compare March 18, 2023 00:14
@vdonato vdonato marked this pull request as ready for review March 18, 2023 00:19
@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from 3d922c5 to 1e30f1d Compare March 20, 2023 21:29
@vdonato vdonato mentioned this pull request Mar 20, 2023
Merged
2 tasks
@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch 2 times, most recently from 2f2e8c7 to 95fe2e2 Compare March 21, 2023 21:59
lukasmasuch
lukasmasuch approved these changes Mar 22, 2023
View reviewed changes
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 👍 only aspect that might need to be adapted is were the gather_metrics decorator is placed.

lib/streamlit/runtime/connection_factory.py Outdated
Copy link
Collaborator

@lukasmasuch lukasmasuch Mar 22, 2023

Choose a reason for hiding this comment

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

I assume this will only cover some packages for which we plan to provide native connection implementations? I was wondering if this is actually better placed inside the connection implementation to have it a bit more self-contained. But thinking about the implementation, it does seem to make it messier if we add try / except to all implementations.

Copy link
Collaborator Author

@vdonato vdonato Mar 22, 2023

Choose a reason for hiding this comment

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

Hm, yeah. I think it's pretty close as to which method would be cleaner, but I'd lean toward not requiring every first-party connection implementation to have to do this.

Will keep it as-is for now and potentially change how this works later

@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from 95fe2e2 to 89f7b7e Compare March 22, 2023 23:47
@vdonato vdonato merged commit 104d9e6 into feature/st.experimental_connection Mar 23, 2023
@vdonato vdonato deleted the vdonato/connection-productionization-1 branch March 23, 2023 00:14
vdonato added a commit that referenced this pull request Mar 23, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
a26aa4c
vdonato added a commit that referenced this pull request Mar 27, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
d540e67
vdonato added a commit that referenced this pull request Mar 29, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
49a7720
vdonato added a commit that referenced this pull request Mar 31, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
ced25c5
vdonato added a commit that referenced this pull request Mar 31, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
1049e89
vdonato added a commit that referenced this pull request Apr 4, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
dd830f5
vdonato added a commit that referenced this pull request Apr 6, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
a3291e5
vdonato added a commit that referenced this pull request Apr 6, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
94e9e3b
vdonato added a commit that referenced this pull request Apr 10, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
5cbd0df
vdonato added a commit that referenced this pull request Apr 11, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
610d7eb
vdonato added a commit that referenced this pull request Apr 15, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
ff4c675
vdonato added a commit that referenced this pull request Apr 17, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
0e5a65d
vdonato added a commit that referenced this pull request Apr 19, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
bfd6c04
vdonato added a commit that referenced this pull request Apr 20, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
4854898
vdonato added a commit that referenced this pull request Apr 21, 2023
@vdonato
Add a skeleton implementation of st.experimental_connection (#6332)
a886392
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukasmasuch lukasmasuch lukasmasuch approved these changes

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 @lukasmasuch