-
Notifications
You must be signed in to change notification settings - Fork 49
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: Add a dockerfile argument to enable aimstack #261
feat: Add a dockerfile argument to enable aimstack #261
Conversation
@anhuong @Ssukriti @ashokponkumar This change is in line with our request to have the dockerfile contain an argument to enable Aim. |
Why are we not using the optional dependency to install aim? |
build/Dockerfile
Outdated
ARG ENABLE_AIM | ||
|
||
# Need a way to keep this aim version in sync with pyproject.toml | ||
RUN if [ "$ENABLE_AIM" ] ; then \ |
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.
Also this formatting is incorrect and thus you see the image build failure. We could expect for ENABLE_AIM to be set to the string true by doing
RUN if [[ "$ENABLE_AIM" == "true ]]; then \
Or we could expect that is ENABLE_AIM is set to any value that we set it
RUN if [[ -n "$ENABLE_AIM" == "true ]]; then \
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.
Hey @anhuong sure I noticed the bug I was busy last 2 days hence did not get to it but fixed now.
0575ccc
to
dc5992f
Compare
Currenlty guarded by a dockerfile argument. Signed-off-by: Dushyant Behl <[email protected]>
dc5992f
to
c6ad231
Compare
@anhuong @ashokponkumar changed the way aim is being installed and updated the arg check. Please let me know if this is okay to merge. |
Signed-off-by: Dushyant Behl <[email protected]>
f140552
to
f39ccab
Compare
@anhuong @ashokponkumar please see the updated issue description for verification |
@anhuong Can you review the PR and merge? Also, let us know the outlook for getting the images with aim and image from |
Thanks @dushyantbehl the changes look good and appreciate adding the notes on testing build the image in both scenarios. Did you verify that running tuning on both images works as expected as well? |
we have not but can certainly do that @anhuong |
@dushyantbehl Tested with Commit ![]() ![]() |
Also tested the same commit It gives an error as expected since the AIM package is not installed.
Full error logs:
|
Thanks a lot @HarikrishnanBalagopal @anhuong both scenarios have been tested so can we proceed with merge? |
@anhuong can we please merge this PR and make the images available. |
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.
LGTM, thank you for testing building and running the image
…stack#261) * Add a dockerfile argument at the end of final layer to enable aimstack. Currenlty guarded by a dockerfile argument. Signed-off-by: Dushyant Behl <[email protected]> * Set the default value of ENABLE_AIM to false Signed-off-by: Dushyant Behl <[email protected]> --------- Signed-off-by: Dushyant Behl <[email protected]>
* Set default value of target_modules to be None in LoraConfig Signed-off-by: Will Johnson <[email protected]> * Removal of transformers logger and addition of python logger Signed-off-by: Abhishek <[email protected]> * FMT and lint check: Removal of transformers logger and addition of python logger Signed-off-by: Abhishek <[email protected]> * fix: remove lm_head for granite with llama arch models (#258) * initial code for deleting lm_head Signed-off-by: Anh-Uong <[email protected]> * fix logic for copying checkpoint Signed-off-by: Anh-Uong <[email protected]> * fix check that embed_tokens and lm_head weights are the same Signed-off-by: Anh-Uong <[email protected]> * fix warning assertion Signed-off-by: Anh-Uong <[email protected]> * fix lm_head check, remove test Signed-off-by: Anh-Uong <[email protected]> * small fixes from code review Signed-off-by: Anh-Uong <[email protected]> * fmt Signed-off-by: Anh-Uong <[email protected]> --------- Signed-off-by: Anh-Uong <[email protected]> Co-authored-by: Anh-Uong <[email protected]> Signed-off-by: Abhishek <[email protected]> * Add config_utils tests Signed-off-by: Angel Luu <[email protected]> * Fix fmt Signed-off-by: Angel Luu <[email protected]> * Separate tests out and use docstrings Signed-off-by: Angel Luu <[email protected]> * Update more field/value checks from HF defaults Signed-off-by: Angel Luu <[email protected]> * Fix: Addition of env var TRANSFORMERS_VERBOSITY check Signed-off-by: Abhishek <[email protected]> * FMT Fix: Addition of env var TRANSFORMERS_VERBOSITY check Signed-off-by: Abhishek <[email protected]> * Add test for tokenizer in lora config (should be ignored) Signed-off-by: Angel Luu <[email protected]> * Adding logging support to accelerate launch Signed-off-by: Abhishek <[email protected]> * FMT_FIX: Adding logging support to accelerate launch Signed-off-by: Abhishek <[email protected]> * bug: On save event added to callback (#256) * feat: On save event added to callback Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Removed additional bracket Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Removed additional bracket Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Format issues resolved Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: rebase with upstream and add new line Signed-off-by: Mehant Kammakomati <[email protected]> --------- Signed-off-by: Padmanabha V Seshadri <[email protected]> Signed-off-by: Mehant Kammakomati <[email protected]> Co-authored-by: Mehant Kammakomati <[email protected]> * feat: All metric handling changes (#263) * feat: All metric handling changes Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Format issues Signed-off-by: Padmanabha V Seshadri <[email protected]> --------- Signed-off-by: Padmanabha V Seshadri <[email protected]> * feat: Configuration to set logging level for trigger log (#241) * feat: Added the triggered login in the operation Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Formatting issues Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Added default config Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Moved the variable to right scope Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Checked added to validate config log level Signed-off-by: Padmanabha V Seshadri <[email protected]> * fix: Removed some unwanted log file Signed-off-by: Padmanabha V Seshadri <[email protected]> --------- Signed-off-by: Padmanabha V Seshadri <[email protected]> * limit peft deps until investigate (#274) Signed-off-by: Anh-Uong <[email protected]> * Data custom collator (#260) * refactor code to preprocess datasets Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * fix formatting Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * allow input/output in validate args Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * format input/output JSON and mask Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * function to return suitable collator Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * add tests for SFT Trainer input/output format Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * remove unused functions Co-authored-by: Alex-Brooks <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> * add eos token to input/output format Signed-off-by: Sukriti-Sharma4 <[email protected]> * fix tests Signed-off-by: Sukriti-Sharma4 <[email protected]> * improve docstrings Signed-off-by: Sukriti-Sharma4 <[email protected]> * keeping JSON keys constant Signed-off-by: Sukriti-Sharma4 <[email protected]> * support for input/output format Signed-off-by: Sukriti-Sharma4 <[email protected]> * formatting fixes Signed-off-by: Sukriti-Sharma4 <[email protected]> * update rEADME formats Signed-off-by: Sukriti-Sharma4 <[email protected]> * formatting README Signed-off-by: Sukriti-Sharma4 <[email protected]> --------- Signed-off-by: Sukriti-Sharma4 <[email protected]> Co-authored-by: Alex-Brooks <[email protected]> * Revert "limit peft deps until investigate (#274)" (#275) This reverts commit f57ff63. Signed-off-by: Anh-Uong <[email protected]> * feat: per process state metric (#239) Signed-off-by: Harikrishnan Balagopal <[email protected]> * Modify test to pass with target_modules: None Signed-off-by: Will Johnson <[email protected]> * Logging changes and unit tests added Signed-off-by: Abhishek <[email protected]> * feat: Add a dockerfile argument to enable aimstack (#261) * Add a dockerfile argument at the end of final layer to enable aimstack. Currenlty guarded by a dockerfile argument. Signed-off-by: Dushyant Behl <[email protected]> * Set the default value of ENABLE_AIM to false Signed-off-by: Dushyant Behl <[email protected]> --------- Signed-off-by: Dushyant Behl <[email protected]> * Solved conflict with main Signed-off-by: Abhishek <[email protected]> * FMT:Fix Solved conflict with main Signed-off-by: Abhishek <[email protected]> * enabling tests for prompt tuning Signed-off-by: Abhishek <[email protected]> * feat: Support pretokenized (#272) * feat: support pretokenized datasets Signed-off-by: Mehant Kammakomati <[email protected]> * fix: rebase with upstream and review commits Signed-off-by: Mehant Kammakomati <[email protected]> * fix: rebase with upstream and review commits Signed-off-by: Mehant Kammakomati <[email protected]> * fix: rebase with upstream and review commits Signed-off-by: Mehant Kammakomati <[email protected]> * consolidate collator code Signed-off-by: Sukriti-Sharma4 <[email protected]> * add valuerrors for incorrect args Signed-off-by: Sukriti-Sharma4 <[email protected]> * feat: add unit tests for validate_data_args and format_dataset Signed-off-by: Mehant Kammakomati <[email protected]> * feat: add unit tests for validate_data_args and format_dataset Signed-off-by: Mehant Kammakomati <[email protected]> * feat: add unit tests for validate_data_args and format_dataset Signed-off-by: Mehant Kammakomati <[email protected]> * feat: add unit tests for validate_data_args and format_dataset Signed-off-by: Mehant Kammakomati <[email protected]> --------- Signed-off-by: Mehant Kammakomati <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> Co-authored-by: Sukriti-Sharma4 <[email protected]> Co-authored-by: Alex Brooks <[email protected]> * Update packaging requirement from <24,>=23.2 to >=23.2,<25 (#212) Updates the requirements on [packaging](https://github.com/pypa/packaging) to permit the latest version. - [Release notes](https://github.com/pypa/packaging/releases) - [Changelog](https://github.com/pypa/packaging/blob/main/CHANGELOG.rst) - [Commits](pypa/packaging@23.2...24.1) --- updated-dependencies: - dependency-name: packaging dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Anh Uong <[email protected]> * enabling tests for prompt tuning (#278) Signed-off-by: Abhishek <[email protected]> Co-authored-by: Anh Uong <[email protected]> * fix: do not add special tokens for custom tokenizer (#279) Signed-off-by: Mehant Kammakomati <[email protected]> * PR changes for changing logger Signed-off-by: Abhishek <[email protected]> * fix: bug where the logger was not being used properly (#286) Signed-off-by: Hari <[email protected]> * Unit Tests changes Signed-off-by: Abhishek <[email protected]> * Add functionality to free disk space from Github Actions (#287) * Add functionality to free disk space from Github Actions Signed-off-by: Will Johnson <[email protected]> * Add functionality to free disk space from Github Actions, relocate from build-and-publish.yaml to image.yaml Signed-off-by: Will Johnson <[email protected]> * Move freeing space step to before building image Signed-off-by: Will Johnson <[email protected]> --------- Signed-off-by: Will Johnson <[email protected]> * commented os.environ[LOG_LEVEL] in accelerate.py for testing Signed-off-by: Abhishek <[email protected]> * PR changes Signed-off-by: Abhishek <[email protected]> * FIX:FMT Signed-off-by: Abhishek <[email protected]> * PR Changes Signed-off-by: Abhishek <[email protected]> * PR Changes Signed-off-by: Abhishek <[email protected]> * Add unit test to verify target_modules defaults correctly (#281) * Add unit test to verify target_modules defaults correctly Signed-off-by: Will Johnson <[email protected]> * Add sft_trainer.main test to ensure target modules properly default for LoRA when set to None from CLI Signed-off-by: Will Johnson <[email protected]> * fmt Signed-off-by: Will Johnson <[email protected]> * Use model_args instead of importing, fix nits Signed-off-by: Will Johnson <[email protected]> * Add test to ensure target_modules defaults to None in job config Signed-off-by: Will Johnson <[email protected]> * Add additional check, fix nits Signed-off-by: Will Johnson <[email protected]> --------- Signed-off-by: Will Johnson <[email protected]> * docs: Add documentation on experiment tracking. (#257) Signed-off-by: Dushyant Behl <[email protected]> * Ensure additional metadata to trackers don't throw error in happy case. (#290) Signed-off-by: Dushyant Behl <[email protected]> * PR Changes Signed-off-by: Abhishek <[email protected]> * fix multiple runid creation bug with accelerate. (#268) Signed-off-by: Dushyant Behl <[email protected]> * feat: logging control operation (#264) Signed-off-by: Padmanabha V Seshadri <[email protected]> * Metrics file epoch indexing from 0 Signed-off-by: Abhishek <[email protected]> * Revert last commit Signed-off-by: Abhishek <[email protected]> * fix run evaluation to get base model path (#273) Signed-off-by: Anh-Uong <[email protected]> * PR Changes Signed-off-by: Abhishek <[email protected]> * PR Changes Signed-off-by: Abhishek <[email protected]> * feat: Added additional events such as on_step_begin, on_optimizer_step, on_substep_end (#293) Signed-off-by: Padmanabha V Seshadri <[email protected]> * Always update setuptools to latest (#288) Signed-off-by: James Busche <[email protected]> Co-authored-by: Anh Uong <[email protected]> * Rename all fixtures with correct .jsonl extension (#295) Signed-off-by: Will Johnson <[email protected]> Co-authored-by: Anh Uong <[email protected]> * feat: add save_model_dir flag where final checkpoint saved (#291) * add save_model_dir flag for final checkpoint Signed-off-by: Anh-Uong <[email protected]> * remove output_dir logic, add save method Signed-off-by: Anh-Uong <[email protected]> * update accelerate_launch, remove save tokenizer Signed-off-by: Anh-Uong <[email protected]> * fix: put back creation of .complete file Signed-off-by: Anh-Uong <[email protected]> * fix failing tests and add new ones Signed-off-by: Anh-Uong <[email protected]> * tests: add sft_trainer test to train and save - small refactor of tests Signed-off-by: Anh-Uong <[email protected]> * add docs on saving checkpoints and fix help msg Signed-off-by: Anh-Uong <[email protected]> * update example and note best checkpoint Signed-off-by: Anh-Uong <[email protected]> * changes based on PR review Signed-off-by: Anh-Uong <[email protected]> * add logging to save, fix error out properly Signed-off-by: Anh-Uong <[email protected]> --------- Signed-off-by: Anh-Uong <[email protected]> --------- Signed-off-by: Will Johnson <[email protected]> Signed-off-by: Abhishek <[email protected]> Signed-off-by: Anh-Uong <[email protected]> Signed-off-by: Angel Luu <[email protected]> Signed-off-by: Padmanabha V Seshadri <[email protected]> Signed-off-by: Mehant Kammakomati <[email protected]> Signed-off-by: Sukriti-Sharma4 <[email protected]> Signed-off-by: Harikrishnan Balagopal <[email protected]> Signed-off-by: Dushyant Behl <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Hari <[email protected]> Signed-off-by: James Busche <[email protected]> Co-authored-by: Abhishek <[email protected]> Co-authored-by: Sukriti Sharma <[email protected]> Co-authored-by: Anh-Uong <[email protected]> Co-authored-by: Abhishek Maurya <[email protected]> Co-authored-by: Angel Luu <[email protected]> Co-authored-by: Angel Luu <[email protected]> Co-authored-by: Padmanabha V Seshadri <[email protected]> Co-authored-by: Mehant Kammakomati <[email protected]> Co-authored-by: Alex-Brooks <[email protected]> Co-authored-by: Hari <[email protected]> Co-authored-by: Dushyant Behl <[email protected]> Co-authored-by: Sukriti-Sharma4 <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Busche <[email protected]>
Description of the change
Add a dockerfile argument to enable aimstack
Related issue number
NA
How to verify the PR
Run with a dockerfile built with and without the argument
ENABLE_AIM
Was the PR tested
Tested the PR with both scenarios,
ENABLE_AIM
does not set andENABLE_AIM=true
Tested without
ENABLE_AIM
argumentVerification with
pip freeze
Tested With
ENABLE_AIM=true
build argVerification with
pip freeze