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.

Sign up for GitHub

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

Jump to bottom

[Oneshot Refactor] Rename get_shared_processor_src to get_processor_name_from_model #1108

Merged
merged 6 commits into from
Feb 5, 2025
Merged

[Oneshot Refactor] Rename get_shared_processor_src to get_processor_name_from_model #1108

merged 6 commits into from
Feb 5, 2025

Conversation

horheynm
Copy link
Collaborator

@horheynm horheynm commented Jan 28, 2025 •
edited
Loading

ORDER OF REVIEWS:

  1. [Oneshot Refactor] Rename get_shared_processor_src to get_processor_name_from_model #1108 <- current PR
  2. [Oneshot Refactor] dataclass Arguments #1103
  3. [Oneshot refactor] Refactor initialize_model_from_path #1109
  4. [Oneshot Refactor] Main refactor #1110

SUMMARY:

  • Rename get_shared_processor_src to get_processor_from_model
  • Appropriate signature on initialize_processor_from_path, where teacher should be optinal

TEST PLAN:

  • Pass all existing tests
  • Search get_shared_processor_src using pygrep
  3 function pygrep() {
  2     local search_term="$1"
  1     shift
126     local search_dirs="${*:-src examples tests}"                           
  1     grep -rn --include="*.py" -E "$search_term" $search_dirs
  2 }

@github-actions GitHub Actions
Copy link

👋 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 horheynm added the ready When a PR is ready for review label Jan 28, 2025
@horheynm horheynm changed the title Rename get_shared_processor_src to get_processor_from_model [Oneshot Refactor] Rename get_shared_processor_src to get_processor_from_model Jan 28, 2025
kylesayrs
kylesayrs reviewed Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@kylesayrs kylesayrs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, thank you for breaking this out

@horheynm
Copy link
Collaborator Author

lgtm, thank you for breaking this out

No problem, Its easier to review

This was referenced Feb 3, 2025
Merged
Merged
Merged
rahul-tuli
rahul-tuli previously approved these changes Feb 4, 2025
View reviewed changes
@kylesayrs kylesayrs changed the title [Oneshot Refactor] Rename get_shared_processor_src to get_processor_from_model [Oneshot Refactor] Rename get_shared_processor_src to get_processor_name_from_model Feb 4, 2025
kylesayrs
kylesayrs approved these changes Feb 4, 2025
View reviewed changes
Copy link
Collaborator

@kylesayrs kylesayrs left a comment

Choose a reason for hiding this comment

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

I still prefer the original name get_shared_processor_src but this change is fine

@dsikka dsikka enabled auto-merge (squash) February 5, 2025 21:17
@dsikka dsikka disabled auto-merge February 5, 2025 22:40
@dsikka dsikka merged commit caee1c8 into main Feb 5, 2025
7 checks passed
@dsikka dsikka deleted the oneshot-refac-rename-tokenizer branch February 5, 2025 22:40
dsikka pushed a commit that referenced this pull request Feb 11, 2025
@horheynm @rahul-tuli
[Oneshot Refactor] dataclass Arguments (#1103)
e604f41 
ORDER OF REVIEWS:
1. #1108
2. #1103 <- current
PR
3. #1109
4. #1110

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:
https://github.com/vllm-project/llm-compressor/blob/6fa5a5eecc7d363ec73474d011d40135b6374179/src/llmcompressor/transformers/finetune/model_args.py#L6
DataTrainingArguments:
https://github.com/vllm-project/llm-compressor/blob/6fa5a5eecc7d363ec73474d011d40135b6374179/src/llmcompressor/transformers/finetune/data/data_args.py#L70
TrainingArguments:
https://github.com/vllm-project/llm-compressor/blob/6fa5a5eecc7d363ec73474d011d40135b6374179/src/llmcompressor/transformers/finetune/training_args.py#L10

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:
* Pass all existing tests
* Search dataclass arguments using `pygrep`
```bash
  3 function pygrep() {
  2     local search_term="$1"
  1     shift
126     local search_dirs="${*:-src examples tests}"                           
  1     grep -rn --include="*.py" -E "$search_term" $search_dirs
  2 }
```

---------

Co-authored-by: Rahul Tuli <[email protected]>
dsikka pushed a commit that referenced this pull request Feb 11, 2025
@horheynm
[Oneshot refactor] Refactor initialize_model_from_path (#1109)
74150cb 
ORDER OF REVIEWS:
1. #1108
2. #1103 
3. #1109 <- current
PR
4. #1110


SUMMARY:
Refactor `initialize_model_from_path` to decouple `training_args`
dependent logic and oneshot (non-training_args) logic.

TEST PLAN:
* Pass all tests
* search `initialize_model_from_path` using `grep`
dsikka pushed a commit that referenced this pull request Feb 24, 2025
@horheynm
[Oneshot Refactor] Main refactor (#1110)
865364f 
ORDER OF REVIEWS:
1. #1108
2. #1103 
3. #1109
4. #1110 <- current
PR


SUMMARY:
* Create a class to decouple dependency to `main`. Class `Oneshot`
consists of pre-processing, carrying out oneshot logic and
post-processing
* Move the oneshot class and method under
`llmcompressor/entrypoints/oneshot.py`.
* Add ReadMe in `/llmcompressor/entrypoints` for info on oneshot
* Delete oneshot logic from `/finetune` directory, add deprecation
warning
* Remove apply used only for stagerunner oneshot pathway in session.py
and session_function.py
* Add oneshot only calibration dataloader logic
* Add a return variable of `model: PretrainedModel` for `def oneshot`.
* Make oneshot carryout logic independent of `TrainingArguments`
* remove `overwrite_output_dir` as oneshot input arg -> only used for
`TrainingArguments`
* Update README on `/finetune` path. Remove `oneshot` logic and `oneshot
with fsdp`
* Update `wrap_save_pretrained` logic to run only if not updated already
-> used for stage runner to avoid double wrapping



Entrypoints: 
```python3
from llmcompressor import oneshot
oneshot(**kwargs) # calls Oneshot
```

or 

```python3
from llmcompressor import Oneshot
oneshot = Oneshot(**kwargs)
oneshot() # preprocesss, carries out logic and post process
```


TEST PLAN:
Pass all tests and examples. 
Verified
https://github.com/vllm-project/llm-compressor/blob/main/examples/quantization_2of4_sparse_w4a16/llama7b_sparse_w4a16.py
works as expected.

FOLLOW UPS:
* Stage runner removal
* Update entrypoints folder with train, eval, predict, etc.

---------

Signed-off-by: George Ohashi <[email protected]>
Signed-off-by: George <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylesayrs kylesayrs kylesayrs approved these changes

@dsikka dsikka dsikka approved these changes

@rahul-tuli rahul-tuli rahul-tuli left review comments

Assignees

@horheynm horheynm

Labels
ready When a PR is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@horheynm @kylesayrs @dsikka @rahul-tuli