Skip to content
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.

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

Refactoring and updates for 2024-10 API spec #401

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Oct 21, 2024

Problem

Needed to consume the 2024-10 API spec.

Solution

The spec updates involved many changes to operation ids. This should not touch the user experience, but does mean this diff is somewhat larger than usual due to the large number of tests for existing functionality that needed to be adjusted for the new names:

  • upsert > upsert_vector
  • fetch > fetch_vectors
  • query > query_vectors
  • delete > delete_vectors
  • list > list_vectors
  • start_import > start_bulk_import
  • describe_import > describe_bulk_import
  • etc

Other changes

Cleanup prerelease code no longer needed

  • Deleted pinecone/core_ea/ which was the home of some code generated off of the 2024-10 spec when bulk import functionality was in a pre-release state. We no longer need this. That's why this PR has 21k lines removed.

Changes to generation process

  • Placed a copy of classes such as ApiClient, Endpoint, ModelNormal, etc which were previously generated into a new folder, pinecone/openapi_support. Even though these used to be generated, they contain no dynamic content. So taking them out of the generation process should make it significantly easier to work on core improvements to the UX and performance of the generated code, since the source of truth will no longer be a mustache file (FML)
  • Updated the codegen script ./codegen/build-oas.sh 2024-10 false to delete instead of de-dupe copies of shared classes like ApiClient, and edit model+api files that continue to be generated to find the classes they need in the new openapi_support folder.

mypy fixes

  • I needed to make adjustments in some openapi_support classes to satisfy the mypy type checker (pinecone/core has been ignored in the past, and our goal should be to eventually have 100% of code typechecked).
  • Wrote some initial unit tests to characterize some of the behavior in ApiClient. The tests were to help give me more confidence I wasn't breaking something along the way while making adjustments for mypy.
  • Added a dev dependency on a package with types for datetime

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)

@jhamon jhamon marked this pull request as ready for review October 21, 2024 13:43
settings={
"response_type": ({str: (bool, dict, float, int, list, str, none_type)},),
"auth": ["ApiKeyAuth"],
"endpoint_path": "/bulk/imports/{id}",
"operation_id": "cancel_import",
"operation_id": "cancel_bulk_import",
Copy link

Choose a reason for hiding this comment

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

I thought we were not using the word "bulk" in the clients anymore wrt to this endpoint? https://pinecone-io.slack.com/archives/C05MYEX7T2L/p1728935415605249?thread_ts=1728935400.761069&cid=C05MYEX7T2L

cc @austin-denoble

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is generated code, so not really publicly exposed. I think this is fine, there's a similar story in other SDKs since the spec contains "bulk" in a few places.

Copy link

@aulorbe aulorbe left a comment

Choose a reason for hiding this comment

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

@@ -13,7 +13,7 @@ if [ "$is_early_access" = "true" ]; then
template_dir="codegen/python-oas-templates/templates5.2.0"
else
destination="pinecone/core/openapi"
modules=("control" "data")
modules=("db_control" "db_data")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is inference going to be a part of the plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, inference stuff comes in via plugin which is why you don't see anything about it here.

Copy link
Contributor

@rohanshah18 rohanshah18 left a comment

Choose a reason for hiding this comment

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

Left a question about inference. Otherwise it looks good, LGTM!

@jhamon jhamon force-pushed the jhamon/extract-static-files branch from f391f0f to 2a6a81f Compare October 25, 2024 19:48
@jhamon jhamon force-pushed the jhamon/extract-static-files branch from 2a6a81f to 5578543 Compare November 14, 2024 18:13
@jhamon jhamon changed the base branch from main to release-candidate/2024-10 November 14, 2024 18:14
@jhamon jhamon merged commit d6cb346 into release-candidate/2024-10 Nov 15, 2024
83 checks passed
@jhamon jhamon deleted the jhamon/extract-static-files branch November 15, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants