Conversation
|
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
karawoo
left a comment
karawoo
left a comment
There was a problem hiding this comment.
The code looks good to me and I was able to generate several correct manifests with it. A couple of minor questions below.
As far as I understand we don't expect this to actually work on Connect just yet, and indeed I was not able to deploy my content (quarto project using jupyter engine) to Connect. But the manifest itself looked fine, so I don't think it's an issue with anything here.
| The parse is pure: it reads only renv.lock and never invokes R or inspects | ||
| locally installed R packages. This mirrors how Posit Publisher resolves R | ||
| dependencies for Python content that also uses R (e.g. rpy2 apps). |
There was a problem hiding this comment.
This means that lockfiles outside of the project root will not be respected, correct? Lockfiles can legitimately be in other locations, either through setting RENV_PATHS_LOCKFILE or if the user is using renv profiles.
In the rsconnect R package this is supported (rstudio/rsconnect#1296). If we are choosing not to support it here, or not for now (which I think is a reasonable choice) then could we document that?
There was a problem hiding this comment.
I did mimic the code in publisher, but I realised now that publisher does invoke the R interpreter thus it would probably handle the profiles and the RENV_PATHS_LOCKFILE by virtue of that.
Invoking the R interpreter didn't seem a good idea in rsconnect-python because I don't want to force the need to install an R environment just to deploy a python project.
As a trade-off I support for RENV_PATHS_LOCKFILE as it was easy enough to support and documented the behavior. I think that in the majority of rpy2 projects it will be perfectly fine to support the renv.lock exclusively in the project directory as it will be shipped with the project itself, but RENV_PATHS_LOCKFILE is a solid escape hatch for exceptions
Correct, that is undergoing work in connect side |
| # "a directory" so renv.lock is appended. A relative override resolves against | ||
| # the current working directory (matching renv), not the project directory. | ||
| # With no override we fall back to renv's default of <project>/renv.lock. | ||
| override = os.environ.get("RENV_PATHS_LOCKFILE") |
There was a problem hiding this comment.
This comment isn't correct about renv's behavior as of the most recent renv version: it resolves against the project directory, not the working directory.
https://github.com/rstudio/renv/blob/538c71944aab9b02d78ddaab626e70713ffcc1f4/R/paths.R#L66-L70
(introduced in rstudio/renv@f0ab135)
Could we adjust the behavior to match and add a test for relative paths in RENV_PATHS_LOCKFILE?
There was a problem hiding this comment.
02c49ad adjusts RENV_PATHS_LOCKFILE to be relative to the project directory.
|
The failure on Python 3.8 is unrelated, it has to do with the release of mistune that is now incompatible with Python 3.8, the same failure happens on main. |
2 participants
Intent
Closes #784
Type of Change
Approach
Implement a
r_environmentthat detectsrenv.lockand parses the dependencies.Automated Tests
Directions for Reviewers
Checklist
rsconnect-python-tests-at-nightworkflow in Connect against this feature branch.