Repository navigation
Conversation
Summary: Creating a BisectLogger with a reused timestamp and object ID can retrieve a previous instance's logger from Python's global logging registry. Its closed handlers then reopen an old log file, mixing output between sessions or raising FileNotFoundError after the old temporary directory has been removed. This caused three setUp errors in GitHub Actions Tests #927: https://github.com/meta-pytorch/tritonparse/actions/runs/35882624122/job/107254643368 Give each BisectLogger its own logging.Logger and detach and close test handlers before removing temporary directories. Add a deterministic regression that forces object ID reuse and covers both retained and deleted previous log directories. Differential Revision: D121418371
meta-cla
Bot
added
the
CLA Signed
|
@FindHao has exported this pull request. If you are a Meta employee, you can view the originating Diff in D121418371. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Initialize the isolated logger’s manager before any info() calls.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR isolates BisectLogger instances and improves handler cleanup to prevent stale log reuse.
Changes:
- Creates per-instance loggers.
- Detaches handlers before temporary-directory cleanup.
- Adds regression coverage for reused IDs and deleted log directories.
| File | Summary |
|---|---|
tritonparse/bisect/logger.py |
Creates isolated logger instances; construction currently fails because the logger manager is unset. |
tests/cpu/test_bisect_results.py |
Cleans up handlers before temporary directories. |
tests/cpu/test_bisect_logger.py |
Adds ID-reuse regression coverage and cleanup helpers. |
tests/cpu/test_bisect_executor.py |
Improves handler cleanup ordering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.logger = logging.Logger(self._logger_name) | ||
| self.logger.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Directly constructed loggers inherit manager from the Logger class. The logging module initializes Logger.manager = Manager(Logger.root) at module scope (CPython 3.14 source). This class attribute is available even when the instance is not registered in manager.loggerDict, so isEnabledFor() can access self.manager.disable without an explicit manager assignment.
I verified the exported code at e94a395a68dcac1b782468c4f96a8e391db851b6 on Python 3.14.7: constructing BisectLogger(...) and calling info() both succeed, the message is written to the log file, and logger.manager is logging.root.manager is True. The reported AttributeError is therefore a false positive; an explicit manager assignment would just duplicate the inherited value.
|
This pull request has been merged in 77e0ba4. |
Summary:
Creating a BisectLogger with a reused timestamp and object ID can retrieve a previous instance's logger from Python's global logging registry. Its closed handlers then reopen an old log file, mixing output between sessions or raising FileNotFoundError after the old temporary directory has been removed. This caused three setUp errors in GitHub Actions Tests #927: https://github.com/meta-pytorch/tritonparse/actions/runs/35882624122/job/107254643368
Give each BisectLogger its own logging.Logger and detach and close test handlers before removing temporary directories. Add a deterministic regression that forces object ID reuse and covers both retained and deleted previous log directories.
Differential Revision: D121418371