Create serverless index functionality #3
Conversation
| pub async fn create_serverless_index( | ||
| &self, | ||
| params: CreateServerlessIndexRequest, | ||
| ) -> Result<IndexModel, PineconeError> { | ||
| let create_index_request = self.create_serverless_index_req(params); | ||
| match openapi::apis::manage_indexes_api::create_index( | ||
| &self.openapi_config(), | ||
| create_index_request?, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(index) => Ok(index), | ||
| Err(e) => Err(PineconeError::CreateIndexError { openapi_error: e }), | ||
| } | ||
| } | ||
|
|
||
| pub fn create_serverless_index_req( | ||
| &self, | ||
| params: CreateServerlessIndexRequest, | ||
| ) -> Result<CreateIndexRequest, PineconeError> { | ||
| let metric_enum = match ¶ms.metric { | ||
| Some(metric) => match metric.as_str() { | ||
| "cosine" => Ok(Some(Metric::Cosine)), | ||
| "euclidean" => Ok(Some(Metric::Euclidean)), | ||
| "dotproduct" => Ok(Some(Metric::Dotproduct)), | ||
| _ => Err(PineconeError::InvalidMetricError { | ||
| metric: metric.clone(), | ||
| }), | ||
| }, | ||
| None => Ok(Some(Metric::Cosine)), | ||
| }?; |
There was a problem hiding this comment.
Is create_serverless_index the function users would hit, and create_serverless_index_req is more of an internal function?
If so, at a minimum we should only make pub the one we want them to hit, and the other should be left private to the crate.
We might want to go further and separate the public and private functions completely, but I'd save that for later.
There was a problem hiding this comment.
Yes, good point -- create_serverless_index should be the only one that users see. Will change that! When you say completely separate the public and private functions, do you mean separate them into different files?
There was a problem hiding this comment.
Yes into different files probably, but also just logically separate them, like maybe you have a separate type where the internal impls live
…ted struct/enum for spec, metric, cloud
| // constructs CreateIndexParams from CreateIndexParamsBuilder fields | ||
| pub fn build(self) -> Result<CreateIndexParams, PineconeError> { | ||
| // required parameters | ||
| let name = self.name.ok_or(PineconeError::MissingNameError)?; | ||
| let dimension = self.dimension.ok_or(PineconeError::MissingDimensionError)?; | ||
| let spec = self.spec.ok_or(PineconeError::MissingSpecError)?; | ||
|
|
||
| Ok(CreateIndexParams { | ||
| name, | ||
| dimension, | ||
| metric: self.metric.unwrap_or(Metric::Cosine), | ||
| spec, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
re: the previous comment about builder
it's better for users to discover the "true" interface at compile time vs runtime
There was a problem hiding this comment.
This makes sense. Part of why I liked the builder was because I thought it provided flexibility for users to not need to worry about variable order in the new function, though I guess they can also just construct the struct directly. Do you think it would make sense to have all of these construction methods so the user can choose what is best for them, or is it better to just limit to one or two?
There was a problem hiding this comment.
In my opinion, it's probably better to limit it to one way of doing things for now. If I'm understanding Silas, this would make things easier from an implementation perspective and allowing users to understand the interface at compile time.
There was a problem hiding this comment.
Do you think it would make sense to have all of these construction methods so the user can choose what is best for them, or is it better to just limit to one or two
No, that kind of "choose your own adventure" interface can lead to confusion for someone trying to figure out how to use it.
At a high level, I think we should be opinionated and design the interface that we think best expresses and supports the semantics and requirements of the underlying operation.
Putting that into practice:
- The interface is clearly defined, and its immediately apparent which parameters are required.
- Rely on compiler/strong types to guide the customer to satisfy the interface (their code won't compile until they satisfy the req'd params with valid values)
- Optional params are clear in the interface
- It's easy for customers to discover optional params (either as builder functions or as Option params). Either way it should be easy for them to set the optional params to valid values with type support from the compiler
There was a problem hiding this comment.
Really coming along, nice work! Given @ssmith-pc's feedback around the builder pattern, I think we should maybe think about going functional arguments instead if that makes sense.
There was a problem hiding this comment.
Apologies for the conflicting code reviews. I see @ssmith-pc recommended the builder pattern in an earlier review. This isn't very idiomatic in Rust code.
There are some modules and structs here that seem unnecessary. I'd also consider flattening the Pinecone struct a bit to not have nested "Config" and "Configuration" structs in it. Just have the attributes directly on the struct itself.
It seems the "openapi configuration" can be derived from the "Config" of the Pinecone struct. It should just do that when needed instead of storing a separate attribute.
| spec, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why have a separate struct for params instead of these attributes directly on the create_index fn?
There was a problem hiding this comment.
This is something we were discussing and we still aren't entirely sure what is better. Mainly we thought that passing all parameters directly into the function could be messy if there are a lot of parameters (such as for creating the pod index) and the user needs to keep track of the parameter order in the function, but I'm not sure how much of a problem this really is.
| control_plane_host: None, | ||
| additional_headers: None, | ||
| source_tag: None, | ||
| } |
There was a problem hiding this comment.
This is equivalent to PineconeBuilder::default() if you derive Default on the struct.
pinecone_sdk/src/pinecone.rs
Outdated
| control_plane_host: Option<String>, | ||
| additional_headers: Option<HashMap<String, String>>, | ||
| source_tag: Option<String>, | ||
| } |
There was a problem hiding this comment.
I saw @ssmith-pc mention the builder pattern in an earlier review and apologies for contradicting here.
This pattern isn't very idiomatic in Rust code. Users should just create the Pinecone struct directly with the required attributes.
|
I'd also just add, use modules a bit more sparingly. There probably shouldn't be modules for every action. It's ok to put the implementations in the "client" module (or whatever name.) I'm also guessing the overall |
There was a problem hiding this comment.
Agreed with @ssmith-pc , I think this looks good in terms of moving forward. Thanks for all the iteration and attention to detail around feedback!
| Err(e) => Err(PineconeError::CreateIndexError { openapi_error: e }), | ||
| } | ||
| } | ||
| /// Lists all indexes. |
There was a problem hiding this comment.
Nice job on the docstring. 👍
Problem
The SDK needs to have a way to easily create a serverless index, but this was not implemented yet.
Solution
We wrote a
create_serverless_index()function which takes in the index name, dimension, metric, cloud, and region. It then makes a call to the OpenAPIcreate_serverless_index()endpoint.Type of Change
Test Plan
We have test cases which should pass.