-
Notifications
You must be signed in to change notification settings - Fork 79
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
REST code generation for 2024-07 prerelease #361
Conversation
@@ -1,22 +1,32 @@ | |||
import time | |||
import warnings | |||
import logging | |||
from typing import Optional, Dict, Any, Union, List, cast, NamedTuple | |||
from typing import Optional, Dict, Any, Union, List, Tuple, cast, NamedTuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A decent amount of change was needed in this file due to changes in generated code related to index spec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't thoroughly review all the code changes for the generated code but overall I really like this approach to code-gen for the python client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work on this Jen, the approach feels clear and easy to follow especially given everything going on under the hood. 🚢
update_apis_repo() { | ||
echo "Updating apis repo" | ||
pushd codegen/apis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to have something similar for making sure the codegen/python-oas-templates
submodule is also up to date? I doubt it would have as much churn as the APIs repo, but just in case.
# Remove the docstring headers that aren't really correct in the | ||
# context of this new shared package structure | ||
find "$target_directory" -name "*.py" -print0 | xargs -0 -I {} sh -c 'sed -i "" "/^\"\"\"/,/^\"\"\"/d" "{}"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup! 👍
# Adjust import paths in every file | ||
find "${destination}" -name "*.py" | while IFS= read -r file; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also cool, very clever piece of scripting here, nice work.
Problem
We need to generate code off of the openapi specs stored in our apis repo. We'll need to massage the output a bit in order to avoid having a lot of confusing duplication of generated exception classes and other utils.
Solution
codegen
directory to act as the home for everything codegen related. Inside this directory there are several things:build-oas.sh
script which does everything to generate the code off the spec, adjust import paths, and place things into thepinecone
module package structure.The script does the following:
shared
package.sed
to adjust import paths to reflect the modified code structure.The surgery to extract shared code is more than just a cosmetic change, as without it you can end up with a lot of confusion in the areas of configuration and error handling due to there being multiple objects that despite being identical in name and functionality remain distinct from the perspective of language utils such as
isinstance
,.__class__
andexcept
.When the script is done running, the generated outputs have this structure:
What actually changed in here?
It seems the main substantive change is in how the
spec
info is expected to be passed when creating an index. So I had to write some additional tests and make some modifications to thecreate_index
method.Also, inference-related stuff is currently generated. But for now, all the wrapper implementations for that functionality are only available by installing a separate plugin, pinecone-plugin-inference.
Type of Change
Test Plan
To run the script, run
make generate-oas
. Before pushing, I ran tests locally withmake test-unit
andPINECONE_API_KEY='key' make test-integration
. This helped me catch a lot of small issues related to the sed rewrite of import paths.Want to see tests passing even though I changed quite a bit about how the generated code is structured.