Use configured R in Positron

[mmarchetti]

Intent

This PR implements an r parameter in the inspection, R package scanning, and publishing APIs, similar to the existing python parameter, that allows the extension to specify which R interpreter to use. The extension detects the active R interpreter in Positron, if available, and passes that in. The default/fallback is the current behavior of running R from PATH.

Fixes #1968

Type of Change

• • Bug Fix
• • New Feature
• • Breaking Change
• • Documentation
• • Refactor
• • Tooling

Automated Tests

Existing tests have been updated.

Directions for Reviewers

Inspect projects, scan for R packages, and deploy R projects from VSCode and Positron. Install multiple versions of R for Positron (I used rig).

• VSCode will run R from PATH.
• Positron will use the selected R interpreter, if any, otherwise R from PATH.
• Publisher debug logs will show "Running command" with the correct R binary.

Checklist

• I have updated CHANGELOG.md to cover notable changes.

[dotNomad]

This did not work for me.

I got setup using rig and have R version 4.3.1 (already installed pre rig setup) and then installed 4.4.1 with rig.

I installed the VSIX built from this branch into Positron and it always used 4.3.1. regardless of the R interpreter I had selected. Perhaps I'm missing something about how I should have this setup but:

• I did not see configurations created with the differing versions of R
• I did not see the R version change in the Posit Publisher output

[sagerb]

Haven't verified, but it looks like Jordan hit a problem there.

The code itself looks good, just a file naming request.

[sagerb]

I reproduced what Jordan had seen.

• I installed rig and added version 4.3.3 to the available R versions. (Positron requires 4.2+)
• I switched to 4.3.3 from 4.4.1 within Positron
• I then installed the Publisher into Positron (newest build), via the VSIX file I built.
• I then opened our sample content directory within Positron
• I then opened app.R...

I see this error:

An error has occurred at entrypointTracker::isDocumentEntrypoint, Msg=Runtime exists on main thread but not extension host: b6f20083579c97eff02cef099c7b7419

I can also encounter the error when I create a new deployment...

Unable to continue with project inspection failure for undefined. An error has occurred at newDeployment, configurations.inspect, Msg=Runtime exists on main thread but not extension host: b6f20083579c97eff02cef099c7b7419

[mmarchetti]

Runtime exists on main thread but not extension host

This can happen if runtimes are still being discovered:

If discovery is in progress, a runtime may exist on the main thread but not
the extension host, so retry a bunch of times. Retrying is more likely to return
faster than waiting for the entire discovery phase to complete since runtimes are
discovered concurrently across languages.

So we may need to hold off availability of some features until we know the R interpreter.

[sagerb]

Summary of what I saw with changes:

• I’m seeing the correct version of R picked up and written into the renv.lock
• I'm see is that the version of R recorded within the renv.lock file, is what is recorded within the config file. (Ex. If I modify my lock file to have R version 9.9.9, that is what I'll get within my new config file.)

In all cases, the output log seems to indicate that we are executing the proper version of R:

for R 4.3.3:

time=2024-08-23T11:03:03.215-07:00 level=INFO msg="Creating renv lockfile" path=/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/renv.lock r=/Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/bin/R
time=2024-08-23T11:03:03.215-07:00 level=DEBUG msg="Running command" cmd=/Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/bin/R args="-s -e renv::snapshot(lockfile=\"/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/renv.lock\")"

for R 4.4.1:

time=2024-08-23T11:05:23.132-07:00 level=INFO msg="Creating renv lockfile" path=/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/renv.lock r=/Library/Frameworks/R.framework/Versions/4.4-x86_64/Resources/bin/R
time=2024-08-23T11:05:23.132-07:00 level=DEBUG msg="Running command" cmd=/Library/Frameworks/R.framework/Versions/4.4-x86_64/Resources/bin/R args="-s -e renv::snapshot(lockfile=\"/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/renv.lock\")"

After chatting with Mike, this appears to be the expected behavior.

You got it right; inspection uses the lockfile version. In a lot of cases they would match, because you'd create the lockfile during development with the active R version.
If you open a project from someone else, and you're not using the same version of R as what's in the lockfile, renv will complain when you try to restore the packages locally. At that point you could change R versions so they match, or update the lockfile.

[sagerb]

The latest code looks good and with it, the functionality is working as we expect it to.

[dotNomad]

Is the expectation that the R version in our Configuration would match the selected R version in Positron if there is no renv.lock file?

If that is the expectation that isn't what I'm seeing with the current build on this PR. Just double checking.

[sagerb]

Agreed with @dotNomad, I've verified what he's reporting.

Inspection seems to be invoked with the correct version, but the config doesn't get that version

