Skip to content

Conversation

kylesayrs
Copy link
Collaborator

@kylesayrs kylesayrs commented Apr 27, 2025

Purpose

  • Reduce model support burden by skipping any modules which are not call graph ancestors of the sequential targets
  • Rather than requiring the user to specify a list of ignored modules, only trace what is necessary to disjointly execute sequential targets
    • In the future, the ignore field will be used to skip untraceable function/method names
  • This change does not change functionality because all ignored modules are already non-ancestors of sequential targets

Changes

  • Remove ignore modules requirement (all ignored modules are already non-ancestors of sequential targets)
  • Implement get_sequential_ancestors which returns all ancestors of the sequential targets
  • Modify tracer to skip anything that is not a sequential ancestor or has offloaded modules
    • The two sets rarely overlap, and when they do, the module is skipped for safety and the user is warned

Testing

Follow ups

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.

Signed-off-by: Kyle Sayers <[email protected]>
Signed-off-by: Kyle Sayers <[email protected]>
@kylesayrs kylesayrs added the ready When a PR is ready for review label Apr 27, 2025
Signed-off-by: Kyle Sayers <[email protected]>
Copy link
Collaborator

@brian-dellabetta brian-dellabetta left a comment

Choose a reason for hiding this comment

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

cool!

Copy link
Collaborator

@shanjiaz shanjiaz left a comment

Choose a reason for hiding this comment

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

DFS logic looks good to me! Should we update this guide now that we automatically handles ignore?

@kylesayrs
Copy link
Collaborator Author

@shanjiaz I'm waiting to do that after #1390 lands, since at that point, ignore will become tracing_ignore and will ignore function names, not modules

Copy link
Collaborator

@shanjiaz shanjiaz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for answering my tracer questions

@kylesayrs kylesayrs enabled auto-merge (squash) April 29, 2025 15:30
@kylesayrs kylesayrs merged commit 56047ac into main Apr 29, 2025
8 checks passed
@kylesayrs kylesayrs deleted the kylesayrs/skip-non-ancestors branch April 29, 2025 16:05
dsikka added a commit that referenced this pull request Apr 30, 2025
## Purpose ##
* When #1389 landed, modules being skipped by ignore were no longer
being skipped. However, this requires that the sequential targets list
be correct. Mllama defaults to targeting vision layers, and hence the
vision tower was being traced, leading to errors.

```python3
_no_split_modules = [
    "MllamaVisionEncoderLayer",
    "MllamaCrossAttentionDecoderLayer",
    "MllamaSelfAttentionDecoderLayer",
]
```

## Changes ##
* Only target text decoder layers, not vision decoder layers

## Testing ##
* #1335 passes

Signed-off-by: Kyle Sayers <[email protected]>
Co-authored-by: Dipika Sikka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants