Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

support list canopy indexes #116

Merged
merged 3 commits into from
Oct 30, 2023
Merged

support list canopy indexes #116

merged 3 commits into from
Oct 30, 2023

Conversation

acatav
Copy link
Contributor

@acatav acatav commented Oct 29, 2023

Problem

Canopy does not provide any way to list Canopy's indexes in Pinecone

Solution

Add a module level method to list Canopy indexes

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

Added a system test

Copy link
Contributor

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

LGTM. See one comment

Comment on lines 53 to 58
try:
pinecone_init()
pinecone_whoami()
except Exception as e:
raise RuntimeError("Failed to connect to Pinecone. "
"Please check your credentials and try again") from e
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try:
pinecone_init()
pinecone_whoami()
except Exception as e:
raise RuntimeError("Failed to connect to Pinecone. "
"Please check your credentials and try again") from e
KnowledgeBase._connect_pinecone()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted the functionality to module level since this method should not know about kB

Copy link
Contributor

@igiloh-pinecone igiloh-pinecone Oct 30, 2023

Choose a reason for hiding this comment

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

Still - no need for code duplication. They are in the same module (no imports required), and _connect_pinecone is a static function (you don't need to instantiate a KB instance).
It's perfectly ok to call it.

@acatav acatav enabled auto-merge October 30, 2023 07:04
@acatav acatav added this pull request to the merge queue Oct 30, 2023
Merged via the queue into dev with commit b7a35f8 Oct 30, 2023
10 checks passed
@acatav acatav deleted the list-canopy-indexes branch October 30, 2023 07:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants