Downgrade to openapi 5.2, add template overrides #229

jhamon · 2023-10-26T23:10:11Z

Problem

Recent versions of openapi generator have added dependencies we don't want to take on, especially pydantic v1. Pydantic v2 has been out for a while and we think this is very likely to lead to dependency clashes for some users.

Solution

The reason we updated in the first place was to avoid generating code that emitted urllib3 deprecation notices. In this PR, I downgrade to the previously used 5.2.0 generator version and address the urllib3 error using template overrides in the generation step.

In this diff:

Go back to generating the core with openapi 5.2.0 (same version as before, e.g. on main)
- Everything under pinecone/core is generated using openapi and don't need to be specifically reviewed. This is like 90% of the files changed.
Use a template override in the generation step (handled in a private repo) to adjust the urllib3 getheaders() usage and avoid spamming those deprecation notices.
- Notice that generated rest.py is using correct syntax for getting headers in urllib3
Fix tests that broke along the way, mostly due to changes in package structure used by the generated code.
Remove dependencies from pyproject.toml that were added when previously going from openapi 5.2.0 => 7.0.1 (pydantic, aenum)

Type of Change

None of the above: Update generated code, change dependencies

Follow-up needed

Docs build broken. By the looks of it, it seems related to numpy. But I want to remove numpy in a follow-up diff anyway so I may merge with this build failing for now.

jhamon · 2023-10-30T19:01:47Z

pinecone/core/client/rest.py

        self.urllib3_response = resp
        self.status = resp.status
        self.reason = resp.reason
        self.data = resp.data

    def getheaders(self):
        """Returns a dictionary of the response headers."""
-        return self.urllib3_response.headers
+        return self.urllib3_response.getheaders()

    def getheader(self, name, default=None):
        """Returns a given response header."""
        return self.urllib3_response.headers.get(name, default)


This return self.urllib3_response.headers.get(name, default) is not showing as a change because it was handled correctly in the 7.0.1 generator that we are downgrading from. But the fact it is not changed means my template override was successfully used; without this template override, openapi 5.2.8 generates return self.urllib3_response.getheader(name, default) which causes a large amount of runtime deprecation notices to appear.

jhamon · 2023-10-30T19:09:02Z

pinecone/data/index.py

 def parse_query_response(response: QueryResponse, unary_query: bool):
    if unary_query:
-        response.results = None
+        response._data_store.pop("results", None)
    else:
-        response.matches = None
-        response.namespace = None
+        response._data_store.pop("matches", None)
+        response._data_store.pop("namespace", None)
    return response


I don't like this at all, but I brought it back from main to get some tests passing. It seems this thing is supposed to return a different data shape depending on what query parameters were used, which is a terrible design. But not something I want to address right now in this diff with all the generated changes.

jhamon · 2023-10-30T19:09:37Z

tests/unit/test_index.py

@@ -3,7 +3,6 @@
 import pytest

 import pinecone
-from pinecone.config import PineconeConfig


I noticed this wasn't being used, so I deleted it.

jhamon · 2023-10-30T19:12:27Z

tests/unit/test_control.py

@@ -24,43 +23,24 @@ def test_passing_host(self):
        (-1, [{"status": {"ready": False}}], 0, 0),
    ])
    def test_create_index_with_timeout(self, mocker, timeout_value, describe_index_responses, expected_describe_index_calls, expected_sleep_calls):
-        mocker.patch.object(IndexOperationsApi, 'describe_index', side_effect=describe_index_responses)


The IndexOperationsApi class generated by openapi 5.2.8 can't be mocked in the normal way because almost all of its methods are attributes defined at runtime from within __init__(). I'm not a mocker guru by any means, but the only way I could get this working was to grab the instance of IndexOperationsApi being used by the pinecone client and monkey patch that directly. 🤦

That's seems like an extremely annoying way to set up a class. Ultimately this seems fine, we do what we can with mocks. 🧪

tests/unit/test_control.py

austin-denoble

Looks good, thanks a ton for taking this on to get things back in place to avoid the dependency hell problem.

tests/unit/test_control.py

austin-denoble · 2023-10-30T21:59:51Z

tests/unit/test_control.py

@@ -24,43 +23,24 @@ def test_passing_host(self):
        (-1, [{"status": {"ready": False}}], 0, 0),
    ])
    def test_create_index_with_timeout(self, mocker, timeout_value, describe_index_responses, expected_describe_index_calls, expected_sleep_calls):
-        mocker.patch.object(IndexOperationsApi, 'describe_index', side_effect=describe_index_responses)


That's seems like an extremely annoying way to set up a class. Ultimately this seems fine, we do what we can with mocks. 🧪

austin-denoble · 2023-10-30T22:24:01Z

The docs build failure is confusing to me since we're also running poetry install for the testing steps and that seems to complete fine, and I don't think they use differing dependencies.

pinecone/config/openapi.py

rohanshah18

LGTM, thanks for making this change!

jhamon added 3 commits October 26, 2023 19:09

Downgrade to openapi 5.2, with template overrides

7761ab3

Remove 7.0.1 deps, fix tests

26fff02

Remove 7.0.1 deps from lockfile

690fc0b

jhamon changed the title ~~[WIP] Downgrade to openapi 5.2, add template overrides~~ Downgrade to openapi 5.2, add template overrides Oct 30, 2023

jhamon commented Oct 30, 2023

View reviewed changes

jhamon requested review from austin-denoble and rohanshah18 October 30, 2023 19:06

jhamon commented Oct 30, 2023

View reviewed changes

tests/unit/test_control.py Show resolved Hide resolved

jhamon marked this pull request as ready for review October 30, 2023 19:13

austin-denoble approved these changes Oct 30, 2023

View reviewed changes

rohanshah18 reviewed Oct 30, 2023

View reviewed changes

pinecone/config/openapi.py Show resolved Hide resolved

rohanshah18 approved these changes Oct 30, 2023

View reviewed changes

jhamon merged commit 70c80a8 into spruce Oct 31, 2023
5 of 6 checks passed

jhamon deleted the jhamon/template-overrides branch October 31, 2023 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Downgrade to openapi 5.2, add template overrides #229

Downgrade to openapi 5.2, add template overrides #229

jhamon commented Oct 26, 2023 •

edited

Loading

jhamon Oct 30, 2023

jhamon Oct 30, 2023

jhamon Oct 30, 2023

jhamon Oct 30, 2023 •

edited

Loading

austin-denoble Oct 30, 2023

austin-denoble left a comment

austin-denoble Oct 30, 2023

austin-denoble commented Oct 30, 2023

rohanshah18 left a comment

Downgrade to openapi 5.2, add template overrides #229

Downgrade to openapi 5.2, add template overrides #229

Conversation

jhamon commented Oct 26, 2023 • edited Loading

Problem

Solution

Type of Change

Follow-up needed

jhamon Oct 30, 2023

Choose a reason for hiding this comment

jhamon Oct 30, 2023

Choose a reason for hiding this comment

jhamon Oct 30, 2023

Choose a reason for hiding this comment

jhamon Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

austin-denoble Oct 30, 2023

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

austin-denoble Oct 30, 2023

Choose a reason for hiding this comment

austin-denoble commented Oct 30, 2023

rohanshah18 left a comment

Choose a reason for hiding this comment

jhamon commented Oct 26, 2023 •

edited

Loading

jhamon Oct 30, 2023 •

edited

Loading