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

Please undo your future::plan() when function exits #105

Closed
HenrikBengtsson opened this issue Dec 12, 2023 · 1 comment · Fixed by #106
Closed

Please undo your future::plan() when function exits #105

HenrikBengtsson opened this issue Dec 12, 2023 · 1 comment · Fixed by #106

Comments

@HenrikBengtsson
Copy link

Congrats to the CRAN release. Author of Futureverse here. I've got two comments, one smaller and one important one:

In

future::plan(future::multisession, workers = max(1, parallelly::availableCores() - 1))

instead of max(1, parallelly::availableCores() - 1), you can use parallelly::availableCores(omit = 1), which is mentioned on https://parallelly.futureverse.org/reference/availableCores.html.

Whenever you find yourself setting the future plan inside a function, make sure that your changes are undone when the function exits. This is very important, because the user might have set the future plan they prefer, and will become rather confused if their settings all of a sudden are changed. If your function is called by another function or package, this might be quite tricky for the user to track down. Also, since you're using all cores (but one)(*), after your function has been called, anything that rely on Futureverse will max out like that. To fix this, please see Section 'For package developers' in help("plan", package = "future").

(*) Which is a no-no on CRAN (), but also elsewhere, e.g. https://www.jottr.org/2022/12/05/avoid-detectcores/
(
) Yes, availableCores() will return 2 when R CMD check is running, so you're saved by it here.

@holgstr
Copy link
Contributor

holgstr commented Dec 27, 2023

Hi @HenrikBengtsson, thank you so much for taking a look and your considerate feedback. We will implement your comments as advised.

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 a pull request may close this issue.

2 participants