-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make transformers optional + allow pinecone-text[dense] #266
Make transformers optional + allow pinecone-text[dense] #266
Conversation
…t[dense] 1. `transformers` shouldn't be a required dependency. It was accidentaly added as such. 2. Added option to intall pinecone-text with dense dependency, under a new extra named `torch` (feels like the right name, since this is the heaviest dependency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added few questions
Now that I made `transformers` optional, needed to add error handling so it won't crash on import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I added make command that installs canopy with a selected list of extras, passed by an env var
Should be the first command after lint
@@ -9,6 +9,10 @@ inputs: | |||
description: "Whether to install canopy library, or dependencies only" | |||
required: true | |||
default: "true" | |||
extras: | |||
description: "Extra dependencies to install" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should explain it is space separated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a list also is an option
@@ -37,8 +41,12 @@ runs: | |||
- name: Install dependencies | |||
shell: bash | |||
if: steps.cached-poetry-dependencies.outputs.cache-hit != 'true' | |||
run: poetry install --no-interaction --no-root --all-extras --with dev | |||
env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need --with dev? By default it is with dev isnt it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure, and I prefer to be explicit...
Improve the text description of the new `Extras` field
Not really needed
Problem
transformers
shouldn't be a required dependency. It was accidentally added as such.pinecone-text
with its[dense]
extra dependencySolution
transformers
optional, only installed with a newanyscale
extratorch
extra (torch is the heaviest installation, so it made more sense as the extra's name thantransformers
orsentence-transformers
, which might be ambiguous)Type of Change
Might be slightly breaking for users who currently use the Anyscale Tokenizer, and would need to re-install with the extra.
Test Plan
No affect on current tests