Set up data plane #20

emily-emily · 2024-07-02T14:59:37Z

Problem

Data plane was not set up.

Solution

Sets up the build process to generate gRPC code for data plane. We also have a basic implementation of upsert that calls the generated function.

This PR also restructures some code to include the implementation of PineconeClient (data.rs and control.rs) under pinecone.

The protobuf file for dataplane is currently committed to the repository due to difficulty with ci; it was originally referenced in a submodule but there is some tricky stuff with auth and submodules in GitHub Actions.

Some dependencies are included in pinecone_sdk/imports/google/api for protoc.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

Existing test cases pass.

pinecone_sdk/src/pinecone/mod.rs

pinecone_sdk/src/utils/errors.rs

pinecone_sdk/tests/integration_test.rs

pinecone_sdk/src/pinecone/data.rs

austin-denoble

Really solid work here! I left some comments for consideration, specifically around dealing with the Index host and how we're generating code.

I think we can consider a lot of this as follow up PRs / work and keep this PR focused on getting the data plane established like you've done so far. Another pass by @haruska and @ssmith-pc would be appreciated if possible. 🙏

pinecone_sdk/imports/google/api/annotations.proto

README.md

.github/workflows/ci.yaml

austin-denoble · 2024-07-10T16:35:51Z

pinecone_sdk/src/pinecone/data.rs

+        let index_host = self.describe_index(name).await?.host;
+        // prepend with "https://" if not already present
+        let index_host = if index_host.starts_with("https://") {
+            index_host
+        } else {
+            format!("https://{}", index_host)
+        };


I believe leaving off the schema and applying the :443 port is how we can specify https, right? At least, we're doing that in a couple other clients rather than using a schema for gRPC. We even strip out the https if it's present in a few cases.

Python - https://github.com/pinecone-io/pinecone-python-client/blob/8b7e7de50e72248de683a24ada024cf18e6cdce3/pinecone/grpc/base.py#L87-#L89

Go - https://github.com/pinecone-io/go-pinecone/blob/a9af78e2e83f4d3a2506314de7519b483dbd426e/pinecone/index_connection.go#L39

Would it make sense to just do the same thing here?

Also @haruska made changes to the Python SDK recently to support allowing users to provide their own port if necessary: pinecone-io/pinecone-python-client#362

@emily-emily had mentioned it sounds like tonic complains if there's not a schema applied, which seems odd. 🤔

pinecone_sdk/src/pinecone/data.rs

austin-denoble · 2024-07-10T17:31:37Z

protos/data_2024-07.proto

@@ -0,0 +1,404 @@
+syntax = "proto3";


This may be a question for @haruska. How would we approach moving the proto file itself out of the codebase in the future?

From what I understand, the Rust code is generated from protos as a part of cargo build, so we're not checking the generated code into source like we do with the other clients. Would there be some way to do that with Rust rather than having it done as part of cargo build? Is it preferable to leave it as-is?

I'm hoping we publish proto files and this becomes a non-issue.

We could also have a git submodule to the APIs repo. This is not ideal as the project will not build except for our own GH actions and for folks with access to that private repo.

Another option is to move the tonic_build process to its own binary and only run it on an updated proto file. There is an option to set the codegen output folder. We could simply check in the generated code similar to other SDKs.

We currently do have a submodule to the APIs repo that we use for the OpenAPI code, but since we build protos through cargo build we need access in the CI. We weren't sure how to authenticate for it other than including somebody's personal access token.

Checking in the generated code could work, should we experiment with that in a different PR or try to fix it here?

I think experimenting with checking in the generated output in a future PR would be fine, you could create a ticket to keep tabs on that as well. 👍

austin-denoble

Very nice work on this, I really like this approach for allowing configuration of the index connection. Overall I think we're ready to move forward from here, great work again getting this all established.

pinecone_sdk/src/pinecone/data.rs

austin-denoble · 2024-07-12T18:19:04Z

pinecone_sdk/src/pinecone/data.rs

+            namespace: namespace.unwrap_or_default(),
+        };
+
+        let response = match self.connection.upsert(request).await {


Would it be possible to use .map_err here?

agree, also not sure we need the clone?

get_ref() returns a reference to the inner object; if I understand correctly, this won't work as the original variable with ownership of the whole thing goes out of scope at the end of this function and our returned reference won't be valid anymore. Are there performance concerns with the clone?

austin-denoble · 2024-07-12T18:20:34Z

pinecone_sdk/src/pinecone/data.rs

+    pub async fn index(
+        &self,
+        host: &str,
+        secure: Option<bool>,
+        port: Option<u32>,
+    ) -> Result<Index, PineconeError> {


This function may be a good candidate for some unit test coverage to validate all the different permutations of manipulating the host if we don't have any.

Could be done as a separate piece of work in a follow up.

ssmith-pc · 2024-07-12T18:45:57Z

pinecone_sdk/src/pinecone/data.rs

+        let endpoint = if endpoint.contains(":") {
+            endpoint.to_string()
+        } else {
+            format!("{}:{}", endpoint, port)
+        };


Based on previous, won't endpoint always contain a :?

Maybe endpoint isn't the right name, is there a different name that would communicate it better? This is supposed to store a working copy of something between the host and the endpoint where we add the scheme or port if they aren't provided. The final endpoint should always contain a :.

I just mean that the end result of running this:

let endpoint = if endpoint.contains("://") { endpoint.to_string() } else { let scheme = if secure { "https" } else { "http" }; format!("{}://{}", scheme, endpoint) };

will result in endpoint definitely having a : in it, because it's ensuring that there is a scheme with ://

Ahh I see, thanks for clarifying. I'm thinking of checking for two colons, since after adding the scheme we know there will be at least 1 colon; if the port was specified, there would now be 2 of them. Does that make sense?

The logic makes sense, but it doesn't really "feel" like the right implementation to me. Using ends_with would be an improvement: https://doc.rust-lang.org/std/primitive.str.html#method.ends_with

and starts_with would probably be better in the other case

I added some code to test these with regex ("^[a-zA-Z]+://" and ":\d+$")

ssmith-pc · 2024-07-12T18:49:08Z

pinecone_sdk/src/pinecone/data.rs

+impl PineconeClient {
+    /// Target an index for data operations.
+    ///
+    /// ### Arguments
+    /// * `host: &str` - The host of the index to target.
+    /// * `secure: Option<bool>` - Whether to use a secure connection. If not provided, defaults to `true`.
+    /// * `port: Option<u32>` - The port of the index to target. If not provided, defaults to `443`.
+    ///
+    /// ### Return
+    /// * `Result<Index, PineconeError>` - A Pinecone index object.
+    ///
+    /// ### Example
+    ///
+    /// ```no_run
+    /// use pinecone_sdk::pinecone::PineconeClient;
+    /// # use pinecone_sdk::utils::errors::PineconeError;
+    ///
+    /// # #[tokio::main]
+    /// # async fn main() -> Result<(), PineconeError>{
+    /// let pinecone = PineconeClient::new(None, None, None, None).unwrap();
+    ///
+    /// let index = pinecone.index("index-name", None, None).await.unwrap();
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub async fn index(
+        &self,
+        host: &str,
+        secure: Option<bool>,
+        port: Option<u32>,
+    ) -> Result<Index, PineconeError> {


Can you remind me, what is the situation when endpoint either will or won't already be constructed with a scheme? And what is the use case for not using a secure connection?

Calling describe_index returns the host without the scheme, so I think it would make sense for us to accommodate that along with the case that the scheme is included anyway.

As for the secure connection, I believe this is mainly for Pinecone local. We haven't been able to connect to prod with http but it sounds like there is a use case for it in a local dev setting.

Gotcha. Since this is the primary interface users will use to connect to Pinecone, it might be confusing to present this as a choice they have to make when 99.9% of the time they should just connect securely.

How do we handle this in other SDKs? I vaguely remember we all discussed this last week with @austin-denoble and @haruska so apologies if this is a rehash of a decision that was made.

We had a discussion about changing the signature to take the host, and along with the host we included secure and port. Would it make sense to remove secure and port since they can be included in host? @austin-denoble @haruska

Like Emily mentioned, the secure flag is primarily to allow development against something like Pinecone local. I think you're right though that this is probably not an option we want to provide more generally at the moment.

In Python there's some similar logic, including a secure flag which is used to configure the gRPC client if you pass a config: https://github.com/pinecone-io/pinecone-python-client/blob/8b7e7de50e72248de683a24ada024cf18e6cdce3/pinecone/grpc/base.py#L101

However, this is kind of buried, and I don't think we really expect users to pass a custom gRPC config. I know Jen has also mentioned wanting to remove it from the interface entirely.

@haruska also added logic for allowing a custom port: https://github.com/pinecone-io/pinecone-python-client/blob/main/pinecone/grpc/base.py#L86-L90

I think we may want to go with something similar to what Jason implemented here if possible.

pinecone_sdk/src/pinecone/data.rs

ssmith-pc · 2024-07-12T19:16:07Z

pinecone_sdk/src/pinecone/data.rs

+        let endpoint = Channel::from_shared(host.to_string())
+            .map_err(|e| PineconeError::ConnectionError { inner: Box::new(e) })?
+            .tls_config(tls_config)
+            .map_err(|e| PineconeError::ConnectionError { inner: Box::new(e) })?;
+
+        let channel = endpoint
+            .connect()
+            .await
+            .map_err(|e| PineconeError::ConnectionError { inner: Box::new(e) })?;


ssmith-pc · 2024-07-12T19:16:33Z

pinecone_sdk/src/pinecone/data.rs

+            .get_ref()
+            .clone();


why do we need to clone?

Without the clone this would return &UpsertError, and I'm not sure that the reference would still be valid after the response object goes out of scope. Is there another way around this?

I think we can use into_inner to avoid the clone and access the inner response object:

let response = self .connection .upsert(request) .await .map_err(|e| PineconeError::UpsertError { inner: Box::new(e) })? .into_inner();

ssmith-pc

LGTM! Thanks for addressing all the feedback

austin-denoble

Looks great, fantastic work! 🚢

emily-emily and others added 10 commits June 26, 2024 13:40

First iteration of generating dataplane from protofile

de37c4b

Refactor files

034bfff

Switch to tonic

977f72d

Remove proto-builder

daa97b0

Merge branch 'main' into emily/generate-dataplane-from-proto

0213d42

Add code framework in data.rs

33a913b

Update ci.yaml with protoc

5fd32c2

Add some connection code, does not connect successfully

af73d57

Make successful connection - added features to Cargo.toml

ab5f8a3

changed some function/struct visibility, removed some unwrap()

4828d7e

emily-emily assigned emily-emily and ericapywang Jul 2, 2024

emily-emily requested review from austin-denoble, haruska and jhamon July 2, 2024 14:59

ericapywang and others added 11 commits July 2, 2024 11:16

Merge branch 'main' into emily/generate-dataplane-from-proto

e2abba3

fixed build

524dc5e

Add submodule checkout to ci

d4a7b4a

Fix ci.yaml syntax

6618815

Update ci.yaml

8bcc95b

Commit proto file and update ci.yaml

c4918b9

Fix ci.yaml syntax

e964a12

added basic test for index

b43f075

delete pinecone_connection.rs file

3f7b962

change error type to UpsertError

c0f5fe0

Restructure to include control and data under pinecone

fbad4c2

emily-emily commented Jul 2, 2024

View reviewed changes

pinecone_sdk/src/pinecone/mod.rs Show resolved Hide resolved

pinecone_sdk/src/utils/errors.rs Outdated Show resolved Hide resolved

pinecone_sdk/tests/integration_test.rs Outdated Show resolved Hide resolved

pinecone_sdk/src/pinecone/data.rs Show resolved Hide resolved

emily-emily marked this pull request as ready for review July 2, 2024 21:11

emily-emily and others added 2 commits July 3, 2024 14:53

Update upsert to take namespace

258c17d

Merge branch 'main' into emily/generate-dataplane-from-proto

3569eb7

emily-emily and others added 3 commits July 8, 2024 09:39

Use tonic::Status

6a9f6b4

Merge branch 'main' into emily/generate-dataplane-from-proto

e010b2e

removed duplicate error defn

0a4249e

austin-denoble reviewed Jul 10, 2024

View reviewed changes

ericapywang and others added 4 commits July 10, 2024 13:48

Merge branch 'main' into emily/generate-dataplane-from-proto

4c40d1b

Move google protos and fix documentation

ebcd7ab

user inputs host instead of index name

1fbc1a3

Add scheme and port options to connection

ce995f4

ericapywang requested review from austin-denoble, ericapywang and ssmith-pc and removed request for ericapywang July 12, 2024 14:44

austin-denoble approved these changes Jul 12, 2024

View reviewed changes

ssmith-pc reviewed Jul 12, 2024

View reviewed changes

pinecone_sdk/src/pinecone/data.rs Outdated Show resolved Hide resolved

Use map_err and unwrap_or

d82c21c

ssmith-pc reviewed Jul 12, 2024

View reviewed changes

emily-emily added 5 commits July 15, 2024 10:22

Use regex for scheme and port

cfd0d2b

Fix default port for index

0f6451b

Remove secure and port options

3fa1c4c

Use static regex object

5ecb4b5

Use into_inner

c091338

ericapywang requested review from austin-denoble and ssmith-pc July 15, 2024 18:03

ssmith-pc approved these changes Jul 15, 2024

View reviewed changes

austin-denoble approved these changes Jul 15, 2024

View reviewed changes

emily-emily merged commit df6f233 into main Jul 15, 2024

emily-emily deleted the emily/generate-dataplane-from-proto branch July 15, 2024 20:00

Set up data plane #20

Set up data plane #20

Uh oh!

Conversation

emily-emily commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Type of Change

Test Plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

austin-denoble left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austin-denoble left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emily-emily Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emily-emily commented Jul 2, 2024 •

edited

Loading

emily-emily Jul 15, 2024 •

edited

Loading