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

KB initial docstring #76

Merged
merged 21 commits into from
Oct 18, 2023
Merged

KB initial docstring #76

merged 21 commits into from
Oct 18, 2023

Conversation

acatav
Copy link
Contributor

@acatav acatav commented Oct 16, 2023

Initial docstrings for KB

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

No need

Comment on lines 57 to 59
Init the knowledge base object.
Note: The knowledge base is not connected to the index
until connect() is called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably include a minimal example of how to use a knowledgeBase, something like:

When creating a new Resin service, the user must first create the underlying Pinecone index.
This is a one-time setup process - the index will exist on Pinecone's managed service until it is deleted.

>>> kb = KnowLedgeBase('my_index')
>>> kb.create_resin_index()

In any future interactions, the user simply needs to connect to the existing service:

>>> kb = KnowLedgeBase('my_index')
>>> kb.connect()
....
kb.upsert(my_documents)
knowledge_base.query(queries=[Query(text="Who was the president in 1996?", top_k=10)])

Comment on lines 71 to 72
index_params: A dictionary of parameters to pass to the index creation API.
see https://docs.pinecone.io/docs/python-client#create_index
Copy link
Contributor

Choose a reason for hiding this comment

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

(@acatav I agree, adding this param to the constructor is really ugly...
I'll try to find a way to remove it and still support from_config.

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.

Please see my suggested fixes and additions.
And please take them as suggestions only - the final format is up to you.

acatav and others added 7 commits October 17, 2023 15:51
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
…ings-template

# Conflicts:
#	resin/knoweldge_base/base.py
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
acatav and others added 8 commits October 17, 2023 17:25
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
Co-authored-by: igiloh-pinecone <118673156+igiloh-pinecone@users.noreply.github.com>
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!
Let's just make sure we don't line break \ indent in places we shouldn't be (in the rendered docstring)

resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved
resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved
resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved
resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved
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!

@acatav acatav merged commit a8a9dc5 into dev Oct 18, 2023
9 checks passed
@acatav acatav deleted the docstrings-template branch October 18, 2023 09:58
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