-
Notifications
You must be signed in to change notification settings - Fork 146
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
[ModuleSparsificationInfo] Add logabble_items
method
#1468
[ModuleSparsificationInfo] Add logabble_items
method
#1468
Conversation
@@ -58,5 +59,13 @@ def from_module(cls, module: torch.nn.Module) -> "ModuleSparsificationInfo": | |||
quantization_info=SparsificationQuantization.from_module(module), | |||
) | |||
|
|||
def loggable_items(self) -> Iterable[Tuple[str, float]]: | |||
raise NotImplementedError() | |||
def loggable_items(self) -> Generator[Tuple[str, Any], None, 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.
Thoughts on making it an __iter__
, such that we can do:
info = ModuleSparsificationInfo.from_module(...)
for tag, item in info:
....
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.
let's not do that for now since the logged items here should directly match with the logging targets from our PRD and not actually be representative of what's contained in this class
Also note: while working on that feature in noticed a bug in |
logabble_items
methodlogabble_items
method
@@ -58,5 +59,13 @@ def from_module(cls, module: torch.nn.Module) -> "ModuleSparsificationInfo": | |||
quantization_info=SparsificationQuantization.from_module(module), | |||
) | |||
|
|||
def loggable_items(self) -> Iterable[Tuple[str, float]]: | |||
raise NotImplementedError() | |||
def loggable_items(self) -> Generator[Tuple[str, Any], None, 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.
let's not do that for now since the logged items here should directly match with the logging targets from our PRD and not actually be representative of what's contained in this class
Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com>
Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com>
…into feature/damian/loggable_items
…ication' into feature/damian/loggable_items
…tion_info' into feature/damian/loggable_items
@@ -108,6 +108,22 @@ def from_module( | |||
operation_counts=Counter([op.__class__.__name__ for op in operations]), | |||
) | |||
|
|||
def loggable_items( |
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.
Thoughts on making an abstract class that all these inherit from? That's the only other thing I can think of for this for ensuring the object you get has loggable_items (or you could just call hasattr)
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, sounds like a good idea.
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, nice use of generators
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 pending comment
|
||
quantization_scheme = self.quantization_scheme[operation] | ||
if quantization_scheme is None: | ||
yield f"{main_tag}/{operation}/precision", 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.
Why None instead of 32/16 based on the param?
* initial_commit * [ModuleSparsificationInfo] Proposal of the main logic (#1454) * initial commit * initial developement * sync with ben * prototype ready * included ben's comments * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * address comments * fix quantization logic * remove identities from leaf operations * fix the Identity removal * [ModuleSparsificationInfo][Tests] Proposal of the main logic (#1479) * Update src/sparseml/pytorch/utils/sparsification_info/helpers.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * addressing PR comments --------- Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * [ModuleSparsificationInfo] Add `logabble_items` method (#1468) * initial commit * initial developement * sync with ben * prototype ready * included ben's comments * initial commit * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * address comments * fix quantization logic * formatting standardised to the PRD * remove identities from leaf operations * fix the Identity removal * initial commit * cleanup * correct tests * address PR comments --------- Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * [ModuleSparisificationInfo] LoggingModifier (#1484) * initial commit * initial developement * sync with ben * prototype ready * included ben's comments * initial commit * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * Update src/sparseml/pytorch/utils/sparsification_info/configs.py Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * address comments * fix quantization logic * formatting standardised to the PRD * remove identities from leaf operations * fix the Identity removal * initial commit * initial commit * checkpoint * Delete modifier_logging.py * Apply suggestions from code review * tested the modifier * Apply suggestions from code review --------- Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com> * [ModuleSparsificationInfo][Tests] SparsificationLoggingModifier (#1485) * Apply suggestions from code review * Trigger tests --------- Co-authored-by: Benjamin Fineran <bfineran@users.noreply.github.com>
Feature Preview
Returns logs:
Note: every log has three copies returned because of the bug, fix here: #1483