time=2024-08-23T14:53:50.447-07:00 level=INFO msg="Possible deployment type" Entrypoint=app.R Type=r-shiny
time=2024-08-23T14:53:50.447-07:00 level=INFO msg="Getting renv lockfile path" r=/usr/local/bin/R
time=2024-08-23T14:53:50.447-07:00 level=DEBUG msg="Running command" cmd=/usr/local/bin/R args="-s -e renv::paths$lockfile()"
time=2024-08-23T14:53:51.396-07:00 level=INFO msg="renv::paths$lockfile returned lockfile path" path=/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/renv.lock
time=2024-08-23T14:53:51.396-07:00 level=INFO msg="Access Log" method=POST url="/api/inspect?dir=shinyapp&entrypoint=app.R" elapsed_ms=949 status=200 req_size=137 resp_size=546 client_addr=127.0.0.1:63594 req.python=/Users/billsager/.pyenv/versions/3.11.3/bin/python req.r=/Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/bin/R

Looks like the correct R version isn't being passed in accurately (it has changed back to the other version) within the PUT configuration API:

time=2024-08-23T14:55:27.472-07:00 level=INFO msg="Access Log" method=PUT url="/api/configurations/configuration-23?dir=shinyapp" elapsed_ms=4 status=200 req_size=503 resp_size=783 client_addr=127.0.0.1:63672 req.entrypoint=app.R req.validate=true req.hasParameters=false req.comments="[ Configuration file generated by Posit Publisher.  Please review and modify as needed. See the documentation for more options:  https://github.com/posit-dev/publisher/blob/main/docs/configuration.md]" req.title=shinyapp16 req.r="map[packageFile:renv.lock packageManager:renv version:4.4.1]" req.files="[app.R renv.lock]" req.$schema=https://cdn.posit.co/publisher/schemas/posit-publishing-schema-v3.json req.type=r-shiny
time=2024-08-23T14:55:27.565-07:00 level=INFO msg="Detecting deployment type and entrypoint..." path=/Users/billsager/dev/publishing-client/test/sample-content/shinyapp/.posit/publish
time=2024-08-23T14:55:27.565-07:00 level=INFO msg="Possible deployment type" Entrypoint="" Type=unknown
time=2024-08-23T14:55:27.566-07:00 level=INFO msg="Access Log" method=POST url="/api/inspect?dir=shinyapp%2F.posit%2Fpublish&entrypoint=configuration-23.toml" elapsed_ms=0 status=200 req_size=137 resp_size=485 client_addr=127.0.0.1:63674 req.python=/Users/billsager/.pyenv/versions/3.11.3/bin/python req.r=/Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/bin/R

Confirmed scenario bug that Jordan found.

[sagerb]

This PR is on hold as we determine what we're going to do on the open questions - updated title.

[sagerb]

Here is the behavior that I'm expecting. @jonkeane can you validate them as reasonable expectations from an R Data Scientist?

When using Positron and selecting a specific R Version from multiple installed:

1. With no renv.lock file, creation of a deployment should create a config that specifies R.version equal to the version selected via Positron, rather than what is on the path.
2. With a renv.lock file, creation of a deployment should create a config that uses the version of R from the renv.lock file (R.Version), even if that version does not exist locally.

When using VSCode or Positron with only one R version (basically R is located on path):

1. With no renv.lock file, creation of a deployment should create a config that specifies R.version equal to the version available on the path.
2. With a renv.lock file, creation of a deployment should create a config that uses the version of R from the renv.lock file (R.Version), even if that version does not exist locally.

Notes:

1. When adding additional R installations, you must restart Positron before it will refresh to include them.
2. As we had hoped, we've removed much of the friction by eliminating our ability to generate a renv.lock file. Any errors or prompts requiring user attention are left to the direct interaction with the user as they execute renv's package methods.
3. There is NO warning that the config file being used to deploy contains the same version as the version specified within the renv.lock file. This would be a nice enhancement, as the user really should be populating local package files for the version they are deploying with, since we are trying to reproduce the local environment closely.

For functionality verification, we'll need to verify

[sagerb]

The latest commits on this branch are ready for review (previous commits were already reviewed), and the branch is ready for re-verification.

Commits:
resolve lint errors
Use the passed in R version for inspection. Also added debug message.

[sagerb]

To support the decisions needed, I have documented an overview of which language interpreters are used when inspecting, package scanning, or deploying content within the current publisher code base.

For R:

• value detected/passed in by extension

  • Positron:
    • Active version of R
  • VS Code:
    • R Extension for Visual Studio Code

• getRExecutable()

  1. If value passed in from extension and it exists, use it.
  2. If R can be found on path and is executable, use it

• Inspect:

  1. If there is a renv.lock file, then get the version from it.
  2. If not, then attempt to get the renv lockfile path from the Rexecutable (from getRExecutable)
  3. If no lockfile, then get version from the Rexecutable (from getRExecutable)
  4. Fail if R is required but R could not be found

• Package Scanning: (No longer done by extension)

• Deploying

  • Build manifest
    1. Read lock file
       2. Execute Rexecutable to get lib paths
       3. Execute Rexecutable to get available packages at all of the lib paths
       4. If Bioconductor dependency:
       1. Execute Rexecutable to get BioconductorRepos
       2. Execute Rexecutable to get available packages at all of the BioconductorRepos
          5. Determine source & respository of packages
          6. For all of the packages:
       3. Read package descriptions:
       4. Get version from description, if version mismatch from lock file, return error
          3. If source missing, return error
          4. Build up a list of manifest packages

For Python:

• value detected/passed in by extension

  • Positron:
    • Active version of Python
  • VS Code:
    • Python Extension (Microsoft)

• getPythonExecutable()

  1. If value passed in from extension and it exists, use it.
  2. If python3 or python can be found on path and is executable, use it

• Inspect

  1. Get version from PythonExecutable (from getPythonExecutable)
  2. Warn if no requirements.txt file (log message only)

• Scan

  1. Scan entrypoint file for imports
  2. Execute PythonExecutable (from getPythonExecutable) to get lib dirs
  3. Determine import names from lib dirs
  4. Record name and possible version of each imported lib

• Deploy: (No use of PythonExecutable)

For Quarto:

• value detected/passed in by extension

  • Positron:
    • Quarto Extension
  • VS Code:
    • Quarto Extension

• getQuartoExecutable (conceptual only - not implemented)

  1. Quarto simply executed from path.

• Inspect

  1. Execute QuartoExecutable to run inspect command

Observations:

1. Ability to specify Connect interpreter versions within configuration records provides explicit execution (vs. magic).
   • Need isolation from local environment versions and ones available on server
2. Initial deployment configuration and immediate deployment will reflect local development environment.
3. Differences occur w/ delayed deployment and re-deployments if local development environment is upgraded or changed.
   • Local upgraded R and Python interpreters used to determine dependencies to be set in bundle manifest
   • versions of dependencies may be incompatible with older versions of R and Python interpreters, and fail execution on server
     • upgrade to local environment should be coordinated with change
       to versions within configuration file and Connect server environments
4. Additional support is needed to use configured R, Python and Quarto versions in Positron and VS Code. ()

[sagerb]

After documenting the version usage, I now feel we should move forward with this PR as it is the first step of multiples which will respect the user's working environment within Positron.

Most of the code was written by @mmarchetti, with some minor bug fixes added with my updates.

@marcosnav Can you please take a look at this PR?

[marcosnav]

@sagerb The bats test issue in this PR is different from the one on the Nightly runs. Seems this one requires a minor update to an expected error message.

[marcosnav]

Just curious, if getPreferredRuntime has it's own retry logic, is it worth having that extra check again here? In which cases could it fail and then succeed?

[sagerb]

Not sure of Mike's intentions. I'll take a look.

[sagerb]

getPreferredRuntime is an API that executes within Positron. They retry to handle queries during the initialization of Positron when the info is not yet available. Our testing showed that our query to them required additional retries to be reliable, so Mike added additional ones.

I'm checking with Mike for further clarification, but currently I don't think any change is required here.

[marcosnav]

Related to pushing on having more tests on the extension code, this file contains a few methods for which it is worth having tests for.

[sagerb]

Will be adding tests.

[sagerb]

I investigated implementing vitests for this utility, using a few different techniques, but all were unsuccessful. Attempts to run the tests failed as they attempted to resolve the vscode imports, in which they could not resolve the type definition into an import.

We also discussed the two techniques I investigated (mocking the extension runtime environment and injection pattern for removing the extension runtime dependencies). We concluded that going this route would be very costly to mock the entire runtime environment of VSCode extension and do it correctly where it would accurately represent the actual runtime environment.

Instead, we quickly looked back at the guidelines provided by Microsoft on how to best test the extensions. (https://code.visualstudio.com/api/working-with-extensions/testing-extension).

We already have the Extension Test Runner setup in our project, with one simple test. It is executed with npm run test. While there are some changes that we'll probably want to make (like switching from mocha to jest), this appears to follow the best practices for testing functionality within the extension's runtime environment.

I propose that we go in this direction and that it is outside of the scope of this PR. I'd recommend we move forward with this PR, merge it, and then implement some simple tests for the detection of the preferred R and Python versions using the extension test runner.

@dotNomad I think this summarizes our discussion?

@marcosnav Let's discuss at our next opportunity, if you have any questions or concerns.

[marcosnav]

Using the @vscode/test-* utilities sound great, I do think we should definitely use that and it would be great to have more test running on it. At the same time, that is one element to have a full circle in a testing strategy, more on the side of integration testing. Having to run a VSCode instance to test a single function feels a bit off.

This is definitely worth a future discussion. I'm ok on making these efforts outside of this PR.

[dotNomad]

Great write up @sagerb thanks for putting that down in a comment.