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

CRAN guardrails #1459

Merged
merged 3 commits into from Aug 25, 2023
Merged

CRAN guardrails #1459

merged 3 commits into from Aug 25, 2023

Conversation

t-kalinowski
Copy link
Member

@t-kalinowski t-kalinowski commented Aug 23, 2023

Closes #1441

  • Improved error message when reticulate attempts to load a broken venv like this. At the least, it should include the path to the python. We'll need to wrap the system2() call in python_config_impl() with a tryCatch() to augment the error message.
  • A guardrail that deters people from creating or finding venvs on CRAN. When reticulate detects its running on CRAN, we should set WORKON_HOME to a private location, perhaps tempfile().
  • In initialize_python(), issue a warning if we're on a CRAN machine, and then in a subsequent release, turn it into an error. Packages should not be initializing Python on CRAN. (See reticulate:::check_forbidden_initialization() for a start on this)
  • Update guidance in vignettes about how to avoid initializing Python on CRAN (guard examples and tests with if(py_available()) {...}, and/or if(py_module_available('foo'))
  • Reticulate attempting to load a broken python installation should be a warning, not an error. Reticulate should emit a warning and move on the the next python in the 'Order of Discovery'.
  • Sprinkle a few more reticulate:::check_forbidden_install() calls across the venv installation management functions.

@t-kalinowski
Copy link
Member Author

Revisiting this, I don't see a reliable way to detect if we're running on a CRAN machine. The two tasks that would modify reticulate behavior on CRAN test machines I will leave unimplemented.

Additionally, the guidance of using py_module_available() is already discussed here: https://rstudio.github.io/reticulate/articles/package.html#checking-and-testing-on-cran

While py_module_available() can cause reticulate to initialize python, I'm now thinking this is an acceptable approach, even on CRAN machines. If the need for stringent guarding against python initialization reappears, we can revisit and maybe export reticulate:::python_module_available(), which probes the python installation via a system2() call rather than attempting to import in the current session. But I'm inclined to not expand/clutter the exported package API at this moment, for the (hopefully one-off) scenario where a forbidden broken python installation is present on a CRAN testing machine.

@t-kalinowski t-kalinowski marked this pull request as ready for review August 25, 2023 17:57
@t-kalinowski t-kalinowski merged commit 72268cd into main Aug 25, 2023
12 checks passed
@t-kalinowski t-kalinowski deleted the cran-guardrails branch August 25, 2023 18:18
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.

Reticulate can initialize Python on CRAN
1 participant