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

update R startup heuristic #1289

Merged
merged 5 commits into from
Sep 12, 2023
Merged

update R startup heuristic #1289

merged 5 commits into from
Sep 12, 2023

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Sep 11, 2023

Only start an R runtime immediately if the workspace contains R files, or if it's empty but the user has installed RStudio. This is less likely to incorrectly start R in Python projects for users that happen to have RStudio installed.

See #1282 (comment) for more detail.

Only start an R runtime immediately if the workspace contains R files,
or if it's empty but the user has installed RStudio. This is less likely
to incorrectly start R in Python projects for users that happen to have
RStudio installed.

See
#1282 (comment)
for more detail.
@seeM seeM requested a review from jmcphers September 11, 2023 16:28
@juliasilge
Copy link
Contributor

This won't start R in a workspace that has Quarto or R Markdown files, which is a pretty common situation for R users (no .R files at all, but Quarto or R Markdown files for analysis and reporting).

@seeM
Copy link
Contributor Author

seeM commented Sep 11, 2023

Good point! Will fix that

@jjallaire
Copy link
Contributor

Rmd files would definitively indicate R. qmd files could of course be either. You can use quarto inspect to determine what "engines" are in play for a project, however I think that would be too slow for this application (~.5 to 1.5 seconds). You could regex scan the qmds looking for R cells as a failsafe?

@seeM
Copy link
Contributor Author

seeM commented Sep 12, 2023

Here's an updated list of globs from poking around R projects on GitHub:

['*.R', '*.Rmd', '.Rprofile', 'renv.lock', '.Rbuildignore', '*.Rproj']

I'm wondering if the .qmd heuristic is still necessary given the above. How likely is it that an R workspace would not match any of the above, but still have .qmd files with R code blocks?

@jjallaire
Copy link
Contributor

For existing R projects I think *.Rproj would indeed cover any project that had been used in RStudio (which might be good enough). An R project full of .qmd that doesn't have an .Rproj would likely have been created in Positron (increasingly likely as time goes on). So I think for now it would be fine to just key on those extensions but then later I think we may need a bit more introspection (alternatively, for ambiguous cases of .qmd and no other hints we could just use the "default" runtime for a user based on their previous behavior).

@seeM seeM requested a review from juliasilge September 12, 2023 15:56
@seeM
Copy link
Contributor Author

seeM commented Sep 12, 2023

I'm going to leave out the qmd heuristic for now, although it's in a725cef (which I've since reverted) if we want it

This is ready for review

Co-authored-by: Julia Silge <julia.silge@gmail.com>
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

'**/renv.lock',
'**/.Rbuildignore',
'**/.Renviron',
'**/*.Rproj'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance, I think almost all of these should only be searched for at the workspace root since that's the only location at which we expect to find them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added **s in all cases to cater for users who open a monorepo at the repo root. My assumption re performance was that, in the worst case, we would still have to iterate through every file in the workspace to check the **/*.R pattern, so it shouldn't be a significant performance drop to add more ** patterns on top of that. Similarly, in the best case, we would match one of the ** patterns early, and the rest wouldn't matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen a somewhat common pattern where such repositories will have separate sub-directories for different language bindings; e.g. you have a folder for R bindings, Python bindings, and so on. In this scenario, because we cannot really ascertain what the "right" default runtime is, I would argue it would be better to search at the workspace root.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that in that case we would start both an R and Python runtime. But if e.g you had multiple sub-directories with R projects using different R versions, I suppose we wouldn't know which specific R version to start, so I agree that we shouldn't set "immediate" startup behavior unless we're very certain. I'll make a follow-up PR.

@petetronic petetronic merged commit 9509a49 into main Sep 12, 2023
@seeM seeM deleted the update-r-startup-heuristic branch September 13, 2023 08:29
wesm pushed a commit that referenced this pull request Mar 28, 2024
…startup behavior

Merge pull request #212 from posit-dev/immediate-startup

heuristically set immediate startup behavior
--------------------
Commit message for posit-dev/positron-python@a2f48a5:

add conda files to startup heuristic

--------------------
Commit message for posit-dev/positron-python@b788426:

Update src/client/positron/provider.ts

Co-authored-by: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com>
--------------------
Commit message for posit-dev/positron-python@0a7f647:

heuristically set immediate startup behavior

Set startup behavior to immediate if the workspace contains files
relevant to Python development.

Addresses #1282.

Note that this branch won't work as expected until
#1289 is merged.


Authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com>
wesm pushed a commit that referenced this pull request Mar 28, 2024
…ristic

Merge pull request #214 from posit-dev/immediate-startup

refine immediate startup heuristic
--------------------
Commit message for posit-dev/positron-python@2a5a988:

refine immediate startup heuristic

Restrict config files to the workspace root. See this discussion for
more: #1289 (comment).

Also fixes the `.venv` and `.conda` cases which were incorrectly
searching for files with those names instead of folders.

Relates to #1282.


Authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com>
wesm pushed a commit that referenced this pull request Mar 28, 2024
…startup behavior

Merge pull request #212 from posit-dev/immediate-startup

heuristically set immediate startup behavior
--------------------
Commit message for posit-dev/positron-python@a2f48a5:

add conda files to startup heuristic

--------------------
Commit message for posit-dev/positron-python@b788426:

Update src/client/positron/provider.ts

Co-authored-by: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com>
--------------------
Commit message for posit-dev/positron-python@0a7f647:

heuristically set immediate startup behavior

Set startup behavior to immediate if the workspace contains files
relevant to Python development.

Addresses #1282.

Note that this branch won't work as expected until
#1289 is merged.


Authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com>
wesm pushed a commit that referenced this pull request Mar 28, 2024
…ristic

Merge pull request #214 from posit-dev/immediate-startup

refine immediate startup heuristic
--------------------
Commit message for posit-dev/positron-python@2a5a988:

refine immediate startup heuristic

Restrict config files to the workspace root. See this discussion for
more: #1289 (comment).

Also fixes the `.venv` and `.conda` cases which were incorrectly
searching for files with those names instead of folders.

Relates to #1282.


Authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com>
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.

6 participants