Skip to content
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

[Bug] fix requirements.txt for example chat-w/-pdf #284

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

LatentDream
Copy link
Member

@LatentDream LatentDream commented Oct 2, 2024

The example is missing some dependencies

To resolve this comment: https://github.com/ploomber/cloud-frontend/pull/601#pullrequestreview-2341988066


📚 Documentation preview 📚: https://ploomber-doc--284.org.readthedocs.build/en/284/

aiohappyeyeballs==2.4.3
aiohttp==3.9.3
aiosignal==1.3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

All the dependencies can go in a requirements.lock.txt file. In this file, you can add the specific package that was needed. You can see the format in the other examples

Copy link
Member Author

Choose a reason for hiding this comment

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

requirements.lock.txt this file isn't use by our deployment pipeline. So thus why the missing dependencies.

This is the package installation of a Panel Dockerfile from our platform:

WORKDIR /srv
# Caching Introduced here
COPY requirements.txt /srv/
RUN pip install -r requirements.txt --no-cache-dir

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the lock file is for debugging purposes. We can just add the specific package required to resolve the issue langchain_community.document_loaders in this file. I'm not sure why all the installed packages need to be mentioned in the requirements.txt file

Copy link
Contributor

@neelasha23 neelasha23 Oct 2, 2024

Choose a reason for hiding this comment

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

It is mentioned in the error: You can install it using pip install -U langchain-community
So just add langchain-community in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't the command pip freeze > requirements.txt better? It captures all packages necessary for the app with their versions. This way, future users can build it with the same versions and be sure the app will work.

  • Langchain are often breaking api so you need the exact version
  • All those requirements are from installing the
panel
openai
chromadb
pypdf
langchain
tiktoken
langchain-community

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally freeze all requirements in the lock file and not the requirements.txt. But maybe @edublancas can suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

there are pros and cons for each approach. in the past, we've created a .lock.txt file and a .txt file. the problem is that the platform only looks at the .txt file.

let's pin dependencies in a .txt file so the platform uses that, but let's only list requirements manually (instead of running pip freeze) because listing everything has the potential to break examples (as there might be packages that are incompatible with Linux or that aren't even needed)

annotated-types==0.6.0
anyio==4.3.0
asgiref==3.7.2
Copy link
Contributor

Choose a reason for hiding this comment

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

let's pin versions but only for the explicitly imported packages

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix: 3a3a901

@edublancas edublancas merged commit 2b6a9c7 into main Oct 2, 2024
1 check passed
@edublancas edublancas deleted the fix-panel-example-requirements branch October 2, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants