Repository navigation
Fixes to io.ascii fast reader when used in multiprocessing 'spawn' mode - #8853
Conversation
| fill_exclude_names=None, | ||
| fill_extra_cols=0, | ||
| fast_reader=True): | ||
| fast_reader=None): |
There was a problem hiding this comment.
The default True would have never worked since we pop it below!
|
Does this mean we don't need #8852 after all? Wow, great investigation! |
Codecov Report
@@ Coverage Diff @@
## master #8853 +/- ##
==========================================
+ Coverage 87% 87.02% +0.02%
==========================================
Files 401 402 +1
Lines 59496 59625 +129
Branches 1100 1100
==========================================
+ Hits 51763 51889 +126
- Misses 7092 7095 +3
Partials 641 641
Continue to review full report at Codecov.
|
|
I've now identified the bug causing the memory issue, so this PR is now complete. I'll close #8852 since that workaround won't be needed. |
…because source_bytes did not have a persistent Python reference
…nce both get treated as bytes anyway when pickled via __reduce__
2110f52 to
4d4bcc2
Compare
|
By the way, it looks like as a result of how multiprocessing works in 'spawn' mode, the contents of the table are actually pickled, which means we lose the memory mapping. This is not something I've changed here, just what the behavior would have been, as implemented, if it hadn't been buggy. I've opened a new issue to remind us to check the performance in the 'spawn' mode: #8858 |
| # Since we already deallocated the tokenizer above, we should make sure we | ||
| # set self.tokenizer to NULL to avoid deallocating the tokenizer again | ||
| # inside __dealloc__. | ||
| self.tokenizer = NULL |
There was a problem hiding this comment.
This may be needed in the two except clauses above too ?
|
Nice work! I think the changes makes sense to me, but this code is quite horrible so it's not easy to be sure :D. |
|
@saimn - indeed it looks like things work properly on Windows now so I've removed the restrictions. Let's see if it works on AppVeyor too (edit: it does!). For the fast_reader bug in the CParser init, I don't think we really need a regression test because that class is never actually called directly by users, so it's never actually been called with no |
|
@astrofrog - this looks great, esp. the new support for windows! Just to be clear, this does not change the start method in any way, it just makes things work with the default start method on all platforms. The issue #8858 should definitely be followed up to see if there is a big hit. Although it is not clear if we can actually do anything about it. The multiprocessing docs say "set_start_method() should not be used more than once in the program.". At face value that implies the io.ascii fast reader would not be free to temporarily set to fork for a call to the fast reader on MacOS. It is certainly annoying that Apple has not addressed problems and Mac is basically getting the crippled multiprocessing support that Windows has. |
|
@taldcroft - yes this doesn't change the start method, it just makes it work with the default on all platforms. In principle the Python docs do point out that you can temporarily set a context where you use a specific start method (see what I had done in #8852 as a workaround) but I think it'd be better to just make things work with the default, as done here. |
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
I have not reviewed the detailed cparser changes, but if tests pass then I'm satisfied. All the surrounding changes look good to me. Thanks for taking the dive to fix this long-standing limitation on Windows!
|
@saimn , do you want to add your seal of approval before merge? Tests passed and there's one approval already. But extra approvals never hurt. |
saimn
left a comment
saimn
left a comment
There was a problem hiding this comment.
Looks good, and great to have this enabled on Windows !
Fixes to io.ascii fast reader when used in multiprocessing 'spawn' mode
This is a partial fix for #8851 but it might be worth already getting these changes in master, as I believe they are correct and will be needed whatever the full fix is. Basically it looks like the call to CParser in
__reduce__used an old API. With these changes, the example shown in #8851 works but there are still memory deallocation warnings and issues with performance - but I think those could be dealt with in a separate PR.Unfortunately I don't really know what the best way is to test this automatically since the multiprocessing starting mode has to be set globally. This will effectively get tested implicitly once we start to have Python 3.8 MacOS X builds.