Update generated client code #215

jhamon · 2023-10-11T14:47:35Z

Problem

We need to update the generated core module to reflect upcoming API changes

Solution

Migrate from v5 to v7 openapi generator to avoid generating code with deprecated urllib3 usage.
Regenerate contents of pinecone.core using pinecone-protos (private) repo.
Refactor to move all non-generated items out from under pinecone.core. These never should have been under pinecone.core in the first place.
- Added pinecone.utils
- Added pinecone.grpc to hold GRPCIndex and related code.
Add new dependencies required by updated generated code (pydantic for typechecking, aenum for enums)
Disable legacy integ tests which won't work until we refactor configuration.

Type of Change

New feature (non-breaking change which adds functionality)

jhamon · 2023-10-16T15:59:24Z

pyproject.toml

+loguru = "0.5.3"
 typing-extensions = ">=3.7.4"
 dnspython = ">=2.0.0"
 python-dateutil = ">=2.5.3"
-urllib3 = ">=1.21.1"
+urllib3 = "1.25.3"
 tqdm = ">=4.64.1"
 numpy = ">=1.22.0"
 grpcio = ">=1.44.0"
 grpc-gateway-protoc-gen-openapiv2 = "0.1.0"
 googleapis-common-protos = ">=1.53.0"
 lz4 = ">=3.1.3"
 protobuf = "~=3.20.0"
+aenum = "3.1.11"
+pydantic = "1.10.12"


Updated and added dependencies to satisfy the requirements of the generated openapi code. Both have permissive open source licenses, so no issue there.

Could potentially use pydantic in a similar way to typebox in the TS client if we wanted to add better runtime validation and error handling, if I'm understanding things correctly.

jhamon · 2023-10-16T16:02:09Z

tests/integ/test_index.py


+@pytest.mark.skip()


There are a lot of black changes in this file, but all the tests are currently marked skip because they can't pass until we overhaul configuration.

The urllib3_mock we are depending on throws a lot of deprecation warnings and appears to be abandoned so we should figure out some other tool there if we need to do request mocking in the future. But in reality I don't think we need to mock requests at that level if we refactor our tests into either pure unit tests or real integration tests against a live environment.

jhamon · 2023-10-16T16:03:08Z

tests/unit/test_config.py

@@ -6,210 +6,204 @@
 import tempfile
 import os

+
 @pytest.fixture(autouse=True)


I removed references to project name from these config tests.

jhamon · 2023-10-16T16:10:56Z

tests/unit/test_grpc_index.py

I don't think much changed inhere besides formatting.

jhamon · 2023-10-16T16:12:02Z

tests/unit/test_index.py

 import pinecone
+from pinecone import UpsertRequest, Vector


I added these imports to make the tests a bit less verbose.

jhamon · 2023-10-16T16:15:01Z

tests/unit/test_index.py

+        chunks = [
+            [Vector(id="vec1", values=self.vals1, metadata=self.md1)],
+            [Vector(id="vec2", values=self.vals2, metadata=self.md2)],
+        ]
+        with pinecone.Index("example-index", pool_threads=30) as index:
+            mocker.patch.object(index._vector_api, "upsert", autospec=True)
+
            # Send requests in parallel
            async_results = [
-                index.upsert(vectors=ids_vectors_chunk, namespace="ns", async_req=True)
-                for ids_vectors_chunk in chunks
+                index.upsert(vectors=ids_vectors_chunk, namespace="ns", async_req=True) for ids_vectors_chunk in chunks
            ]
            # Wait for and retrieve responses (this raises in case of error)
            [async_result.get() for async_result in async_results]

-            Endpoint.__call__.assert_any_call(
-                index._vector_api.upsert,
-                pinecone.UpsertRequest(vectors=[
-                    pinecone.Vector(id='vec1', values=self.vals1, metadata=self.md1),
-                ],
-                    namespace='ns'),
-                async_req=True
+            index._vector_api.upsert.assert_any_call(
+                UpsertRequest(
+                    vectors=[
+                        Vector(id="vec1", values=self.vals1, metadata=self.md1),
+                    ],
+                    namespace="ns",
+                ),
+                async_req=True,
            )

-            Endpoint.__call__.assert_any_call(
-                index._vector_api.upsert,
-                pinecone.UpsertRequest(vectors=[
-                    pinecone.Vector(id='vec2', values=self.vals2, metadata=self.md2),
-                ],
-                    namespace='ns'),
-                async_req=True
+            index._vector_api.upsert.assert_any_call(
+                UpsertRequest(
+                    vectors=[
+                        Vector(id="vec2", values=self.vals2, metadata=self.md2),
+                    ],
+                    namespace="ns",
+                ),
+                async_req=True,
            )


This test was pretty heavily refactored to mock out at the vector_api level instead of reaching in to touch the Endpoint internals, which got changed between v5 and v7 openapi versions.

jhamon · 2023-10-16T16:17:03Z

tests/unit/test_index.py

 import pytest
 import warnings

-from pinecone.core.client.api_client import Endpoint


This export is not available, and most substantive changes in this test file are to refactor away from depending on it.

Good call, thank you.

jhamon · 2023-10-16T16:18:16Z

.github/actions/bump-version/action.js

@@ -6,10 +6,10 @@ function bumpVersion(currentVersion, bumpType, prerelease) {
  if (prerelease) {
    newVersion = `${newVersion}.${prerelease}`;
  }
-  core.setOutput('previous_version', currentVersion)
-  core.setOutput('previous_version_tag', `v${currentVersion}`)
+  core.setOutput('previous_version', currentVersion);


Maybe black did this? Not really my intention, sorry.

No problem, I think it was prettier / lint since it's a .js file. I had the same thing happen when I was working on workflows: https://github.com/pinecone-io/pinecone-python-client/pull/216/files

austin-denoble

Thank you for all the work that went into getting the core regenerated and updated. Overall this looks good, it seems like we'll need a few tickets for follow-up here around re-enabling integration tests, and replacing urllib3 if needed for mocking.

It looks like the pdoc build is also failing due to a couple import related issues. I think this is due to it using dynamic analysis when extracting docstrings, so Python modules are executed/imported when it runs:

ModuleNotFoundError: No module named 'pinecone.core.client.model'
ImportError: cannot import name 'Endpoint' from 'pinecone.core.client.api_client' (/home/runner/work/pinecone-python-client/pinecone-python-client/pinecone/core/client/api_client.py)

I can pull things down and maybe take a look, we could also skip / disable for now since we're in a feature branch. I think this also still may need a rebase against origin/spruce to pull in the workflow changes.

austin-denoble · 2023-10-16T16:44:58Z

.github/actions/bump-version/action.js

@@ -6,10 +6,10 @@ function bumpVersion(currentVersion, bumpType, prerelease) {
  if (prerelease) {
    newVersion = `${newVersion}.${prerelease}`;
  }
-  core.setOutput('previous_version', currentVersion)
-  core.setOutput('previous_version_tag', `v${currentVersion}`)
+  core.setOutput('previous_version', currentVersion);


No problem, I think it was prettier / lint since it's a .js file. I had the same thing happen when I was working on workflows: https://github.com/pinecone-io/pinecone-python-client/pull/216/files

austin-denoble · 2023-10-16T16:46:28Z

Makefile

+
+upload-spruce:


You'll need to add upload-spruce to the .PHONY: ... line at the top so it's callable.

austin-denoble · 2023-10-16T16:47:39Z

pinecone/__init__.py

 from .config import *
 from .exceptions import *
-from .info import *


praise: Thanks for pulling these out. I had stripped them out of the docs as well because they're all internal and I don't think we expect external users to have access to these.

austin-denoble · 2023-10-16T17:05:49Z

pyproject.toml

+loguru = "0.5.3"
 typing-extensions = ">=3.7.4"
 dnspython = ">=2.0.0"
 python-dateutil = ">=2.5.3"
-urllib3 = ">=1.21.1"
+urllib3 = "1.25.3"
 tqdm = ">=4.64.1"
 numpy = ">=1.22.0"
 grpcio = ">=1.44.0"
 grpc-gateway-protoc-gen-openapiv2 = "0.1.0"
 googleapis-common-protos = ">=1.53.0"
 lz4 = ">=3.1.3"
 protobuf = "~=3.20.0"
+aenum = "3.1.11"
+pydantic = "1.10.12"


Could potentially use pydantic in a similar way to typebox in the TS client if we wanted to add better runtime validation and error handling, if I'm understanding things correctly.

austin-denoble · 2023-10-16T17:08:51Z

tests/unit/test_index.py

 import pytest
 import warnings

-from pinecone.core.client.api_client import Endpoint


Good call, thank you.

austin-denoble · 2023-10-16T17:10:00Z

tests/unit/test_index.py

-        pinecone.init(api_key='example-key')
-        self.index = pinecone.Index('example-name')
-
-    def test_upsert_numpy_deprecation_warning(self, mocker):


question: Are we still planning on deprecating numpy? I've seen references to that in the codebase and this seems to confirm that.

If so we can probably add a ticket.

Yeah, we should remove it before releasing spruce. I'll add a ticket.

austin-denoble · 2023-10-16T17:14:56Z

pinecone/grpc/index_grpc.py

@@ -15,8 +15,8 @@



This will be good for actually adding some documentation around the GRPC bits, thank you. 👍

jhamon requested a review from austin-denoble October 11, 2023 15:13

jhamon force-pushed the jhamon/spruce-codegen branch 2 times, most recently from 54bd898 to b758766 Compare October 11, 2023 20:48

jhamon commented Oct 16, 2023

View reviewed changes

austin-denoble approved these changes Oct 16, 2023

View reviewed changes

Squash changes to simplify rebase

199c7f4

jhamon force-pushed the jhamon/spruce-codegen branch from 0e79c67 to 199c7f4 Compare October 16, 2023 21:59

jhamon merged commit 17e0566 into spruce Oct 16, 2023
5 checks passed

jhamon deleted the jhamon/spruce-codegen branch October 16, 2023 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update generated client code #215

Update generated client code #215

jhamon commented Oct 11, 2023 •

edited

Loading

jhamon Oct 16, 2023

austin-denoble Oct 16, 2023

jhamon Oct 16, 2023 •

edited

Loading

jhamon Oct 16, 2023

jhamon Oct 16, 2023

jhamon Oct 16, 2023

jhamon Oct 16, 2023

jhamon Oct 16, 2023

austin-denoble Oct 16, 2023

jhamon Oct 16, 2023

austin-denoble Oct 16, 2023

austin-denoble left a comment

austin-denoble Oct 16, 2023

austin-denoble Oct 16, 2023

austin-denoble Oct 16, 2023

austin-denoble Oct 16, 2023

austin-denoble Oct 16, 2023

austin-denoble Oct 16, 2023

jhamon Oct 16, 2023

austin-denoble Oct 16, 2023


		@pytest.mark.skip()


		upload-spruce:

Update generated client code #215

Update generated client code #215

Conversation

jhamon commented Oct 11, 2023 • edited Loading

Problem

Solution

Type of Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon commented Oct 11, 2023 •

edited

Loading

jhamon Oct 16, 2023 •

edited

Loading