Skip to content

Conversation

tdonia
Copy link
Contributor

@tdonia tdonia commented Aug 24, 2023

Problem

Per #196 a typo in init's kwargs results in a default that might not be expected.

Solution

Utils now includes check_kwargs, which takes a caller + the given kwargs, and fails with a traceback if they don't match.

Open Questions

cc @jhamon @austin-denoble @rohanshah18

  1. Where else should we use check_kwargs?
  2. Is the traceback useful? in this instance it risks leaking api keys into logs, so for init in particular i question its value.

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)

Test Plan

pinecone.init(not_a_kwarg='foo') should fail

@tdonia
Copy link
Contributor Author

tdonia commented Aug 24, 2023

here's example output with the traceback -- note the xxxx-xxxx-xxxx-xxxx-xxxx would be sensitive, and ideally not logged:

(popy-py3.11) popy :> poetry run python popy/py-cone.py
Traceback (most recent call last):
  File "/Users/travisdonia/src/popy/popy/py-cone.py", line 6, in <module>
    pinecone.init(wrong_kwarg="xxxx-xxxx-xxxx-xxxx-xxxx", environment="northamerica-northeast1-gcp")
  File "/Users/travisdonia/Library/Caches/pypoetry/virtualenvs/popy-b0Yl8H78-py3.11/lib/python3.11/site-packages/pinecone/config.py", line 248, in init
    check_kwargs(init, kwargs)
  File "/Users/travisdonia/Library/Caches/pypoetry/virtualenvs/popy-b0Yl8H78-py3.11/lib/python3.11/site-packages/pinecone/core/utils/__init__.py", line 118, in check_kwargs
    raise TypeError(caller.__name__ + ' had unexpected keyword argument(s): ' + ', '.join(diff))
TypeError: init had unexpected keyword argument(s): wrong_kwarg

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 see what you mean about the problem of sensitive information in the stacktrace. I found this old package, which is currently archived, which implemented a decorator-based approach to scrubbing sensitive information from stacktraces. Maybe you could look around to find a similar project that isn't dead or else lift the relevant bits of source for us to directly copy. It doesn't look like that much code, and it's MIT licensed so fair game.

I think we can still move forward with this change because the problem of secrets in stacktraces is not a new issue. Any kind of error that results in a thrown exception would already have this problem if the person is initializing with method params (instead of environment variables, which should not appear). And stacktrace secret-scrubbing could be handled as a follow-up item.

As for where else this util should get used... I'm not sure. Most of the bug reports I have seen along these lines were problems with the init call. I think once people get started, they're more likely to take an iterative approach if something isn't working the way they expect due to them passing the wrong stuff. But when they run into this on their very first line of code with Pinecone, they seem to just throw up their hands in frustration.

@tdonia
Copy link
Contributor Author

tdonia commented Aug 25, 2023

I've modified this specifically to omit the traceback for this particular check because the risk of leaking an API Key in a bad init call is especially high. also added a test and moved that import up. thanks for the review!

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.

LGTM!

@tdonia tdonia merged commit 2e606bb into main Aug 25, 2023
@tdonia tdonia deleted the tdonia/check-kwargs-init branch August 25, 2023 21:43
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.

3 participants