build: use poetry for reproducible virtual environments

[VassilisVassiliadis]

Description of the change

• Use poetry:
  1. to build reproducible virtual environments
  2. during CI/CD testing
  3. to build the container image
• Update test_launch_script.py to handle exceptions that accelerate.commands.launch.launch_command(args) raises when the accelerate option num_processes is greater than 1
• Update tests in test_launch_script.py to try and use a free port. This is useful if you have a default accelerate_config.yml file (e.g. under ~/.cache/transformers) with a num_processes > 1 field.
• Updated tests that set the training argument --eval_strategy epoch to fallback to --evaluation_strategy for backwards compatibility with older version of transformers (e.g. 4.39.3 and 4.40.2).
  • due to fms_acceleration having a constraint to transformers<4.40.0 transformers gets set to 4.39.3 when you use poetry to install fms-hf-tuning (even if you do not add the cli arg --with fms-accel). fms_acceleration is no longer an optional package of fms-hf-tuning, there's an explicit constraint for transformers which restricts the package to <=4.40.2
  • This PR introduces a method get_train_args() in test_sft_trainer.py for building the configs.TrainingArguments object

Related issue number

Contributes to #60

How to verify the PR

Use poetry install to install fms-hf-tuning from source. You will end up installing the python dependencies listed in poetry.lock

Was the PR tested

• I have added >=1 unit test(s) for every new method I have added.
• I have ensured all unit tests pass

[VassilisVassiliadis]

Actually, this doesn't make sense, so I'll revert this change. Poetry already takes care of "pinning" this dependency for the reproducible virtual environments it creates (i.e. the poetry.lock file).

We just have to run poetry lock (or manually update python dependencies) whenever fms_acceleration releases a new version and we'd like to update the dependencies of fms-hf-tuning to match those of fms_acceleration.

For example, at the moment fms_acceleration has a constraint transformers<4.40.0 which propagates to fms-hf-tuning and sets transformers to 4.39.3.

[tedhtchang]

Just curious what's this change for ?

[VassilisVassiliadis]

logging.error isn't part of transformers.utils.logging .

For example, when you run this:

 python -c 'from transformers.utils import logging; logging.error("foo")'

python raises the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: module 'transformers.utils.logging' has no attribute 'error'

so I just created a logger object called "error" and had that print the traceback. I assume that this line of code was originally using the logging module for which logging.error("foo") works

[VassilisVassiliadis]

Unfortunately, I don't remember how I made sft_trainer.py execute that code.

[VassilisVassiliadis]

I updated the description of the issue with a couple more changes that I had to do for the tests to pass.

[tedhtchang]

For reviewers
Keep in mind this change won't affect fms-hf-tuning packages users who do pip install fms-hf-tuning. pip installs the appropriate indirect (latest) dependencies versions for their environment.

For package developers, these are the most used poetry commands. So you may follow those examples to verify the PR:

# (NOT optional) Installed poetry in a dedicated venv [manually](https://python-poetry.org/docs/#installing-manually)
python3 -m venv $VENV_PATH
$VENV_PATH/bin/pip install -U pip setuptools
$VENV_PATH/bin/pip install poetry
export PATH=$PATH:$VENV_PATH/bin/poetry # verify poetry -h prints help text

# To verify the PR, first clone and create a new venv.
git clone https://github.com/VassilisVassiliadis/fms-hf-tuning.git -b poetry
# If not already in a venv poetry will create and activate one automatically. So w/o activate a venv:
poetry install
poetry shell # to active the venv created by poetry.
# Run pip list in the shell to verify dependencies from the poetry.lock are installed + fms-hf-tuning in editing mode.

# Install deps with dev group
poetry install --extras=dev

# Install deps without fms-hf-tuning in editing mode(dependencies only)
poetry install --no-root

# Update packages version in lock file. 
# Supposedly dependabot should update the lock file and create PR
poetry update --lock # optionally re-run poetry install to update the packages in venv.
# verify with `git diff poetry.lock` that some versions are bumped.

# Verify tox tasks ran withtou issue
tox -e py,fmt,lint,build,twinecheck,coverage

# Verified image has deps specified in the lock file
docker build . -t sft-trainer:dev -f build/Dockerfile
docker run -it sft-trainer:dev pip freeze

[tedhtchang]

@VassilisVassiliadis some files have been updated could you rebase. thanks

[VassilisVassiliadis]

I rebased the PR and removed the instructions to install fms-accell using poetry. I replaced that text with a link to the relevant section in the README.md file.

[VassilisVassiliadis]

I also tested building the dockerfile for the last release of fms-hf-tuning on pyli i.e.

 docker build --progress plain  . -t sft-trainer:dev -f build/Dockerfile --build-arg WHEEL_VERSION=0.4.0

In a future PR we could have the images built for a release actually install the dependencies from the poetry.lock file associated with that release (assuming there's a way to guess the git-tag/commit that was used to produce the pypi release)

[tedhtchang]

@VassilisVassiliadis Thanks this looks good. And I agree. We need a mechanism to build/rebuild any release image with consistent dependencies for a given commit/tag/sha.

@Ssukriti @anhuong @jbusche Could you take a look the PR.

[jbusche]

Hi @tedhtchang, I tried it and it looked good...
I was able to poetry install and try the

poetry update --lock
git diff poetry.lock

and see that it had an update:

 [[package]]
 name = "orjson"
-version = "3.10.5"
+version = "3.10.6"

I was then able to install aim with the original orjson and then update it:

poetry install --extras=aim
poetry shell
pip list |grep orj
orjson                   3.10.5
poetry update --lock
git diff poetry.lock
-version = "3.10.5"
+version = "3.10.6"
poetry install --extras=aim
Installing dependencies from lock file
Package operations: 0 installs, 1 update, 0 removals
 - Updating orjson (3.10.5 -> 3.10.6)

[jbusche]

LGTM, tested on my Red Hat Server

[Ssukriti]

Suggested change

	Alternatively, you could continue `pip install -e . `, if you do not wish to leverage the lock file and have environmental contrainsts

[Ssukriti]

I pushed some changes, please review.

Additionally in release process, @tedhtchang please add a post step to run poetry update and commit the new lock file. This has to be done post release by maintainers to keep it updated

[Ssukriti]

Suggested change

	# Install using poetry if PyPi wheel_version is empty else download the wheel from PyPi
	# Install using poetry if PyPi wheel_version is empty else download the wheel from PyPi
	# If creating your own dockerfile we suggest to use poetry export for a reproducible environment

[fabianlim]

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 the PATH 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 the peoetry in user and try to update it?

[fabianlim]

There quite a few instances of these in other worflows as well.

[tedhtchang]

tox always creates it's own isolated venv.
Agree it would probably work without --user

[VassilisVassiliadis]

Good catch, tox and poetry should go in the same virtual-environment and that environment should not be the one that we install fms-hf-tuning in. Because tox creates a new virtual environment for the environments 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 inside tox can use the poetry 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.

[fabianlim]

I feel this is abit confusing. If we have a peoetry flow, and there is a new dep, shouldnt we have some guidance on when it has to be added to the lockfile?

Also some guidance to say that for optional deps it should not be added.

[Ssukriti]

true, maybe we can continue adding to pyproject.yml , like we do now and then we have to run poetry update ?
I can change it to that instead

[VassilisVassiliadis]

There're 2 ways to add a new dependency. One is to use poetry i.e. poetry add the other is to manually edit the pyproject.toml file and then run poetry update which will then ensure poetry.lock is consistent with pyproject.toml (that's basically what poetry add does on your behalf).

If you opt for the 2nd route but forget to run poetry update then the next time you run poetry install (or the CI/CD runs it) you'll get an exception that the poetry.lock and pyproject.toml files are inconsistent and that you should run the poetry update command.

[fabianlim]

@VassilisVassiliadis there is a PR here to add back the [.fms-accel] dep. It was removed before with the intention of being added back #223

• why will poetry constraint packages under optional dependencies? that makes no sense

[fabianlim]

this line is not very clear.. this will do not any installation correct? Its just an export of the locked deps to a requirements file?

[Ssukriti]

ya its just to export the requirements and same lock file and then we have to install. hmm should be just link our dockerfile as example?

[VassilisVassiliadis]

@VassilisVassiliadis there is a PR here to add back the [.fms-accel] dep. It was removed before with the intention of being added back #223

@fabianlim I'll rebase the PR once #223 makes its way into the main branch.

why will poetry constraint packages under optional dependencies? that makes no sense

I'm actually not sure. I assume that they do this so that poetry install and poetry install -E fms-accel both end up installing the same non-optional dependencies (in our case transformers).