-
Notifications
You must be signed in to change notification settings - Fork 78
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
[Oneshot Refactor] dataclass Arguments #1103
Conversation
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
@horheynm Can you touchbase with Mark on the data arguments he shared during the meeting this morning? |
Is this ready for review? I'm a little confused what the comment "make changes in a different pr" means |
Yes |
It means it still needs to be broken down, it should only contain dataargs only. |
…lm-compressor into oneshot-refac-recipe_args
@horheynm Is this ready for review now? |
@kylesayrs yes |
Please resolve conflicts |
https://docs.google.com/document/d/1YbR1dTQmCzqhGk74m5msBzqoPHQgB6dVxDtf6cTmetc/edit?usp=sharing Does this cover everything? |
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.
Overall looks like a good change but I would go one step further and move the args from under transformers
altogether plus add docs for the restructured args.
src/llmcompressor/transformers/utils/arg_parser/training_arguments.py
Outdated
Show resolved
Hide resolved
src/llmcompressor/transformers/utils/arg_parser/model_arguments.py
Outdated
Show resolved
Hide resolved
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.
few comments for ya
tests/llmcompressor/transformers/sparsification/test_compress_tensor_utils.py
Show resolved
Hide resolved
Co-authored-by: Rahul Tuli <[email protected]>
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, but should re-request review from Dipika or Rahul before merging in
7f49448
ORDER OF REVIEWS:
SUMMARY:
Refactor dataclass used for llm-compressor entrypoints (oneshot, train, apply) to decouple non-relevant attributes from the existing dataclass. Ex. recipe in training_args. Recipe is contained in a session, not in the trainer that training_args govern.
Dataclass refactor details are in
https://docs.google.com/document/d/1YbR1dTQmCzqhGk74m5msBzqoPHQgB6dVxDtf6cTmetc/edit?usp=sharing
Note:
#1110 takes care of using a new entrypoint that will prohibit the post_train / oneshot call to use training_args. Current entrypoint will need training_args for oneshot to function - this PR is just for refactoring the dataclass.
Before:
ModelArguments:
llm-compressor/src/llmcompressor/transformers/finetune/model_args.py
Line 6 in 6fa5a5e
DataTrainingArguments:
llm-compressor/src/llmcompressor/transformers/finetune/data/data_args.py
Line 70 in 6fa5a5e
TrainingArguments:
llm-compressor/src/llmcompressor/transformers/finetune/training_args.py
Line 10 in 6fa5a5e
After:
ModelArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-58fd0f7ae4564317960ae0d4d4b2cdb97b9588c1915f062915e74ecf51b5502cR6
DatasetArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-5e43f74ba5d8327b937adada3c7f30a7efb13f9a44cb3fdb5e1a2a12b8b8ea27R70
RecipeArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-0ff9c048a4deb55e5459054bdc61a5d8c81da9c94588ec2355e6b2c2ec8675d1R6
TrainingArguments: https://github.com/vllm-project/llm-compressor/pull/1103/files#diff-249ee96763dd50956a7309f898eda4bcaa91c6af653474568fbda10b5a39c817R12
TEST PLAN:
pygrep