-
Notifications
You must be signed in to change notification settings - Fork 570
[PyTorch] Avoid initializing recipe state in fusible op base class constructor #2421
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
Conversation
Op attrs may not be set. Move recipe state initialization to linear op constructor. Signed-off-by: Tim Moon <tmoon@nvidia.com>
|
/te-ci pytorch |
ksivaman
left a comment
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.
LGTM
Greptile OverviewGreptile SummaryFixed initialization order bug where recipe state was initialized in Key Changes:
Root Cause: Impact: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant quantized_model_init as quantized_model_init context
participant BasicLinear
participant BasicOperation
participant FP8GlobalStateManager
Note over User,FP8GlobalStateManager: Before this PR (broken)
User->>quantized_model_init: enter context
quantized_model_init->>FP8GlobalStateManager: set with_fp8_parameters=True
User->>BasicLinear: __init__()
BasicLinear->>BasicOperation: super().__init__()
BasicOperation->>FP8GlobalStateManager: with_fp8_parameters()
FP8GlobalStateManager-->>BasicOperation: True
BasicOperation->>BasicOperation: reset_recipe_state(recipe)
Note over BasicOperation: BUG: Called before child attrs set!
BasicOperation-->>BasicLinear: return
BasicLinear->>BasicLinear: set _with_quantized_weight
Note over BasicLinear: Too late! Recipe state already initialized<br/>without access to this attribute
Note over User,FP8GlobalStateManager: After this PR (fixed)
User->>quantized_model_init: enter context
quantized_model_init->>FP8GlobalStateManager: set with_fp8_parameters=True
User->>BasicLinear: __init__()
BasicLinear->>BasicOperation: super().__init__()
Note over BasicOperation: No recipe state initialization
BasicOperation-->>BasicLinear: return
BasicLinear->>BasicLinear: set _with_quantized_weight
BasicLinear->>FP8GlobalStateManager: with_fp8_parameters()
FP8GlobalStateManager-->>BasicLinear: True
BasicLinear->>BasicLinear: reset_recipe_state(recipe)
Note over BasicLinear: FIXED: Recipe state initialized<br/>after all attrs are set
|
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.
3 files reviewed, no comments
yaox12
left a comment
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.
LGTM. Thanks for your quick fix. Verified on my case.
…nstructor (#2421) Do not initialize recipe state in base op class Op attrs may not be set. Move recipe state initialization to linear op constructor. Signed-off-by: Tim Moon <tmoon@nvidia.com>
…nstructor (NVIDIA#2421) Do not initialize recipe state in base op class Op attrs may not be set. Move recipe state initialization to linear op constructor. Signed-off-by: Tim Moon <tmoon@nvidia.com> Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
Description
This fixes a bug when constructing the
Quantizeop within aquantized_params_initcontext.#1951 changed the
BasicOperationclass so that it would initialize the quantization recipe state when constructed within aquantized_params_initcontext. This was needed so that the recipe state is available when creating quantized weights. However, putting this logic within the base class means that we are initializing the recipe state before any child class attrs are set, so the recipe state initialization can't adapt to any user-provided config. This PR fixes this bug by moving the recipe state initialization to theBasicLinearop, which is currently the only op that supports quantized params.Pinging @yaox12.
Type of change
Changes
Checklist: