Skip to content

Start work on the BaseConnection abstract class #6346

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

Start work on the BaseConnection abstract class #6346

vdonato merged 1 commit into from
Mar 23, 2023

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented Mar 20, 2023 •
edited
Loading

Notes:

📚 Context

As part of the st.experimental_connection feature, we'll be providing a base class for connection class
authors to be able to use to help them write their own custom connection classes (we'll also be using
this same base class in the first party connections that we provide).

This PR starts work on this class by adding an implementation + tests for the parts of the base class
that we know we definitely want. Some of the details of the BaseConnection API are still being ironed
out, and these things will be built in subsequent PRs.

  • What kind of change does this PR introduce?

    • Feature

🧪 Testing Done

  • Added/Updated unit tests

@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label Mar 20, 2023
@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from 1e30f1d to 2f2e8c7 Compare March 20, 2023 23:35
@vdonato vdonato force-pushed the vdonato/connection-productionization-2 branch from 4f4b606 to b236255 Compare March 20, 2023 23:36
@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from 2f2e8c7 to 95fe2e2 Compare March 21, 2023 21:59
@vdonato vdonato force-pushed the vdonato/connection-productionization-2 branch from b236255 to 4e1b8f2 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 👍

lib/streamlit/connections/base_connection.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.

nit: I assume this is supposed to be part of the public API or? We probably could also add a @gather_metrics("connection:get_secrets") here as well.

Copy link
Collaborator Author

@vdonato vdonato Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's part of the public API but is intended to be used by connection authors rather than end-users (it's useful for implementing a connect function within a concrete connector), so I'm not sure if we'll want metrics on this. Won't add it for now but can do so if folks on the data team think it'd be useful to do so

CC @blackary in case you have an opinion on how useful it'd be to collect data on this

lib/streamlit/connections/base_connection.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.

nit: same as above. If this is part of the public API, we could add @gather_metrics("connection:reset")

Copy link
Collaborator Author

@vdonato vdonato Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This one is another one that I don't think it'd be too useful to collect metrics on.

It's part of the public API and is meant to be used by people writing apps, but its intended usage is to be able to help implement error handling / retries, so I think we'd just end up indirectly measuring connection error rates in various Streamlit app deployments if we're collecting metrics for this. Will also check with the data team on this, though

@vdonato vdonato force-pushed the vdonato/connection-productionization-1 branch from 95fe2e2 to 89f7b7e Compare March 22, 2023 23:47
@vdonato vdonato force-pushed the vdonato/connection-productionization-2 branch from 4e1b8f2 to 0a30412 Compare March 22, 2023 23:50
Base automatically changed from vdonato/connection-productionization-1 to feature/st.experimental_connection March 23, 2023 00:14
@vdonato vdonato force-pushed the vdonato/connection-productionization-2 branch from 0a30412 to 1d22e71 Compare March 23, 2023 00:16
@vdonato vdonato merged commit 530bb99 into feature/st.experimental_connection Mar 23, 2023
@vdonato vdonato deleted the vdonato/connection-productionization-2 branch March 23, 2023 00:17
vdonato added a commit that referenced this pull request Mar 23, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
13e7c31
vdonato added a commit that referenced this pull request Mar 27, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
a80c80e
vdonato added a commit that referenced this pull request Mar 29, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
76b171b
vdonato added a commit that referenced this pull request Mar 31, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
5da9b2a
vdonato added a commit that referenced this pull request Mar 31, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
629bdca
vdonato added a commit that referenced this pull request Apr 4, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
818c203
vdonato added a commit that referenced this pull request Apr 6, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
e8e6e35
vdonato added a commit that referenced this pull request Apr 6, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
272dea4
vdonato added a commit that referenced this pull request Apr 10, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
5161946
vdonato added a commit that referenced this pull request Apr 11, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
1f05a97
vdonato added a commit that referenced this pull request Apr 15, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
11671aa
vdonato added a commit that referenced this pull request Apr 17, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
c6518fa
vdonato added a commit that referenced this pull request Apr 19, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
f2051ed
vdonato added a commit that referenced this pull request Apr 20, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
510a800
vdonato added a commit that referenced this pull request Apr 21, 2023
@vdonato
Start work on BaseConnection abstract class (#6346)
4caecf3
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