Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements proper cleanup for the memory monitoring system to prevent resource leaks and callback accumulation. The training function now tracks the memory monitor's lifecycle and ensures proper shutdown when training completes or fails.
- Extended memory monitor API to report start/stop success and expose callback removal functionality
- Modified
train_comprehensiveto properly manage monitor lifecycle and clean up registered callbacks - Added comprehensive regression test to verify no callback duplication and proper thread cleanup
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_train_memory_monitor.py | New test file with regression test for memory monitor cleanup |
| azchess/utils/memory_monitor.py | Added return values to start/stop methods and callback removal API |
| azchess/utils/init.py | Exported new remove_memory_alert_callback function |
| azchess/training/train.py | Added monitor lifecycle tracking and cleanup in train_comprehensive |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| while False: | ||
| yield None |
There was a problem hiding this comment.
This generator will never yield any values since the while condition is False. This should be while True: or use a different approach to create an empty generator.
| while False: | |
| yield None | |
| if False: | |
| yield |
| """Stop the memory monitoring thread.""" | ||
| if not self.is_monitoring: | ||
| return | ||
| if not self.is_monitoring and not (self.monitor_thread and self.monitor_thread.is_alive()): |
There was a problem hiding this comment.
[nitpick] This condition check is confusing. Consider simplifying to check if there's nothing to stop: if not self.is_monitoring and (not self.monitor_thread or not self.monitor_thread.is_alive()):
| if not self.is_monitoring and not (self.monitor_thread and self.monitor_thread.is_alive()): | |
| if not self.is_monitoring and (not self.monitor_thread or not self.monitor_thread.is_alive()): |
Summary
train_comprehensiveto track monitor lifecycle, unregister its alert callback, and stop monitoring during cleanupTesting
https://chatgpt.com/codex/tasks/task_e_68d1c2a371b88323861a41c2fc7d9bc8