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

Install text-generation-server from poetry.lock export #2786

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alvarobartt
Copy link
Member

What does this PR do?

This PR installs the text-generation-server Python requirements from an exported requirements.txt-like file generated out of the poetry.lock, to be able to reuse the generated lock with the pinned dependencies and avoid dependency issues or conflicts when building outdated Dockerfiles in case any dependency is loose, as any potential conflict is always solved before the release and the lock is always working.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guidelines, Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the documentation guidelines, and here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

cc @OlivierDehaene

Installing from an exported `requirements.txt` like file generated out
of the `poetry.lock` makes sense to be able to reuse the generated lock
with the pinned dependencies and avoid dependency issues or conflicts
when building outdated Dockerfiles in case any dependency is loose and
runs into conflicts i.e. any potential conflict is always solved before
the release
@alvarobartt alvarobartt self-assigned this Nov 29, 2024
Dockerfile Outdated Show resolved Hide resolved
@alvarobartt alvarobartt marked this pull request as draft November 29, 2024 13:10
Manually install the plugin to avoid issues with future `poetry`
versions as of the warning messages recommend:

```
WARNING: Running pip as the 'root' user can result in broken permissions
and conflicting behaviour with the system package manager. It is
recommended to use a virtual environment instead:
https://pip.pypa.io/warnings/venv Warning: poetry-plugin-export will not
be installed by default in a future version of Poetry. In order to avoid
a breaking change and make your automation forward-compatible, please
install poetry-plugin-export explicitly. See
https://python-poetry.org/docs/plugins/#using-plugins for details on how
to install a plugin. To disable this warning run 'poetry config
warnings.export false'.
```
@alvarobartt alvarobartt marked this pull request as ready for review December 4, 2024 17:39
@@ -233,7 +250,8 @@ COPY server/Makefile server/Makefile
RUN cd server && \
make gen-server && \
pip install -r requirements_cuda.txt && \
pip install ".[attention, bnb, accelerate, compressed-tensors, marlin, moe, quantize, peft, outlines]" --no-cache-dir && \
pip install -r requirements_poetry.txt --no-cache-dir && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the issue again ? Why are we not using poetry directly ?

Also maybe we should take the opportunity to switch to uv ? https://docs.astral.sh/uv/pip/compile/#locking-requirements I have no idea, but it seems like the poetry lock isn't locking tightly enough, maybe ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the delay on this PR, and happy new year BTW! So the issue is that since we're not using poetry as the pip installer the locked dependencies are not used leading to potential issues when partners as e.g. Google build the containers ahead in time, as those dependencies are installed from the pyproject.toml rather than the lock.

I know this is not an issue impacting directly TGI per se, but for robustness and reproducibility we should try to lock those and install those from a lock. The installation from the lock can either be via poetry (the current approach), or switching to uv instead and installing from the uv.lock.

Maybe we should start an internal conversation and decide what's best, happy to help whatever the solution / proposal is!

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.

2 participants