Remove create_with_new_index() #57

igiloh-pinecone · 2023-10-03T13:43:28Z

Problem

It's complicated to have both a from_config() constructor and a create_with_new_index() constructor.

Solution

If a KnowledgeBase is instantiated with an index_name of a non-existing index, as long as the connection with pinecone is ok - no error is raised. Instead, we simply print a message "please create the index".
Since we had a check whether the index is connected \ existed before any operation anyway - this doesn't change the error handling much.

The only edge case here is that a user instantiating a KnowledgeBase with a misspelled index_name, and not explicitly checking kb.verify_connection_health() - would only get the error when he tries to perform upsert\query. The CLI and the app would do the explicit check, so no late error there.
I think we can live with it

Type of Change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Test Plan

Modified tests

I changed `create_resin_index()` to be a method, which happens after KB instantiation. We were checking for `if self._index in None` in all of our functions anyway - so this doesn't change the error logic. If the user will create a KB instance with a name of a non-existing index, it will be printed that he has to create it first

The index name was already passed, there is no point passing it again to create_index()

resin/knoweldge_base/knowledge_base.py

This makes more sense, since now the constructor is very "fast" (logic only), and the slow process of actually connecting\creating happens explicitly later

Following the latest .connect() change

resin/knoweldge_base/knowledge_base.py

changes required

resin/knoweldge_base/knowledge_base.py

acatav · 2023-10-11T08:18:08Z

resin/knoweldge_base/knowledge_base.py

+            raise RuntimeError(
+                f"KnowledgeBase is already connected to index {self.index_name}. "
+            )
+        self._index = self._connect_index()


What is the way in the client side to restart the actual connection? should we support some flag that force re-connection?

I'm not sure there is one...
I'll look in the client code

Nope, there isn't really a way to do that

Even if you create a new Index object or call pinecone.init ? so you think holding the index object over days should work fine?

Theoretically - it should. The connection hangs up after ~10-15 minutes anyway.
And if it won't - that's not something we're going to solve in resin...

Got feedback in comments of PR #57 that this code is a bit unreadable, so I decided to implement explicitly in the constructors. It's a bit more code duplication, but much more readable. While at it, I also made some arguments mandatory, while keeping them optional in the config

Following feedback on PR #57

acatav · 2023-10-15T05:54:00Z

resin/knoweldge_base/knowledge_base.py

@@ -54,6 +48,7 @@ def __init__(self,
                 chunker: Optional[Chunker] = None,
                 reranker: Optional[Reranker] = None,
                 default_top_k: int = 5,
+                 index_params: Optional[dict] = None,


Can't we take this params only on the create method? getting it here makes an illusion that you pass index params and get a KB with it, but the truth is you can use it only in create method

We will explain it in the docstring.
It has to be here, so you can do that:

kb = KnowledgeBase.from_config(cfg) kb.create_index()

Got it, not ideal but good enough for now. I think it means that we want the defaults to be in the config.yaml

acatav · 2023-10-15T06:01:23Z

resin/knoweldge_base/knowledge_base.py

        except Exception as e:
-            try:
-                pinecone_whoami()
-            except Exception:
-                raise RuntimeError(
-                    "Failed to connect to Pinecone. "
-                    "Please check your credentials and try again"
-                ) from e
+            raise RuntimeError(
+                "The index did not respond. Please check your credentials and try again"
+            ) from e

-            if self._index_name not in list_indexes():
-                raise RuntimeError(
-                    f"index {self._index_name} does not exist anymore"
-                    "and was probably deleted. "
-                    "Please create it first using `create_with_new_index()`"
-                ) from e
-            raise RuntimeError("Index unexpectedly did not respond. "
-                               "Please try again in few moments") from e


I actually think it's better to have this detailed validation where we can tell the user exactly where is the problem. We hide pinecone API from them and also the full name of their index so we make it hard for them to debug where is the actual problem

We give very detailed errors in .connect(), it seemed like a duplication.
If someone has passed connect(), then for sure all of his credentials etc were already checked. This method would only be called after the initial connection was successfully created (at least once).

Got it. In the minimum I would change the message to something like "unexpected error" and not something about credential which is not the most likely thing in this situation.
I still think that providing a detailed reason in this point it's important and we should try to provide it without the need for the user to manually try to connect again, so I wonder if we can call here connect or do some small change to connect so it won't set a new index in this situation. Also use connect as some retry mechanism is an option.

Anyway, these are my thoughts, you can make any decision and I'm ok with it

resin/knoweldge_base/knowledge_base.py

If the index already exists - we will error out anyway.

igiloh-pinecone added 3 commits October 3, 2023 16:14

[kb] Further simplify create_index and solve a few bugs

bfe9980

The index name was already passed, there is no point passing it again to create_index()

[cli] Update cli to use new kb.create_resin_index()

87f3432

igiloh-pinecone requested a review from acatav October 3, 2023 13:43

igiloh-pinecone marked this pull request as draft October 3, 2023 13:43

acatav reviewed Oct 3, 2023

View reviewed changes

resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved

igiloh-pinecone added 2 commits October 3, 2023 17:06

[kb] remove redundant print

9e01275

[test] Fix kb tests following create_with_new() removal

6f4dcb4

igiloh-pinecone changed the title ~~WIP: Remove create_with_new_index()~~ Remove create_with_new_index() Oct 4, 2023

igiloh-pinecone requested review from acatav and miararoy October 4, 2023 20:13

igiloh-pinecone added 5 commits October 5, 2023 00:24

[test] test_app bug fix

71c71c0

[kb] Added explicit connect() step

f3a1d82

This makes more sense, since now the constructor is very "fast" (logic only), and the slow process of actually connecting\creating happens explicitly later

[test] Changed test_kb to work with the new .connect() mechanism

d8a832c

Merge remote-tracking branch 'origin/dev' into remove_create_new

119c65a

linter

08ad731

igiloh-pinecone marked this pull request as ready for review October 10, 2023 10:00

[test] Fix test_kb

96666dc

Following the latest .connect() change

acatav previously approved these changes Oct 10, 2023

View reviewed changes

resin/knoweldge_base/knowledge_base.py Outdated Show resolved Hide resolved

resin/knoweldge_base/knowledge_base.py Show resolved Hide resolved

acatav self-requested a review October 10, 2023 15:05

acatav suggested changes Oct 11, 2023

View reviewed changes

[kb] Improve connect() mechanism

94e292e

Following feedback on PR #57

acatav approved these changes Oct 15, 2023

View reviewed changes

[kb] Remove explicit error on calling .create_index() after .connect()

3eee7f8

If the index already exists - we will error out anyway.

igiloh-pinecone enabled auto-merge October 15, 2023 13:47

igiloh-pinecone added 2 commits October 15, 2023 19:48

Merge branch 'dev' into remove_create_new

9370399

Merge branch 'dev' into remove_create_new

c6fc163

igiloh-pinecone merged commit 1ec5e90 into dev Oct 15, 2023
9 checks passed

igiloh-pinecone deleted the remove_create_new branch October 15, 2023 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove create_with_new_index() #57

Remove create_with_new_index() #57

igiloh-pinecone commented Oct 3, 2023 •

edited

Loading

acatav Oct 11, 2023

igiloh-pinecone Oct 11, 2023

igiloh-pinecone Oct 12, 2023

acatav Oct 15, 2023

igiloh-pinecone Oct 15, 2023

acatav Oct 15, 2023

igiloh-pinecone Oct 15, 2023

acatav Oct 15, 2023

acatav Oct 15, 2023

igiloh-pinecone Oct 15, 2023

acatav Oct 15, 2023

Remove create_with_new_index() #57

Remove create_with_new_index() #57

Conversation

igiloh-pinecone commented Oct 3, 2023 • edited Loading

Problem

Solution

Type of Change

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igiloh-pinecone commented Oct 3, 2023 •

edited

Loading