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

Include main classes in canopy's main __init__ #141

Closed
wants to merge 2 commits into from

Conversation

igiloh-pinecone
Copy link
Contributor

Problem

The import path for some of our classes was too convoluted, making our examples complicated.

Solution

  1. I included all the major classes + data classes in canopy's main __init__.py
  2. Changed the README, example notebook, and docstring examples accordingly

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

None

I've imported the major classes and data models directly under `canopy`.
Changed the README, notebook, and docstring examples accordingly
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@acatav acatav left a comment

Choose a reason for hiding this comment

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

I'm not sure we want to import so many things in Canopy highest level because it means that you can't take small parts of it without importing many many stuff (that right now takes more than 4 seconds to import). I'm leaving it to you decision but just noting that putting it in highest level is one way door. Adding some of it later is always possible so you can consider putting non data models stuff outside the canopy init file

@@ -1,4 +1,4 @@
# Canopy Library
z# Canopy Library
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
z# Canopy Library
# Canopy Library

Comment on lines +4 to +6
from .knowledge_base import KnowledgeBase
from .context_engine import ContextEngine
from .chat_engine import ChatEngine
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we want this things to be imported in highest level. It means that even if I just want the chunker from Canopy, I'm going to import many other stuff. Is there anyway around that?

Copy link
Contributor Author

@igiloh-pinecone igiloh-pinecone Nov 5, 2023

Choose a reason for hiding this comment

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

  1. I still think we should implement the Chunkers themselves in pinecone-text, and wrap in canopy. You need to distinct between the chunker logic, which operates on simple data types (List[str]) and the Chunker class, which is a sub-component of KnowledgeBase.
    As I see it, people should use canopy if they want the functionality of KnowledgeBase or ContextEngine. If someone just needs a Chunker \ Ranker \ Encoder etc - they should take it directly from pinecone-text. Even if we'll decide to merge them into a mono repo - they would still be two separate packages, with their own dependencies etc.
  2. Regardless of all of the above, the import itself should be super light and dependency free. Even if someone wants to use only the KnowledgeBase without any LLM or ChatEngine - importing the ChatEngine should be quick and hassle free. Any special dependencies should happen on __init__(), not on import. So it should be ok to import everything at the root level.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree in long term. Right now for both points this is not what we have and we can't tell if ever we will have either. So I wonder if we should not do that until both points or at least the second are true

@miararoy
Copy link
Contributor

miararoy commented Nov 5, 2023

I am unable to do from canopy import KnowledgeBase for some reason

Base automatically changed from cli_server to server-docstrings November 6, 2023 09:13
@igiloh-pinecone
Copy link
Contributor Author

Closing for now, will come back to this in the future

@igiloh-pinecone igiloh-pinecone deleted the import_from_canopy branch November 6, 2023 16:17
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.

3 participants