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

Refactor Recipe creation flow: Directly convert Modifier Instances to Recipe #48

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

Conversation

rahul-tuli
Copy link
Collaborator

@rahul-tuli rahul-tuli commented Aug 1, 2024

This PR refactors the Recipe creation process. The original flow converted Modifier Instances to a recipe string before creating a Recipe. We've simplified this by directly converting Modifier Instances to a Recipe.

  • Updated from_modifiers to create a Recipe from Modifier Instances directly.
  • Simplified validation for the modifiers parameter.
  • Remove relevant unit tests for old flow

Test plan:

Tested using the scripts/recipes from #37.

Added automated e2e tests:

pytest -vv tests/e2e/test_recipe_parsing.py
=========================================================================== test session starts ============================================================================
platform linux -- Python 3.10.12, pytest-8.2.2, pluggy-1.5.0 -- /root/llm-compressor/venv/bin/python3
cachedir: .pytest_cache
rootdir: /root/llm-compressor
configfile: pyproject.toml
plugins: mock-3.14.0, rerunfailures-14.0
collected 3 items                                                                                                                                                          

tests/e2e/test_recipe_parsing.py::test_oneshot[recipe0] PASSED                                                                                                       [ 33%]
tests/e2e/test_recipe_parsing.py::test_oneshot[\nDEFAULT_stage:\n  DEFAULT_modifiers:\n    SmoothQuantModifier:\n      smoothing_strength: 0.8\n      mappings:\n      - - ['re:.*q_proj', 're:.*k_proj', 're:.*v_proj']\n        - re:.*input_layernorm\n      - - ['re:.*gate_proj', 're:.*up_proj']\n        - re:.*post_attention_layernorm\n    GPTQModifier:\n      sequential_update: false\n      targets: Linear\n      scheme: W8A8\n] PASSED [ 66%]
tests/e2e/test_recipe_parsing.py::test_oneshot[/root/llm-compressor/tests/e2e/recipe.yaml] PASSED    

@rahul-tuli rahul-tuli marked this pull request as ready for review August 1, 2024 15:39
@rahul-tuli rahul-tuli self-assigned this Aug 1, 2024
Copy link
Contributor

@bfineran bfineran left a comment

Choose a reason for hiding this comment

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

LGTM pending test

@@ -3,10 +3,8 @@
import pytest
import yaml

from llmcompressor.modifiers import Modifier
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a new test for a list of modifiers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added Ben!

Copy link
Contributor

@Satrat Satrat left a comment

Choose a reason for hiding this comment

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

Can we add in a test case the tests the bug reported in #37? Reproduced on a tiny model of course. Otherwise LGTM, just make sure to manually run the unit tests before merging since GHA is broken for all the tests requiring runners

@rahul-tuli
Copy link
Collaborator Author

Can we add in a test case the tests the bug reported in #37? Reproduced on a tiny model of course. Otherwise LGTM, just make sure to manually run the unit tests before merging since GHA is broken for all the tests requiring runners

Added e2e tests in last commit and ran them manually:

pytest -vv tests/e2e/test_recipe_parsing.py
=========================================================================== test session starts ============================================================================
platform linux -- Python 3.10.12, pytest-8.2.2, pluggy-1.5.0 -- /root/llm-compressor/venv/bin/python3
cachedir: .pytest_cache
rootdir: /root/llm-compressor
configfile: pyproject.toml
plugins: mock-3.14.0, rerunfailures-14.0
collected 3 items                                                                                                                                                          

tests/e2e/test_recipe_parsing.py::test_oneshot[recipe0] PASSED                                                                                                       [ 33%]
tests/e2e/test_recipe_parsing.py::test_oneshot[\nDEFAULT_stage:\n  DEFAULT_modifiers:\n    SmoothQuantModifier:\n      smoothing_strength: 0.8\n      mappings:\n      - - ['re:.*q_proj', 're:.*k_proj', 're:.*v_proj']\n        - re:.*input_layernorm\n      - - ['re:.*gate_proj', 're:.*up_proj']\n        - re:.*post_attention_layernorm\n    GPTQModifier:\n      sequential_update: false\n      targets: Linear\n      scheme: W8A8\n] PASSED [ 66%]
tests/e2e/test_recipe_parsing.py::test_oneshot[/root/llm-compressor/tests/e2e/recipe.yaml] PASSED    

@rahul-tuli
Copy link
Collaborator Author

Resolves #37 ; and #886

markmc pushed a commit to markmc/llm-compressor that referenced this pull request Nov 13, 2024
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.

3 participants