-
Notifications
You must be signed in to change notification settings - Fork 48
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
build: use poetry for reproducible virtual environments #209
Draft
VassilisVassiliadis
wants to merge
45
commits into
foundation-model-stack:main
Choose a base branch
from
VassilisVassiliadis:poetry
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 43 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
6a8c5b0
feat: update pyproject.toml to use poetry
VassilisVassiliadis 4aba398
fix: add the tuning package
VassilisVassiliadis 97349ca
fix: make group dependencies optional
VassilisVassiliadis c3a1cf5
build: update poetry.lock contents
VassilisVassiliadis 6638038
build: update the Dockerfile to use the poetry lock file
VassilisVassiliadis 54163fc
feat: use poetry-dynamic-versioning
VassilisVassiliadis b404f8c
build: update aim version to 3.22.0 and trl to 0.8.6
VassilisVassiliadis 3655a49
build: use poetry when running tests and building the wheel
VassilisVassiliadis fbacdd4
docs: document how to install the repository using poetry
VassilisVassiliadis 51fea97
build: deal with git error regarding dubious ownership
VassilisVassiliadis 4903eee
build: update flash-attn constraint to ^2.5.6
VassilisVassiliadis dfc818c
fix: add simpleeval to python dependencies
VassilisVassiliadis 19c7acc
fix: support 3.9 to 3.11
VassilisVassiliadis 8baa00a
fix: fms_acceleration dependency
VassilisVassiliadis 13b1893
fix: install fms-hf-tuning with poetry before running pytest
VassilisVassiliadis 1a31ba2
refactor: rename build python file and tests as launcher
VassilisVassiliadis 72a92bc
feat: use gen_train_args() method to generate train args and use eval…
VassilisVassiliadis b8e4443
fix: logging.error doesn't exist in transformers.utils.logging (v4.39.3)
VassilisVassiliadis f40a21b
feat: add missing dependencies to dev group
VassilisVassiliadis 2313ebc
build: update tox to use poetry
VassilisVassiliadis f0fc824
fix: add the missing launcher scripts for the python package
VassilisVassiliadis 0af2888
fix: tox lint and tox coverage
VassilisVassiliadis fc5e362
fix: tox lint and tox coverage
VassilisVassiliadis 1a719b2
build: point fms_acceleration dependency to its last known commit 40a…
VassilisVassiliadis 0d71c23
build: remove the pinned version of fms_acceleration
VassilisVassiliadis 6a46d26
build: update the lock file after modifying pyproject.toml
VassilisVassiliadis 771e2f9
refactor: move launcher scripts back into build
VassilisVassiliadis 1359e6f
build: remove `build` from the `dev` optional dependencies group
VassilisVassiliadis 865bd6c
build: update tox not install poetry in the virtual environment it tests
VassilisVassiliadis db5832e
build: update CI/CD to install poetry in --user site
VassilisVassiliadis 9739f90
build: install poetry for build-and-publish workflow
VassilisVassiliadis 21a1e5c
docs: document how to build a dev environment with poetry and tox
VassilisVassiliadis 109e478
docs: how to install optional dependency groups
VassilisVassiliadis 3aa293a
docs: fix broken link to fms-acceleration docs
VassilisVassiliadis ecc62bb
docs: fix typo
VassilisVassiliadis c3321c1
refactor: update test_run_with_additional_callbacks() unit-test
VassilisVassiliadis e8ff81b
fix: use optional `extra` dependencies instead of `groups`
VassilisVassiliadis 53025ee
fix: update dockerfile to install poetry in an isolated environment
VassilisVassiliadis 2158078
fix: replace "poetry install --with" with "--extras" in tox.ini
VassilisVassiliadis 896da24
fix: use poetry install --extras dev --no-root" for the fmt tox envir…
VassilisVassiliadis 4a3316e
chore: upgrade trl version to ">=0.9.3,<1.0"
VassilisVassiliadis ca9ab4e
chore: remove fms-accel extra group
VassilisVassiliadis a7e2bbb
build: fix the dockerfile by installing/uninstalling wheel and build
VassilisVassiliadis 5207283
Update CONTRIBUTING.md
Ssukriti 0828eb9
Update README.md
Ssukriti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -82,12 +82,45 @@ The following tools are required: | |||||
- [git](https://git-scm.com) | ||||||
- [python](https://www.python.org) (v3.8+) | ||||||
- [pip](https://pypi.org/project/pip/) (v23.0+) | ||||||
- [poetry](https://python-poetry.org/docs/#installation) (v1.8.3+) | ||||||
- Poetry should always be installed in a dedicated virtual environment to isolate it from the rest of your system. It should in no case be installed in the environment of the project that is to be managed by Poetry. This ensures that Poetry’s own dependencies will not be accidentally upgraded or uninstalled. | ||||||
- [tox](https://tox.wiki/en/4.15.1/installation.html) (v4.15.1+) | ||||||
- Just like `poetry` install `tox` in an isolated virtual environment | ||||||
|
||||||
Installation: | ||||||
``` | ||||||
pip install -U datasets | ||||||
pip install -e . | ||||||
|
||||||
```bash | ||||||
: Install poetry and tox in an isolated virtual environment | ||||||
python3 -m venv isolated | ||||||
fabianlim marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
./isolated/bin/pip install -U pip setuptools | ||||||
./isolated/bin/pip install poetry tox | ||||||
|
||||||
: Ensure you can access poetry and tox without activating the | ||||||
: the isolated virtual environment | ||||||
export PATH=$PATH:`pwd`/isolated/bin | ||||||
|
||||||
: Create your development virtual environment | ||||||
python3 -m venv venv | ||||||
. venv/bin/activate | ||||||
|
||||||
: Install a dev version (similar to pip -e ".[dev]") of fms-hf-tuning | ||||||
poetry install --extras dev | ||||||
``` | ||||||
|
||||||
|
||||||
> Note: After installing, if you wish to use [FlashAttention](https://github.com/Dao-AILab/flash-attention), then you need to install these requirements: | ||||||
|
||||||
``` | ||||||
poetry install --extras dev,flash-attn | ||||||
``` | ||||||
|
||||||
If you wish to use [aim](https://github.com/aimhubio/aim), then you need to install it: | ||||||
``` | ||||||
poetry install --extras aim | ||||||
``` | ||||||
|
||||||
If you wish to use [fms-acceleration](https://github.com/foundation-model-stack/fms-acceleration) follow the instructions in [this section of README.md](README.md#fms-acceleration). | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<details> | ||||||
<summary>Linting</summary> | ||||||
|
||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -110,29 +110,44 @@ RUN dnf install -y git && \ | |||||||
rm -f /usr/share/doc/perl-Net-SSLeay/examples/server_key.pem && \ | ||||||||
dnf clean all | ||||||||
USER ${USER} | ||||||||
WORKDIR /tmp | ||||||||
# Ensure that git directory is owned by current user, otherwise git raises | ||||||||
# "fatal: detected dubious ownership" for `/tmp` | ||||||||
WORKDIR /tmp/fms-hf-tuning | ||||||||
|
||||||||
# Install poetry and its dependencies inside an isolated virtual environment which we | ||||||||
# will not copy into the release-base layer | ||||||||
RUN --mount=type=cache,target=/home/${USER}/.cache/pip,uid=${USER_UID} \ | ||||||||
python -m pip install --user build | ||||||||
COPY --chown=${USER}:root tuning tuning | ||||||||
COPY .git .git | ||||||||
COPY pyproject.toml pyproject.toml | ||||||||
python -m venv venv /tmp/isolated && \ | ||||||||
/tmp/isolated/bin/pip install poetry poetry-plugin-export | ||||||||
|
||||||||
# Build a wheel if PyPi wheel_version is empty else download the wheel from PyPi | ||||||||
RUN if [[ -z "${WHEEL_VERSION}" ]]; \ | ||||||||
then python -m build --wheel --outdir /tmp; \ | ||||||||
else pip download fms-hf-tuning==${WHEEL_VERSION} --dest /tmp --only-binary=:all: --no-deps; \ | ||||||||
fi && \ | ||||||||
ls /tmp/*.whl >/tmp/bdist_name | ||||||||
COPY --chown=${USER}:root tuning tuning | ||||||||
COPY --chown=${USER}:root .git .git | ||||||||
COPY --chown=${USER}:root pyproject.toml pyproject.toml | ||||||||
COPY --chown=${USER}:root poetry.lock poetry.lock | ||||||||
COPY README.md README.md | ||||||||
|
||||||||
# Install from the wheel | ||||||||
# Install using poetry if PyPi wheel_version is empty else download the wheel from PyPi | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
RUN --mount=type=cache,target=/home/${USER}/.cache/pip,uid=${USER_UID} \ | ||||||||
python -m pip install --user wheel && \ | ||||||||
python -m pip install --user "$(head bdist_name)" && \ | ||||||||
python -m pip install --user "$(head bdist_name)[flash-attn]" && \ | ||||||||
# Clean up the wheel module. It's only needed by flash-attn install | ||||||||
python -m pip uninstall wheel build -y && \ | ||||||||
# Cleanup the bdist whl file | ||||||||
rm $(head bdist_name) /tmp/bdist_name | ||||||||
if [[ -z "${WHEEL_VERSION}" ]]; then \ | ||||||||
# Extract requirements from poetry and install them in ~/.local \ | ||||||||
# Need wheel and build for the flash-attn package \ | ||||||||
python -m pip install --user wheel build && \ | ||||||||
python -m pip install --user --requirement <(/tmp/isolated/bin/poetry export --format requirements.txt) && \ | ||||||||
# Next install the package with flash-attn \ | ||||||||
python -m pip install --user ".[flash-attn]" && \ | ||||||||
python -m pip uninstall wheel build -y ; \ | ||||||||
else \ | ||||||||
# This will use whatever dependencies versions satisfy the pyproject.toml constraints \ | ||||||||
# but they won't necessarily be the exact same versions as present in poetry.lock \ | ||||||||
# First, install fms-hf-tuning to get its dependencies which include torch. \ | ||||||||
# Then install with the flash-attn extras as the latter expects torch to be present \ | ||||||||
python -m pip install --user wheel build && \ | ||||||||
python -m pip install --user "fms-hf-tuning==${WHEEL_VERSION}" && \ | ||||||||
python -m pip install --user "fms-hf-tuning[flash-attn]==${WHEEL_VERSION}" && \ | ||||||||
python -m pip uninstall wheel build -y ; \ | ||||||||
fi | ||||||||
|
||||||||
RUN python -m pip freeze | ||||||||
|
||||||||
## Final image ################################################ | ||||||||
FROM release-base as release | ||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
why is there a need to install this to
--user
? If you can install peotry the same way as tox, that this obliviates the need for thePATH
setting below.If the aim is for isolation, then I dont think installing in the user space may achieve it. Since any further
pip install commands
will still find thepeoetry
inuser
and try to update 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.
There quite a few instances of these in other worflows as well.
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.
tox always creates it's own isolated venv.
Agree it would probably work without
--user
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.
Good catch,
tox
andpoetry
should go in the same virtual-environment and that environment should not be the one that we installfms-hf-tuning
in. Becausetox
creates a new virtual environment for theenvironments
it processes (e.g.py
, `fmt, etc) poetry and tox will be outside the tested virtual-environment.We don't need to run tox "inside" one of the virtual environments that tox creates but we do need to run
poetry
. Here's how I chose to do that.First, I installed poetry under the user pip install directory (which by default for linux is
~/.local/
. Then I updated the file that $GITHUB_ENV points to so that in subsequent steps the$PATH
environment variable contains the path~/.local/bin
.This ensures that the
poetry
commands running insidetox
can use thepoetry
executable. Alternatively, we can create a new virtual environment under a directory of our choosing e.g./tmp/isolated
in there, we install just poetry and then update$GITHUB_ENV
so that$PATH
includes/tmp/isolated/bin
.