-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix all the things by fixing multiprocessing #733
Conversation
A shameless self-promoter here: #411. It is almost like this PR without the UI changes. I've tested with python 3.8 and 3.9 on both macOS 10.15 Catalina and WSL. |
@stephenbach LGTM! I have tested on MacOs Catalina with Python 3.7, 3.8 and 3.9, and everything looks good. NB: I had to change L184 to Maybe keep the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested using "datasets==1.18.4"
without #735.
(With #735, "datasets==2.0.0
" works, see https://github.com/tianjianjiang/bigscience-promptsource/runs/5802249762?check_suite_focus=true#step:4:26)
It works with "python>=3.7,<3.10"
for macOS BigSur 11.6.5 on Intel, too.
I will also test Monterey and re-test Catalina soon, but they probably won't cause any issue.
For "python>=3.10,<4"
, see #733 (comment).
Co-authored-by: 姜 天戩 Mike Tian-Jian Jiang <[email protected]>
Mutliprocessing in the helicopter view is the source of a lot of inconsistent bugs across OSs and Python versions. This PR fixes promptsource when using the spawn mode (default in python 3.8+ and required to run smoothly on latest OS X). It also changes it to the use spawn always so that hopefully there is more consistent behavior.
Before merging, we should test on a range of OSs and Python versions.
The main changes are actually pretty small, but the diff is huge because most of app.py had to be wrapped in a function. (This is so that this code is not executed in each subprocess, which was our problem when spawning.) The main changes besides wrapping the code is
all_infos
toget_infos
as an argument