Track reasons for R discovery and rejection and do stuff with them #5648

jennybc · 2024-12-06T20:35:18Z

Addresses #1397
Addresses #5220
Continued preparation for #5223

A lot of the changes are very internal, but there are also user-facing improvements:

Even more effort to track the reasons (plural) why we've discovered an R installation and, perhaps, the reason an R installation can't be used with Positron. The richer information appears in the Positron R Extension output channel.
If we discover at least one R installations, but there is no usable R installation, we put a pop-up warning. The exact details of this will be good to discuss in the next R meeting.

QA Notes

To experience this, you need to have at least 1 unusable R installation around and 0 usable ones.

For rig-installed R, use rig rm release (or similar) to remove an R installation. Repeat as necessary, until all usable R is gone!

Older R installers are here for macOS arm64: https://cran.rstudio.com/bin/macosx/big-sur-arm64/base/. This makes it easy to get R 4.1, for example, which is below our minimum supported version.

You should see a modal telling you that all R installations are unusable, allowing you to look at the Positron R Extension output channel or dismiss.

jennybc · 2024-12-06T20:55:07Z

extensions/positron-r/src/provider.ts

-	}
+	// Find the current R binary(ies) for various definitions of "current".
+	// The first item here will eventually be marked as The Current R Binary.
+	const currentBinaries = await currentRBinaryCandidates();


There are (at least) 3 possible ways to define the current version of R. currentRBinaryCandidates() returns them all, because for discovery we want them all, and there's some chance they differ.

The code around the current version of R needs to be factored a certain way because it's used here, but also used by $validateMetadata in the RRuntimeManager, via a higher-level function currentRBinary(), which only returns a single installation.

Positron consults these sources, in this order, to define the current version of R:

Windows *nix

Registry ✅ ❌

PATH, via which ✅ ✅

Target of [Cc]urrent symlink in HQ ❌ ✅

jennybc · 2024-12-06T20:56:17Z

extensions/positron-r/src/provider.ts


