-
Notifications
You must be signed in to change notification settings - Fork 66
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
allow user to configure opt_handle_signals #333
allow user to configure opt_handle_signals #333
Conversation
Codecov Report
@@ Coverage Diff @@
## main #333 +/- ##
==========================================
- Coverage 41.50% 41.50% -0.01%
==========================================
Files 76 76
Lines 4652 4653 +1
==========================================
Hits 1931 1931
- Misses 2721 2722 +1
|
Thank you. Could you also update the FAQ.md please with better advice for multithreading? |
6385f91
to
e9b7d18
Compare
@cjdoris sure thing, just pushed and referenced the discussion thread. |
@@ -129,7 +129,7 @@ def args_from_config(): | |||
CONFIG['opt_sysimage'] = sysimg = path_option('sysimage', check_exists=True)[0] | |||
CONFIG['opt_threads'] = int_option('threads', accept_auto=True)[0] | |||
CONFIG['opt_warn_overwrite'] = choice('warn_overwrite', ['yes', 'no'])[0] | |||
CONFIG['opt_handle_signals'] = 'no' | |||
CONFIG['opt_handle_signals'] = choice('handle_signals', ['yes', 'no'], default='no')[0] |
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.
Can we default to yes
? Defaulting to a behaviour which only causes crashes for backward compatibility doesn't seem very useful.
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.
Can we default to
yes
? Defaulting to a behaviour which only causes crashes for backward compatibility doesn't seem very useful.
I agree, that's probably better to default to yes
, but I left off for now as it is a behavioral change. Just posted a comment on this thread too, let's see what the author thinks
Thanks! Please open a separate issue/PR to change the default - AFAIR making it yes by default has undesirable side-effects on Python. |
Hi @cjdoris , this PR is to allow a user to configure what is passed to
julia --handle-signals=<yes,no>
. I have just updated it to be in line with all the other CONFIG params, defaulting tono
to retain backwards compatibility.Following discussion thread starting here, I have found that this is much more robust and clean way to allow for multi-threaded julia code than my previous workaround.
Related to #219
Related to #298
Related to #330