-
Notifications
You must be signed in to change notification settings - Fork 289
address darwin system parallel threading issues, without numpydoc fixes #708
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
base: main
Are you sure you want to change the base?
Conversation
|
@jph00 Thoughts? |
|
Thanks for this. Can you run I don't actually know what's happening in this PR. Can you tell me about the problem and solution? |
|
@jph00 can do, apologies, that I forgot to do so |
|
@jph00 I think this is an issue with Specifically the cell: test_eq(urljson('https://httpbin.org/get')['headers']['User-Agent'], url_default_headers['User-Agent'])Re-running this multiple times on both main and on this branch I've found non-deterministic behavior. |
|
@jph00 thoughts? Or how else should I address this? |
jph00
left a comment
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'm not sure about these changes, but certainly interested in discussing them. Would love to hear your more detailed thoughts.
| @wraps(f) | ||
| def _f(*args, **kwargs): | ||
| res = (Thread,Process)[process](target=g, args=args, kwargs=kwargs) | ||
| if process: |
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 don't think we want this, do we? I'd expect process=True to give us a normal Process. Why would we want something different on Mac?
| if threadpool: pool = ThreadPoolExecutor | ||
| else: | ||
| if not method and sys.platform == 'darwin': method='fork' | ||
| if not method and sys.platform == 'darwin': |
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.
export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES is a good workaround in practice, in our experience. How about we check for that first?
| def run_procs(f, f_done, args): | ||
| "Call `f` for each item in `args` in parallel, yielding `f_done`" | ||
| processes = L(args).map(Process, args=arg0, target=f) | ||
| Proc = get_context('fork').Process if sys.platform == 'darwin' else Process |
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.
fork feels a bit unexpected to me here too.
I re-ran the tests - sorry about that! |
No description provided.