Check env for py_require()d packages#1850
Conversation
|
Thanks! Can you please add a test and NEWS entry? |
|
We should probably add an escape hatch to let users disable the warning by setting an envvar. |
|
my initial idea was disabling with |
3e0f0d7 to
c1ea8b9
Compare
a0a32b4 to
813deb8
Compare
d2161ef to
e5695d6
Compare
|
Thanks, Daniel! I fixed the CI failures and tweaked the approach slightly. Like all things involving Python environment management, this change has a high potential for unintended behavior. I’ll merge it right after the next CRAN release to let it run on the dev version of reticulate and surface any issues. I also noted two TODOs:
|
|
|
||
| if (would_install) { | ||
| # subset the py_require()$packages vector to keep only those that | ||
| # match output from `uv pip install`. |
There was a problem hiding this comment.
Isn't the output from uv already just the subset of packages that are required and are missing in the env?
There was a problem hiding this comment.
uv outputs frozen versions docutils==1.2.3. I think we should display the actual requirement in py_require(), which is likely unfrozen docutils, or with a looser constraint like docutils[extra]>=1.2.3
R/package.R
Outdated
| "--color never --no-progress", | ||
| "--no-build", | ||
| # "--verbose", | ||
| "--offline", "--no-config", |
There was a problem hiding this comment.
I don't think --offline is a good idea here. This will only warn if the package is already on the cache. Otherwise it's a resolution error, and status != 0.
If we keep --offline, we will need to change below.
There was a problem hiding this comment.
When I gave it a quick try locally I didn't run into issues, but thats probably because i already had all the packages in my local cache. I'm fine switching it back. We should have a unit test for this to make sure we don't accidentally trigger a download of a package!
There was a problem hiding this comment.
How would you test that?
There was a problem hiding this comment.
Maybe an approach like this pseudo-code?
output <- r_session({
py_require("tensorflow")
use_virtualenv("some/tmp/venv")
py_config()
})
expect_false(grepl("downloading tensorflow", output))There was a problem hiding this comment.
Yeah, although we will need a escape hatch to return the actual uv output. With the changes mentioned in #1850 (comment) we no longer forward this output to the user.
Warns when py_require()d packages are not installed in the resolved virtual environment.