fix: proper context management of handlers #2322
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2322 +/- ##
==========================================
+ Coverage 89.77% 90.83% +1.05%
==========================================
Files 222 222
Lines 11958 11976 +18
==========================================
+ Hits 10735 10878 +143
+ Misses 1223 1098 -125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
294155f to
e15f54e
Compare
Can you perhaps try to apply the fix without the refactoring first? See if we can isolate it |
I am not having a fix, It is just a feeling that the structure is not robust and may cause it |
|
Fixes #2320 |
@nan-wang can u review again, I made sure it only |
Run on master: on this branch: I am not seeing any big conclusion. |
| self.body = open(self.path, self.mode) | ||
| self.header = open(self.path + '.head', self.mode) |
There was a problem hiding this comment.
Why are we opening the files in __init__? Shouldn't we only open them in __enter__?
There was a problem hiding this comment.
To avoid the concerns from @nan-wang , we avoid opening and closing at everybatch which could damage performance. But we still check the need for opening
There was a problem hiding this comment.
I see. So we're kinda using "half" of what a ctx mger should do (open and close within in the context, right)?
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| self.flush() |
There was a problem hiding this comment.
Why don't we close() on exit?
There was a problem hiding this comment.
Because we wanted to avoid the need to reopen all the time, but at least we assume the data is flushed
There was a problem hiding this comment.
Closing the file after every 'add' will be quite expensive.
There was a problem hiding this comment.
Yes, got it. That's what I tried in the previous approach. And we're kinda doing "half" of the ctx management. Makes sense
Try to have a better and more robust implementation of
BinaryPbIndexerand the access to theWritingandReadHandlers.I believe this can be a cause of #2320