-
Notifications
You must be signed in to change notification settings - Fork 157
Removed recipeStage, recipeModifer, stageModifier classes #1514
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
👋 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. |
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.
Hello @shanjiaz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR, authored by shanjiaz, is a work in progress aimed at simplifying the internal representation of recipes and modifiers. The core change involves removing the recipeStage
, recipeModifier
, and stageModifier
classes and updating the codebase to use a flatter structure where the Recipe
class directly holds a list of Modifier
instances.
Highlights
- Recipe Structure Simplification: The hierarchical structure involving
recipeStage
,recipeModifier
, andstageModifier
has been removed. TheRecipe
class now directly manages a list ofModifier
instances. - Codebase Adaptation: Various parts of the codebase, including the lifecycle, session, pipelines, and transformers, have been updated to reflect the removal of the old stage/modifier classes and interact directly with the
Recipe
's list ofModifier
objects. - Recipe Parsing and Serialization: The
Recipe
class includes a new model validator (parse_from_dict
) to maintain backward compatibility for loading recipes defined in the old staged YAML format. The serialization logic (yaml
,dict
) has been updated to handle the new internal structure, although the YAML output format attempts to retain some grouping structure. - Recipe Combination Logic Removed: The
simplify_combine_recipes
method and related logic for combining multiple recipes have been removed from theRecipe
class and theupdate_and_save_recipe
utility.
Changelog
Click here to see the changelog
- src/llmcompressor/core/lifecycle.py
- Updated imports and type hints to use
Modifier
instead ofStageModifiers
. - Changed the
modifiers
field type fromList[StageModifiers]
toList[Modifier]
.
- Updated imports and type hints to use
- src/llmcompressor/core/session.py
- Removed the
get_modifiers
method, which previously aggregated modifiers fromStageModifiers
.
- Removed the
- src/llmcompressor/entrypoints/oneshot.py
- Updated access to modifiers from
session.get_modifiers()
tosession.lifecycle.modifiers
.
- Updated access to modifiers from
- src/llmcompressor/modifiers/init.py
- Removed
StageModifiers
from the__all__
list.
- Removed
- src/llmcompressor/pipelines/independent/pipeline.py
- Removed import of
StageModifiers
. - Updated the
__call__
method to iterate directly oversession.lifecycle.modifiers
instead of creating temporaryStageModifiers
instances.
- Removed import of
- src/llmcompressor/pipelines/layer_sequential/pipeline.py
- Updated access to modifiers from
session.get_modifiers()
tosession.lifecycle.modifiers
.
- Updated access to modifiers from
- src/llmcompressor/pipelines/registry.py
- Added a blank line in
from_modifiers
.
- Added a blank line in
- src/llmcompressor/pipelines/sequential/helpers.py
- Removed a blank line in
get_targets_from_modifiers
.
- Removed a blank line in
- src/llmcompressor/pipelines/sequential/pipeline.py
- Updated access to modifiers from
session.get_modifiers()
tosession.lifecycle.modifiers
.
- Updated access to modifiers from
- src/llmcompressor/recipe/init.py
- Removed imports and
__all__
entries forRecipeBase
,RecipeModifier
, andRecipeStage
.
- Removed imports and
- src/llmcompressor/recipe/recipe.py
- Removed imports for
RecipeBase
,RecipeModifier
,RecipeStage
. Added imports forBaseModel
,ConfigDict
, andModifierFactory
. - Changed
Recipe
to inherit fromBaseModel
. - Added explicit fields (
version
,args
,stage
,modifiers
) andmodel_config
. - Updated
from_modifiers
to directly assign modifiers and stage. - Added
parse_from_dict
model validator to handle parsing the old staged recipe format. - Updated
simplify_recipe
to filter modifiers based ontarget_stage
. - Updated
simplify_combine_recipes
to extend themodifiers
list and updateargs
. - Updated
create_modifier
to instantiateModifier
objects from the stored list. - Removed
remap_stages
andextract_dict_stages
methods. - Updated
dict
andyaml
methods to use the newget_yaml_serializable_dict
helper. - Removed the
_get_yaml_dict
method. - Renamed
get_yaml_serializable_stage_dict
toget_yaml_serializable_dict
and updated its logic for the new structure. - Added a
traceback.print_stack()
call in theyaml
method (likely for debugging).
- Removed imports for
- src/llmcompressor/transformers/compression/helpers.py
- Added import for
Modifier
. - Updated
infer_sparsity_structure_from_stage_modifiers
to acceptList[Modifier]
and iterate directly.
- Added import for
- src/llmcompressor/transformers/sparsification/compressed_tensors_utils.py
- Simplified
update_and_save_recipe
to save the current recipe, potentially renaming the file if an existing recipe is found. - Removed the logic for combining recipes using
simplify_combine_recipes
.
- Simplified
- tests/llmcompressor/helpers.py
- Removed a test recipe string that used multiple stages.
- tests/llmcompressor/recipe/test_recipe.py
- Updated tests to reflect the removal of
StageModifiers
and the new structure. - Added
create_modifier()
calls intest_serialization
.
- Updated tests to reflect the removal of
- tests/llmcompressor/transformers/finetune/test_oneshot_then_finetune.py
- Commented out the
shutil.rmtree
call intearDown
.
- Commented out the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Stages removed, stages gone,
A flatter recipe carries on.
Modifiers now, a simple list,
Complexity, gently dismissed.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request focuses on refactoring the recipe handling mechanism by removing RecipeStage
, RecipeModifier
, and StageModifier
classes. This simplification is a significant step towards a more streamlined recipe structure. The changes are generally well-implemented and align with the PR's objectives.
I've identified a few areas that need attention, primarily a debugging artifact, a potential unintended behavior change, and a docstring update. Given this is a Work In Progress (WIP), these points should help in finalizing the changes.
Overall, good progress on simplifying the recipe system!
Summary of Findings
- Debugging Artifact: A
traceback.print_stack()
call was found insrc/llmcompressor/recipe/recipe.py
, which should be removed. - Docstring Example Update Needed: The docstring example for
Recipe.create_modifier()
insrc/llmcompressor/recipe/recipe.py
needs to be updated to reflect the new return type and structure after refactoring. - Potential Behavior Change in Recipe Saving: The
update_and_save_recipe
function insrc/llmcompressor/transformers/sparsification/compressed_tensors_utils.py
no longer merges existing recipes with the current session's recipe. Instead, it saves the new recipe separately if an old one exists. This behavior change should be confirmed. - Test Cleanup: The
tearDown
method intests/llmcompressor/transformers/finetune/test_oneshot_then_finetune.py
has its directory cleanup logic commented out. This should be restored for proper test hygiene. - Whitespace Changes: Minor whitespace changes (addition/removal of blank lines) were observed in several files. These are stylistic and do not affect functionality. (Not commented inline due to severity settings)
- Recipe Modifier State Management: In
src/llmcompressor/recipe/recipe.py
, theself.modifiers
attribute can be eitherList[Dict[str, Any]]
orList[Modifier]
. Thedict()
andyaml()
methods rely onget_yaml_serializable_dict
which expectsList[Modifier]
. This impliescreate_modifier()
must be called before serialization if the recipe was loaded from a file/string. This implicit dependency could be documented or the design made more robust. (Not commented inline due to severity settings)
Merge Readiness
This pull request is currently marked as Work In Progress (WIP), and there are a few critical
and medium
severity issues identified that should be addressed before considering it for merging. Specifically, the debugging traceback
call needs to be removed, the docstring example updated, the recipe saving behavior confirmed, and test cleanup restored. Once these points are resolved, the PR will be in a much better state. As a reviewer, I am not authorized to approve pull requests, but I recommend addressing these findings.
src/llmcompressor/transformers/sparsification/compressed_tensors_utils.py
Outdated
Show resolved
Hide resolved
tests/llmcompressor/transformers/finetune/test_oneshot_then_finetune.py
Outdated
Show resolved
Hide resolved
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
This reverts commit f60d113.
Signed-off-by: shanjiaz <[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.
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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.
looks good but got a breakpoint()
in code. also can you remove [WIP]
from PR header if it's ready?
Signed-off-by: shanjiaz <[email protected]>
src/llmcompressor/transformers/sparsification/compressed_tensors_utils.py
Show resolved
Hide resolved
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
…oject/llm-compressor into hz-remove-recipemodifier
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[email protected]>
Signed-off-by: shanjiaz <[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.
I think this is looking good!
4e56d0f
to
8f7ecf5
Compare
Signed-off-by: shanjiaz <[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.
Looks like all references to modifiers are caught, good job
grep -r '\.modifier' src/ tests/ --include="*.py" | grep -v 'from'
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.
Really nice job. My big question is whether we can fold Recipe.create_modifier
into the Recipe construction step, since it seems like recipe creation is always followed by create_modifier
. This would simplify the Recipe.modifier
type hint too.
Other than that, just a few nits and questions. Awesome job!
@@ -135,7 +136,7 @@ def create_instance( | |||
) | |||
logger.debug(f"Input string: {path_or_modifiers}") | |||
obj = _load_json_or_yaml_string(path_or_modifiers) | |||
return Recipe.model_validate(obj) | |||
return Recipe.model_validate(filter_dict(obj, target_stage=target_stage)) |
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.
Did you mean to pass a recipe_stage argument to Recipe here?
Also, why use model_validate rather than create_instance?
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.
This is inside the create_instance
, if I call create_instance
here it'll end up in infinite recursive calls?
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.
I mean that the recipe which you have created does not know which stage it is
Maybe you want something like this
recipe_dict = _load_json_or_yaml_string(path_or_modifiers)
recipe_dict = filter_dict(obj, target_stage=target_stage)
Recipe.create_recipe(modifiers=recipe_dict, stage=target_stage)
version: str = None | ||
args: Dict[str, Any] = Field(default_factory=dict) | ||
stages: List[RecipeStage] = Field(default_factory=list) | ||
return Recipe.model_validate(filter_dict(obj, target_stage=target_stage)) |
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.
Same here, did you mean to pass stage to reipce creation and why not create using create_instance?
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.
same as above.
@@ -31,6 +34,13 @@ class Recipe(RecipeBase): | |||
when serializing a recipe, yaml will be used by default. | |||
""" | |||
|
|||
version: Optional[str] = None |
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.
If this isn't being used then we should just remove it
sequential_targets = get_sequential_targets(modifiers, model, dataset_args) | ||
|
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.
nit: remove
test2_stage: | ||
MagnitudePruningModifier: | ||
start: 5 | ||
end: 10 | ||
init_sparsity: 0.1 | ||
final_sparsity: 0.5 | ||
targets: __ALL_PRUNABLE__ |
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.
Why is this test removed?
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.
This is the only test that assumes recipe should work with multiple stages. I could rewrite the test, at the moment I thought removing the second stage is faster.
@@ -52,8 +54,7 @@ def test_recipe_creates_correct_modifier(): | |||
recipe_instance = Recipe.create_instance(yaml_str) | |||
|
|||
stage_modifiers = recipe_instance.create_modifier() | |||
assert len(stage_modifiers) == 1 | |||
assert len(modifiers := stage_modifiers[0].modifiers) == 1 | |||
assert len(modifiers := stage_modifiers) == 1 |
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.
Do you know what this assertion test is really doing? Do you think we can remove it?
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.
Yeah, this line assert len(modifiers := stage_modifiers[0].modifiers) == 1
is just unwrapping the stage_modifier
structure, and since we don't have stage_modifier
anymore, it's safe to remove.
SUMMARY:
Removed recipeStage, recipeModifer, stageModifier classes and dependencies.
TEST PLAN:
Tested locally
DETAILS & CONCERNS:
recipe.py
is where most changes happened.parse_from_dict
.Recipe
class now is basically just a list ofModifiers
, all other changes are downstream dependencies.utils
. Had to rewrite serialization logic.self.stage
.test_recipe
. We support creating a recipe instance by reading a multi-stage recipe but no longer support serializing multi-stage recipes directly. The current structure assumes one recipe could only have one stage.update_and_save_recipe
function since now each recipe only has one stage and therefore can't handle merging multi-stage recipes. The new setup:infer_recipe_from_model_path
will find existing recipe path.yaml
method now takes in an optional argumentexisting_recipe_path
and merge the existing recipe with the current recipe. Since there would only be at most one existing recipe at a time, we should be safe.TODO:
recipe.py
structure