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

[kb] Removed index_params from __init__() #78

Merged
merged 5 commits into from
Oct 18, 2023
Merged

Conversation

igiloh-pinecone
Copy link
Contributor

Problem

It doesn't make too much sense to require the index_params argument in the constructor. In interactive usage - this param should be passed directly to the create_resin_index()

Solution

method. I had to complicate the code of from_config() a bit - but it makes the usage pattern of KnowledgeBase much more explicit and readable.

Type of Change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Plan

Currently not covered. Will add test case separately.

It doesn't make too much sense to require the `index_params` argument in the constructor. In interactive usage - this param should be passed directly to the `create_resin_index()` method.
I had to complicate the code of `from_config()` a bit - but it makes the usage pattern of KnowledgeBase much more explicit and readable.
This is a tough one, I hope my explanations makes sense...
@igiloh-pinecone igiloh-pinecone merged commit 9e9ebcf into dev Oct 18, 2023
9 checks passed
@igiloh-pinecone igiloh-pinecone deleted the remove_index_params branch October 18, 2023 09:06
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