-	// other conventional places we might find an R binary (or a symlink to one)
-	const possibleBinaries = [
+	const moreBinaries = discoverAdHocBinaries([


I'm increasingly skeptical that we need to check these places and would be open to dropping them. I think this is something we basically inherited from ?RStudio?

Sadly I do not have anything substantive to offer on this.

jennybc · 2024-12-06T20:57:54Z

extensions/positron-r/src/provider.ts

 }

 /**
- * Discovers R language runtimes for Positron; implements
- * positron.LanguageRuntimeDiscoverer.
+ * Discovers R language runtimes for Positron; implements positron.LanguageRuntimeDiscoverer.


The change in rRuntimeDiscover() is a mix of actual change and just changing the order of operations. I'll comment on the interesting bits.

jennybc · 2024-12-06T21:01:06Z

extensions/positron-r/src/provider.ts

-	// * version < minimum R version supported by positron-r
-	// * (macOS only) version is not orthogonal and is not the current version of R
-	// * invalid R installation
+	const rejectedRInstallations: Array<RInstallation> = [];


Starting to keep the rejected R installations around, at least for a little while. This lays the groundwork for possibly incorporating this info into a section of a picker (see #5223) that could be hidden/unhidden.

extensions/positron-r/src/provider.ts

jennybc · 2024-12-06T21:16:15Z

extensions/positron-r/src/r-installation.ts

+export enum ReasonDiscovered {
+	affiliated,
+	registry,
+	/* eslint-disable @typescript-eslint/naming-convention */
+	PATH,
+	HQ,
+	/* eslint-enable @typescript-eslint/naming-convention */
+	adHoc,
+	user
+}
+
+/**
+ * Enum represents why we rejected an R binary.
+ */
+export enum ReasonRejected {
+	invalid = 'invalid',
+	unsupported = 'unsupported',
+	nonOrthogonal = 'non-orthogonal'
+}


While expanding the treatment of "reasons", I also did what @jmcphers requested here re: enum handling: https://github.com/posit-dev/positron/pull/4878/files#r1785400981

jennybc · 2024-12-06T21:24:34Z

extensions/positron-r/src/provider.ts

-	user = 'user-specified directory'
+export interface RBinary {
+	path: string;
+	reasons: ReasonDiscovered[]


Keeping an array of reasons for discovery now. For #5223, it should be possible for certain R installations to appear in more than 1 section of the picker, especially in a future with increased R support for environments, in the renv or conda sense.

extensions/positron-r/src/r-installation.ts

jennybc · 2024-12-06T21:32:17Z

extensions/positron-r/src/r-installation.ts

+	toJSON() {
+		return {
+			...this,
+			reasonDiscovered: this.reasonDiscovered?.map(friendlyReason) ?? null,
+			reasonRejected: this.reasonRejected ? friendlyReason(this.reasonRejected) : null
+		};


This makes human-readable reasons appear in the log. Here's an example of an unusable R installation:

2024-12-06 12:13:40.397 [info] R installation discovered: { "usable": false, "supported": true, "reasonDiscovered": [ "Found in the primary location for R versions on this operating system" ], "reasonRejected": "Non-orthogonal installation that is also not the current version", "binpath": "/Library/Frameworks/R.framework/Versions/4.2-arm64/Resources/bin/R", "homepath": "/Library/Frameworks/R.framework/Resources", "semVersion": { "options": {}, "loose": false, "includePrerelease": false, "raw": "4.4.2", "major": 4, "minor": 4, "patch": 2, "prerelease": [], "build": [], "version": "4.4.2" }, "version": "4.4.2", "arch": "arm64", "current": false, "orthogonal": false }

jennybc · 2024-12-06T21:33:37Z

extensions/positron-r/src/util.ts

@@ -42,8 +43,13 @@ export function timeout(ms: number, reason: string) {
 }

 export function readLines(pth: string): Array<string> {
-	const bigString = fs.readFileSync(pth, 'utf8');
-	return bigString.split(/\r?\n/);
+	try {


In this journey, I went through a state where I mistakenly read from a non-existent file and discovered we needed better error handling here.

Motivated by making sure this works in Workbench, for example

…son it has

jennybc · 2024-12-09T22:37:28Z

extensions/positron-r/src/provider.ts

+			const showLog = await positron.window.showSimpleModalDialogPrompt(
+				vscode.l10n.t('All discovered R installations are unusable by Positron.'),
+				vscode.l10n.t('Learn more about R discovery at <br><a href="https://positron.posit.co/r-installations.html">https://positron.posit.co/r-installations.html</a>'),
+				vscode.l10n.t('View logs'),
+				vscode.l10n.t('Dismiss')
+			);


How this currently looks:

I will open a separate issue about improving positron.window.showSimpleModelDialogPrompt(), as per a related slack discussion. In the meantime, I'm making do.

jennybc · 2024-12-09T22:47:07Z

extensions/positron-r/src/r-installation.ts

+export enum ReasonDiscovered {
+	affiliated = "affiliated",
+	registry = "registry",
+	/* eslint-disable @typescript-eslint/naming-convention */
+	PATH = "PATH",
+	HQ = "HQ",
+	/* eslint-enable @typescript-eslint/naming-convention */
+	adHoc = "adHoc",
+	user = "user"
+}
+
+/**
+ * Enum represents why we rejected an R binary.
+ */
+export enum ReasonRejected {
+	invalid = "invalid",
+	unsupported = "unsupported",
+	nonOrthogonal = "nonOrthogonal"
+}


These really do need to be string enums, so that, when reason arrives in friendlyReason() below, we can use its value to determine which enum we're dealing with. This seems to be a fundamental awkwardness with enums in TypeScript (?), https://stackoverflow.com/q/63753808.

juliasilge

I destroyed my R situation 😆 and this is working great!

I see this in the logs:

[info] R installation discovered: {
  "usable": false,
  "supported": false,
  "reasonDiscovered": [
    "Found in PATH, via the `which` command",
    "Found in the primary location for R versions on this operating system",
    "Found in a conventional location for symlinked R binaries"
  ],
  "reasonRejected": "Unsupported version, i.e. version is less than 4.2.0",
  "binpath": "/Library/Frameworks/R.framework/Versions/4.1-arm64/Resources/bin/R",
  "homepath": "/Library/Frameworks/R.framework/Versions/4.1-arm64/Resources",
  "semVersion": {
    "options": {},
    "loose": false,
    "includePrerelease": false,
    "raw": "4.1.3",
    "major": 4,
    "minor": 1,
    "patch": 3,
    "prerelease": [],
    "build": [],
    "version": "4.1.3"
  },
  "version": "4.1.3",
  "arch": "arm64",
  "current": true,
  "orthogonal": true
}

extensions/positron-r/src/provider.ts

juliasilge · 2024-12-10T00:07:51Z

extensions/positron-r/src/provider.ts


-	// other conventional places we might find an R binary (or a symlink to one)
-	const possibleBinaries = [
+	const moreBinaries = discoverAdHocBinaries([


Sadly I do not have anything substantive to offer on this.

Co-authored-by: Julia Silge <[email protected]> Signed-off-by: Jennifer (Jenny) Bryan <[email protected]>

jennybc · 2024-12-10T22:49:24Z

Also passes the basic smell test on Windows for me. Here's a log and pop-up in the case of only having R 4.1.3:

2024-12-10 22:46:24.870 [info] R installation discovered: {
  "usable": false,
  "supported": false,
  "reasonDiscovered": [
    "Runtime previously affiliated with this workspace",
    "Found in Windows registry",
    "Found in the primary location for R versions on this operating system"
  ],
  "reasonRejected": "Unsupported version, i.e. version is less than 4.2.0",
  "binpath": "C:\\Program Files\\R\\R-4.1.3\\bin\\x64\\R.exe",
  "homepath": "C:\\Program Files\\R\\R-4.1.3",
  "semVersion": {
    "options": {},
    "loose": false,
    "includePrerelease": false,
    "raw": "4.1.3",
    "major": 4,
    "minor": 1,
    "patch": 3,
    "prerelease": [],
    "build": [],
    "version": "4.1.3"
  },
  "version": "4.1.3",
  "arch": "x86_64",
  "current": true,
  "orthogonal": true
}
2024-12-10 22:46:24.870 [info] Filtering out C:\Program Files\R\R-4.1.3\bin\x64\R.exe, reason: Unsupported version, i.e. version is less than 4.2.0.
2024-12-10 22:46:24.870 [warning] All discovered R installations are unusable by Positron.
2024-12-10 22:46:24.870 [warning] Learn more about R discovery at https://positron.posit.co/r-installations

jennybc force-pushed the reasons-for-r branch from 893aa40 to c8ed35e Compare December 6, 2024 21:21

jennybc commented Dec 6, 2024

View reviewed changes

jennybc marked this pull request as ready for review December 7, 2024 00:17

jennybc and others added 4 commits December 9, 2024 11:40

Guard against a non-existent path

7df3634

Track reasons for discovery and rejection; warn for a special case

48968b0

Only check path and not reasons with relevant integration tests

3c6da74

Use positron-provided facilities for the modal

ab567e7

Motivated by making sure this works in Workbench, for example

jennybc force-pushed the reasons-for-r branch from 196c027 to ab567e7 Compare December 9, 2024 21:46

jennybc added 2 commits December 9, 2024 14:24

Use HTML

dad8666

Use string enums so friendlyReason() can figure out which type of rea…

36af701

…son it has

juliasilge mentioned this pull request Dec 9, 2024

Console UI could be extended to allow rich information to be shown when a runtime has not yet started. #5674

Open

Until/unless the button gets bigger, this needs to be very terse

431bcf8

jennybc commented Dec 9, 2024

View reviewed changes

jennybc requested a review from juliasilge December 9, 2024 22:47

jennybc mentioned this pull request Dec 9, 2024

Revisit Positron's modals #5676

Open

juliasilge previously approved these changes Dec 10, 2024

View reviewed changes

Improve the messaging

ed794ec

Co-authored-by: Julia Silge <[email protected]> Signed-off-by: Jennifer (Jenny) Bryan <[email protected]>

jennybc dismissed juliasilge’s stale review via ed794ec December 10, 2024 18:56

Use a URL that's a bit easier on the eyes

2aab56a

juliasilge approved these changes Dec 10, 2024

View reviewed changes

jennybc merged commit 25b2d2e into main Dec 10, 2024
5 checks passed

jennybc deleted the reasons-for-r branch December 10, 2024 22:49

github-actions bot locked and limited conversation to collaborators Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track reasons for R discovery and rejection and do stuff with them #5648

Track reasons for R discovery and rejection and do stuff with them #5648

jennybc commented Dec 6, 2024 •

edited

Loading

jennybc Dec 6, 2024 •

edited

Loading

jennybc Dec 6, 2024

juliasilge Dec 10, 2024

jennybc Dec 6, 2024

jennybc Dec 6, 2024

jennybc Dec 6, 2024

jennybc Dec 6, 2024

jennybc Dec 6, 2024

jennybc Dec 6, 2024

jennybc Dec 9, 2024

jennybc Dec 9, 2024 •

edited

Loading

juliasilge left a comment

juliasilge Dec 10, 2024

jennybc commented Dec 10, 2024

	Windows	*nix
Registry	✅	❌
PATH, via which	✅	✅
Target of [Cc]urrent symlink in HQ	❌	✅

Track reasons for R discovery and rejection and do stuff with them #5648

Track reasons for R discovery and rejection and do stuff with them #5648

Conversation

jennybc commented Dec 6, 2024 • edited Loading

QA Notes

jennybc Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

juliasilge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc commented Dec 10, 2024

jennybc commented Dec 6, 2024 •

edited

Loading

jennybc Dec 6, 2024 •

edited

Loading

jennybc Dec 9, 2024 •

edited

Loading