feat: Initial support for agnostic Event tracking across devices#29
feat: Initial support for agnostic Event tracking across devices#29
Event tracking across devices#29Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for get_default_event functionsequenceDiagram
participant User
participant get_default_event
participant torch.cuda
participant torch.xpu
participant torch.mps
participant torch.cpu
User->>get_default_event: device_type, enable_timing, blocking, interprocess
get_default_event->>get_default_event: device_type = device_type or ezpz.get_torch_device_type()
alt device_type == 'cuda' and torch.cuda.is_available()
get_default_event->>torch.cuda: Event(enable_timing, blocking, interprocess)
torch.cuda-->>get_default_event: Event
else device_type == 'xpu' and torch.xpu.is_available()
get_default_event->>torch.xpu: Event(enable_timing)
torch.xpu-->>get_default_event: Event
else device_type == 'mps'
get_default_event->>torch.mps: Event(enable_timing)
torch.mps-->>get_default_event: Event
else
get_default_event->>torch.cpu: Event()
torch.cpu-->>get_default_event: Event
end
get_default_event-->>User: Event
Class diagram for the new Event classclassDiagram
class Event {
- enable_timing: bool = False
- blocking: bool = False
- interprocess: bool = False
- device_type: str
- rank: int
- local_rank: int
- device: torch.device
- event: Event
+ __post_init__()
+ __new__(enable_timing: bool = False)
}
note for Event "This class encapsulates device-specific event handling."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @saforem2 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a factory pattern instead of a long if/else chain in
get_default_event. - The
Eventclass's__new__method seems to have a typo (event.__new__) and doesn't align with the intended behavior; it should likely be removed or corrected.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.device = torch.device(f'{self.device_type}:{self.local_rank}') | ||
| self.event = get_default_event() | ||
|
|
||
| def __new__(cls, enable_timing: bool = False): |
There was a problem hiding this comment.
issue (bug_risk): Potential misreference of 'new' in the Event class.
The new method calls 'event.new(cls, enable_timing=enable_timing)', but 'event' is not defined as a class name; it should likely be 'Event.new(cls, enable_timing=enable_timing)' or use 'super().new(cls)' to ensure proper object instantiation.
| self.rank = ezpz.get_rank() | ||
| self.local_rank = ezpz.get_local_rank() | ||
| self.device = torch.device(f'{self.device_type}:{self.local_rank}') | ||
| self.event = get_default_event() |
There was a problem hiding this comment.
suggestion (bug_risk): Inconsistent passing of initialization parameters for the event.
The new method accepts an enable_timing parameter, but post_init calls get_default_event() without passing any parameters. This disconnect means that any intended configuration provided during construction is not propagated to the underlying event, potentially leading to unexpected behavior.
| self.event = get_default_event() | |
| self.event = get_default_event(enable_timing=self.enable_timing) |
| @dataclass | ||
| class Event: | ||
| """ | ||
| Event class | ||
| """ | ||
| enable_timing: bool = False | ||
| blocking: bool = False | ||
| interprocess: bool = False | ||
|
|
||
| def __post_init__(self): |
There was a problem hiding this comment.
suggestion: Overriding new in a dataclass may conflict with post-initialization.
Dataclasses typically rely on init and post_init for initialization. Overriding new here—especially to inject extra parameters—is unconventional and can lead to subtle issues in instance creation. Consider consolidating initialization logic within post_init to ensure consistent and clear behavior.
Suggested implementation:
Removing the new method ensures that the dataclass uses the default instance creation process with init and post_init, avoiding potential issues with initialization.
| self.event = get_default_event() | ||
|
|
||
| def __new__(cls, enable_timing: bool = False): | ||
| return event.__new__(cls, enable_timing=enable_timing) |
There was a problem hiding this comment.
The new method incorrectly references 'event.new' (in the following line) even though 'event' is undefined. Consider using super().new(cls) or referring to Event.new(cls) instead.
| return event.__new__(cls, enable_timing=enable_timing) | |
| return super().__new__(cls) |
| self.event = get_default_event() | ||
|
|
There was a problem hiding this comment.
[nitpick] The post_init method calls get_default_event() without passing the instance’s event configuration (enable_timing, blocking, interprocess), which may lead to inconsistent behavior. Consider forwarding these parameters to ensure the event is created with the intended settings.
| self.event = get_default_event() | |
| self.event = get_default_event( | |
| enable_timing=self.enable_timing, | |
| blocking=self.blocking, | |
| interprocess=self.interprocess | |
| ) |
Copilot Summary
This pull request introduces a new feature to the
ezpzlibrary by adding theget_default_eventfunction and theEventclass. These additions enhance the library's capability to handle default events for different device types.New feature additions:
src/ezpz/__init__.py: Imported theget_default_eventfunction from theezpz.eventmodule and added it to the list of exported functions. [1] [2]src/ezpz/event.py: Added theget_default_eventfunction, which determines the appropriate event type based on the device type and availability. Introduced theEventclass, which initializes with default event settings and device-specific configurations.Summary by Sourcery
New Features:
Eventclass andget_default_eventfunction to handle default events for different device types (cuda, xpu, mps, cpu).