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.

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] Oneshot Refactor #1041

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

[Oneshot] Oneshot Refactor #1041

wants to merge 36 commits into from

Conversation

horheynm
Copy link
Collaborator

@horheynm horheynm commented Jan 7, 2025

SUMMARY:
Edit oneshot pathway to use only the necessary code.

Problem:
Oneshot uses a pathway that is shared in other entrypoints, eg. train.
Oneshot doesnt train, so do not need use code that trains, as an example of redundant code. Remove other parts of the code and refactor

Design
Screenshot 2025-01-16 at 2 36 33 PM

Changes:

  • Oneshot class responsible for carrying out the oneshot pipeline
  • Oneshot run will not depend on the session. Only use CompressionLifecycle that has modifer logic support. Places that uses active_session, such as saving to the compressed model by reading the stagemodifiers, has been replaced by passing in the necessary args

TEST PLAN:
Pass existing passing tests
Pass all finetune tests merging transformer main for HFQuantizer support
Verified new pathway has the same scales for the int8 w8a8 dynamic per token values for Meta-Llama-3-8B-Instruct

Copy link

github-actions bot commented Jan 7, 2025

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Copy link
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

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

It seems like we're introducing more abstractions, not fewer, when separating out the oneshot entrpoint and I'm not sure these are necessary.

we also seem to be repeating a lot of code everywhere.

examples/quantization_w8a8_int8/llama3_example.py Outdated Show resolved Hide resolved
examples/quantization_w8a8_fp8/llama3_example.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/finetune/session_mixin.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/finetune/text_generation.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/finetune/model_args.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/finetune/trainer.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/calibration/oneshot.py Outdated Show resolved Hide resolved
src/llmcompressor/transformers/calibration/oneshot.py Outdated Show resolved Hide resolved
@dsikka
Copy link
Collaborator

dsikka commented Jan 8, 2025

Screenshot 2025-01-07 at 6 49 43 PM SUMMARY: Edit oneshot pathway to use only the necessary code.

Problem: Oneshot uses a pathway that is shared in other entrypoints, eg. train. Oneshot doesnt train, so do not need use code that trains, as an example of redundant code. Remove other parts of the code and refactor

Changes:

  • Oneshot run will not depend on the session. Only use CompressionLifecycle that has modifer logic support.
  • Return model as a return type to oneshot to obtain model without relying on session.

TEST PLAN: Pass existing passing tests Verified new pathway has the same scales for the int8 w8a8 dynamic per token values for Meta-Llama-3-8B-Instruct

Please update the PR desription showing what the updated entrypoint looks like as a result of this PR.

@horheynm
Copy link
Collaborator Author

horheynm commented Jan 9, 2025

@dsikka Thanks for looking at the pr early. Yes thats the next to do before the full review. One ready i will ping you!

@horheynm horheynm changed the title [Draft] Oneshot entrypoint refactor [Oneshot] Oneshot Refactor Jan 10, 2025
@@ -75,7 +75,6 @@ def test_oneshot_application(self):
model=self.model,
dataset=self.dataset,
output_dir=self.output,
overwrite_output_dir=True,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylesayrs kylesayrs self-requested a review January 23, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants