-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: optional transformers
#5101
Conversation
Co-authored-by: Massimiliano Pippi <[email protected]>
Pull Request Test Coverage Report for Build 5264914479
💛 - Coveralls |
@ZanSara Really looking forward to this PR! I tried out this branch and manually installed Haystack's dependencies including transformers but without pytorch, sentencepiece, sentence-transformers and huggingface-hub. I was able to run examples/agent_multihop_qa.py. All I had to do was also install
After that it worked as expected without any torch/cuda dependencies installed. 🎉 Not sure which of pytorch, sentencepiece, sentence-transformers and huggingface-hub usually installed Pillow. We should make the import from Pillow also a lazy import. That can happen in a separate PR. Once this PR here is merged, I will open one removing torch from Haystack's core dependencies. |
Thanks for testing it! I'll update the PR description to include Pillow |
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.
Looks very good to me and as I have already said earlier, I'm really looking forward to it! 👍 I have a few minor questions in the review comments. Once we have sorted them out, this PR will be ready to be merged.
@@ -3,21 +3,25 @@ | |||
import logging | |||
from pathlib import Path | |||
|
|||
import torch | |||
from tqdm.auto import tqdm | |||
import numpy as np | |||
from PIL import Image |
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.
Let's double check whether this needs a LazyImport too. I'm guessing it does.
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.
For now I'd just install it, Pillow is not huge. But in the spirit of making all dependencies somewhat optional in the future, we could move it out as well. Let's do it in another PR.
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.
Ready to be merged now! ✅ Thanks for addressing the change requests so quickly. 👍
Proposed Changes:
Apparently PyScript (based on Pyodide) and PyTorch are currently incompatible: pyodide/pyodide#1625. However, we could still make some nice demos in PyScript with components that do not use transformers.
On the other hand, we don't want to extract transformers from the core dependencies, due to the fact that it would restrict too much the number of nodes users could use out of the box.
This PR makes all the imports of Torch and Transformers optional, but does NOT extract transformers from the list of core dependencies. In this way, a casual user doing
pip install farm-haystack
will get transformers and pytorch, while a motivated power user that absolutely can't install transformers or torch can create a requirements.txt with the following content:and then do:
pip install --no-deps farm-haystack && pip install -r requirements.txt
to obtain a PyTorch-free version of Haystack.How did you test it?
pip install --no-deps . && pip install -r requirements.txt && haystack
Notes for the reviewer
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.