feat: add disable_limit_max_threads feature and limit the max threads to 2 by default for CRAN compatibility
#720
Conversation
eitsupi
force-pushed
the
auto-limit-threads
branch
from
7d6dc78 to
6f17296
Compare
R/zzz.R
Outdated
| # Auto limit the max number of threads used by polars | ||
| if ( | ||
| !(cargo_rpolars_feature_info()[["disable_auto_limit_max_threads"]] || | ||
| isTRUE(getOption("polars.disable_auto_limit_max_threads"))) && |
There was a problem hiding this comment.
Related to #713, here disable_auto_limit_max_threads must be set via the options function.
Functions in the package polars are not useful here, since the value must be set when Polars is invoked.
|
@etiennebacher Could you take a look at this? |
eitsupi
force-pushed
the
auto-limit-threads
branch
from
4337be1 to
b2ea892
Compare
| @@ -23,5 +24,5 @@ | |||
| - strictly_immutable: input must be TRUE or FALSE. | |||
| - debug_polars: input must be TRUE or FALSE. | |||
|
|
|||
| More info at `?polars_options`. | |||
| More info at `?polars::polars_options`. | |||
There was a problem hiding this comment.
Note: This is not directly related to this PR, but it is necessary to be able to display the help page without loading the package because the package cannot be loaded if the option validation fails.
|
Just fyi I'll try to do another review tomorrow or friday, very busy week |
R/options.R
Outdated
| @@ -179,6 +175,7 @@ polars_options = function() { | |||
| df_knitr_print = getOption("polars.df_knitr_print"), | |||
| do_not_repeat_call = getOption("polars.do_not_repeat_call"), | |||
| int64_conversion = getOption("polars.int64_conversion"), | |||
| limit_max_threads = getOption("polars.limit_max_threads") %||% "Not set (NULL)", | |||
There was a problem hiding this comment.
if the default is to limit the number of threads to 2, shouldn't it be %||% TRUE?
There was a problem hiding this comment.
TRUE doesn't make sense. This is because there is a possibility that the behavior will be changed on the lib side. Only an explicit FALSE is meaningful
There was a problem hiding this comment.
Sorry I don't understand that. Suppose I install the package and I don't change any option. This means that there is an automatic limit to use 2 threads. If I do polars_options(), what would be printed?
limit_max_threads TRUE or limit_max_threads Not set (NULL)?
If it's TRUE, I think it's fine, but if it's Not set (NULL) then that's confusing because I would understand it as "there's no limit in the number of threads", while actually there is one.
There was a problem hiding this comment.
The default is changed to !polars_info()$features$disable_limit_max_threads
eitsupi
changed the title
disable_auto_limit_max_threads featuredisable_limit_max_threads feature and
eitsupi
changed the title
disable_limit_max_threads feature anddisable_limit_max_threads feature and limit the max threads to 2 by default for CRAN compatibility
|
Thanks for your detailed review! |
| @@ -77,10 +81,22 @@ check_feature = function(feature_name, context = NULL, call = sys.call(1L)) { | |||
| #' | |||
| #' The threadpool size can be overridden by setting the | |||
| #' `POLARS_MAX_THREADS` environment variable before process start. | |||
| #' (The thread pool is not behind a lock, so it cannot be modified once set). | |||
| #' It cannot be modified once `polars` is loaded. | |||
Close #241