Skip to content
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

take default threads from CRAN env var #5807

Closed
wants to merge 3 commits into from

Conversation

tdhock
Copy link
Member

@tdhock tdhock commented Dec 7, 2023

Closes #5658
(already closed but actually still an issue)

DT currently takes default number of threads from env vars OMP_THREAD_LIMIT or OMP_NUM_THREADS, as defined in src/openmp-utils.c

CRAN does not allow using more than two CPUs during checks. Previously it had set OMP_THREAD_LIMIT=2 see #5658 (comment)

Recently CRAN stopped setting OMP_THREAD_LIMIT, but it does set _R_CHECK_EXAMPLE_TIMING_CPU_TO_ELAPSED_THRESHOLD_=2.5 which I propose in this PR to examine, round down, and use as a default for the DT number of threads.

Currently packages which use DT in tests/examples/vignettes need to run setDTthreads(2) to get CRAN checks to pass. After merging this PR, the default would satisfy CRAN (package which use DT would no longer need to run setDTthreads in tests/examples/vignettes).

@tdhock tdhock mentioned this pull request Dec 7, 2023
@jangorecki
Copy link
Member

jangorecki commented Dec 7, 2023

Currently packages which use DT in tests/examples/vignettes need to run setDTthreads(2) to get CRAN checks to pass. After merging this PR, the default would satisfy CRAN (package which use DT would no longer need to run setDTthreads in tests/examples/vignettes).

According to reports in our GH this is not guaranteed. They ended up setting threads to 1 or putting examples into dontrun.

I don't like proposed change because it is rather unexpected (when possibly not solving the CRAN problem). One may think that setting that particular env var will not impact his/her data pipelines, but only R CMD check. A DT user may expect their R command to not be affected by settings that are meant to affect R CMD check commands only.

@tanho63
Copy link

tanho63 commented Dec 22, 2023

image

fwiw one of my pkgs got this email from cran today after implementing similar. interested to see how it turns out if you do end up giving it a go

@jangorecki
Copy link
Member

Thanks for letting us know. I think we will get sample reply. Those, and any others vars, should not be used to detect if pkg check runs on CRAN or not, so it is rather not surprising.
Having your example I think we can abandon this draft PR.

@tdhock
Copy link
Member Author

tdhock commented Dec 23, 2023

thanks for the info, that is helpful.

@tdhock tdhock closed this Dec 23, 2023
@jangorecki jangorecki deleted the default-threads-from-CRAN-env-var branch December 23, 2023 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Best practice for using max 2 cores on CRAN?
3 participants