-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
make microbatch models skippable #11020
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
@@ -335,7 +335,7 @@ def execute(self, model, manifest): | |||
|
|||
class MicrobatchModelRunner(ModelRunner): | |||
batch_idx: Optional[int] = None | |||
batches: Dict[int, BatchType] = field(default_factory=dict) |
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.
Usage of field(default_factory=...)
was invalid outside of a dataclass
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.
Have we verified that having it set as batches: Dict[int, BatchType] = {}
doesn't make it so two instances of MicrobatchModelRunner
share the same underlying dict?
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 a test. We do indeed need to do something different
>>> class MyClass:
... batches = {}
...
>>> instance1 = MyClass()
>>> instance2 = MyClass()
>>> instance1.batches
{}
>>> instance2.batches
{}
>>> instance1.batches['catch_phrase'] = "I like cats!"
>>> instance1.batches
{'catch_phrase': 'I like cats!'}
>>> instance2.batches
{'catch_phrase': 'I like cats!'}
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.
Good call. Fixed with init override:
>>> class MyClass:
... def __init__(self):
... self.batches = {}
...
>>> instance1 = MyClass()
>>> instance2 = MyClass()
>>> instance1.batches
{}
>>> instance2.batches
{}
>>> instance1.batches['catch_phrase'] = "I like cats!"
>>> instance1.batches
{'catch_phrase': 'I like cats!'}
>>> instance2.batches
{}
>>>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11020 +/- ##
==========================================
- Coverage 89.14% 89.08% -0.07%
==========================================
Files 183 183
Lines 23760 23764 +4
==========================================
- Hits 21182 21171 -11
- Misses 2578 2593 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Resolves #11021
Problem
Solution
field(default_factory=...)
outside a dataclass (MicrobatchModelRunner)Checklist
🎩
(extra '.' in skip result is a general issue, not just for microbatch models)