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

Support passing custom host for Index operations #218

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

austin-denoble
Copy link
Contributor

@austin-denoble austin-denoble commented Oct 12, 2023

Problem

We need support for supplying a custom host value for index operations.

Solution

pinecone.init() and Config already support the ability to supply a custom host. It didn't seem like it was being passed through to the ApiClient:

  • Update _get_api_instance() in manage.py to check for a Config.CONTROLLER_HOST value, and apply it to the client_config if so. This will then be used throughout the index operations.
  • Add a unit test file for manage.py, test setting controller_host and timeout functionality in create_index / delete_index.

Type of Change

  • New feature (non-breaking change which adds functionality)

Test Plan

Pull this branch down and test locally. I validated myself by passing a bogus host in and looking over the errors, then passing the proper host via init to validate it works properly.

>>> poetry shell
>>> python3

>>> import pinecone
>>> pinecone.init(api_key="123-456-789", environment="my_environment", host: "https:custom-host-foo.com")

# Having passed a custom host that's inaccessible should fail
>>> pinecone.list_indexes()
# urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='fooblahblah.io', port=443): Max retries exceeded with  
# url: /databases (Caused by NameResolutionError("<urllib3.connection.HTTPSConnection object at 0x107eb9dd0>:
# Failed to resolve 'fooblahblah.io' ([Errno 8] nodename nor servname provided, or not known)"))


# Re-Init - should work as expected
>>> pinecone.init(api_key="123-456-789", environment="my_environment")

pinecone.list_indexes()
# ['austin-3', 'austin-py-test', 'resin--test-index-1', 'test-create-1']

@austin-denoble austin-denoble requested a review from jhamon October 12, 2023 15:46
Copy link
Collaborator

@jhamon jhamon left a comment

Choose a reason for hiding this comment

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

I know it's asking for a lot in this situation, but a test case would be great to see here.

@austin-denoble
Copy link
Contributor Author

I know it's asking for a lot in this situation, but a test case would be great to see here.

I was thinking about that as well, definitely worth getting one added. I'll take a pass.

@jhamon
Copy link
Collaborator

jhamon commented Oct 13, 2023

Looks great, thanks!

@austin-denoble austin-denoble force-pushed the adenoble/spruce-configuration-support branch from 68dbc01 to 55acdd5 Compare October 13, 2023 21:20
@austin-denoble austin-denoble merged commit 7ff4ed8 into main Oct 13, 2023
5 checks passed
@austin-denoble austin-denoble deleted the adenoble/spruce-configuration-support branch October 13, 2023 21:24
austin-denoble added a commit that referenced this pull request Oct 17, 2023
## Problem
Previous revision we added some code to apply a custom `host` value to
control plane operations:
#218

This change caused 404s when calling dataplane operations. The
`Configuration._base_path` value was being overwritten by the custom
`host` that was provided due to the `Configuration.host` setter:

```
  @host.setter
  def host(self, value):
      """Fix base path."""
      self._base_path = value
      self.server_index = None
```

https://github.com/pinecone-io/pinecone-python-client/blob/7ff4ed877ac3e97eb557b747a072a84e57764b91/pinecone/core/client/configuration.py#L493

## Solution
The change I made was actually modifying the reference to
`Config.OPENAPI_CONFIG` which overwrote things across the board.

Instead of operating on the singleton's instance of the config, we can
use `copy.deepcopy(Config.OPENAPI_CONFIG)` and use that to construct
things before handing it off to `ApiClient`

## Type of Change
- [X] Bug fix (non-breaking change which fixes an issue)

## Test Plan
Pull this branch down, use the client locally and make sure that
dataplane calls work properly without a `host` or with one in the case
of supplying a custom control plane URL.
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.

2 participants