Skip to content

Python: Immediately start a Python runtime in Python workspaces #1282

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

Closed
seeM opened this issue Sep 8, 2023 · 11 comments
Closed

Python: Immediately start a Python runtime in Python workspaces #1282

seeM opened this issue Sep 8, 2023 · 11 comments
Assignees
Milestone

Comments

@seeM
Copy link
Contributor

seeM commented Sep 8, 2023

Via @petetronic:

If we find we are being opened in a Folder that is predominantly Python - perhaps we could start simple and just say we find .py files, or if there are better heuristics from the MS Python extension already - and just mark the preferred Python interpreter as Immediate

@seeM seeM added this to the Internal Preview milestone Sep 8, 2023
@seeM seeM self-assigned this Sep 8, 2023
@petetronic
Copy link
Collaborator

Related to a request for R in #72

@petetronic
Copy link
Collaborator

Some activation events from the python extension include:

        "workspaceContains:mspythonconfig.json",
        "workspaceContains:pyproject.toml",
        "workspaceContains:Pipfile",
        "workspaceContains:setup.py",
        "workspaceContains:requirements.txt",
        "workspaceContains:manage.py",
        "workspaceContains:app.py"

@jmcphers
Copy link
Collaborator

We may be able to remove this hack once the Python extension accepts responsibility for starting Python when appropriate:

// --- Begin TEMPORARY ---
// We want to start a Python runtime as a last-chance fallback if we have no other
// runtimes to start. We consider this to be true under the following circumstances:
//
// - We have completed the discovery phase, which implies we've also started any
// runtimes that asked to be started right away (startupBehavior === Immediate).
// - No runtimes have been started or are running yet.
// - This was not an initially untrusted workspace; in these workspaces, we don't
// know what would have been automatically started, so we don't want to come
// stomping in and starting a Python runtime.
// - No runtimes will be automatically started for this workspace, i.e. this workspace
// has no affiliated runtimes.
//
// This behavior shouldn't live here, but it does until the Python language pack
// can be updated. See https://github.com/rstudio/positron/issues/1009
if (this._discoveryPhase === LanguageRuntimeDiscoveryPhase.Complete &&
!this.hasAnyStartedOrRunningRuntimes() &&
languageRuntimeInfo.runtime.metadata.languageId === 'python' &&
this._initiallyTrustedWorkspace === true &&
!this._workspaceAffiliation.hasAffiliatedRuntime()) {
this.autoStartRuntime(languageRuntimeInfo.runtime,
`No other language runtimes want to start; Python is the default.`);
}
// --- End TEMPORARY ---

@seeM
Copy link
Contributor Author

seeM commented Sep 11, 2023

[...] once the Python extension accepts responsibility for starting Python when appropriate

@jmcphers I need to do some more testing to see whether activating eagerly actually brings Python into the discovery phase, I'd like to track that in #1009.

Until then, I think a good step for this issue for alpha would be:

  1. Update R to only enable immediate startup if the workspace has R files, or if it's empty and the user has RStudio installed i.e. hasRFiles || (isEmpty && isRStudioUser).
  2. Update Python to only enable immediate startup if the workspace contains Python files (including the non .py files Pete mentioned).

That way, R won't be started at all in folders that only contain Python files, and vice versa, until we figure out #1009. I've got branches of positron and positron-python running with this setup and I think it's a big improvement over the current behavior.

What do you think?


I also foresee a separate/related issue by looking at the code, but I haven't tested yet:

I'm not sure how Positron will currently react to multiple languages registering a runtime with immediate startup behavior (EDIT: all within the discovery phase). IIUC it would start whichever language happened to start register a runtime first. Is that a bug or intended?

const languageRuntimeInfos = this._registeredRuntimes.filter(
info => info.startupBehavior === LanguageRuntimeStartupBehavior.Immediate);
if (languageRuntimeInfos.length) {
this.autoStartRuntime(languageRuntimeInfos[0].runtime,
`An extension requested the runtime to be started immediately.`);
}
}

seeM added a commit that referenced this issue 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.
@juliasilge
Copy link
Contributor

I think hasRFiles() should return true with only R Markdown or Quarto files as well.

@seeM
Copy link
Contributor Author

seeM commented Sep 13, 2023

This is ready for review in 2023.09.0-139. It's worth testing in both R and Python projects. Note that this should only affect opening new workspaces

seeM added a commit that referenced this issue Sep 13, 2023
seeM added a commit that referenced this issue Sep 13, 2023
No longer needed since we've addressed #1282.
seeM added a commit that referenced this issue Sep 13, 2023
No longer needed since we've addressed #1282.
@juliasilge
Copy link
Contributor

I just spent some time testing this out with build 139. I have used RStudio as my IDE for... everything 😆 for a while now so it was a pretty good way to explore it.

  • My R package projects all still start R ✅
    • (Side note, this includes yardstick which actually runs Python code, but from a R file!)
  • I have used RStudio as my IDE for Python projects (like Python packages) and these start Python even though they have .Rproj files and friends, since they also have Python files ✅
  • My projects that are talks or websites (no .R or .py files) open R, since I used RStudio with them. In my case, none of these were Python projects, but I expect if they were, R would have started. Certainly I am unusual in using RStudio as my IDE for Python projects, though.

@seeM
Copy link
Contributor Author

seeM commented Sep 14, 2023

Thank you @juliasilge!

Re your 3rd point: Positron doesn't currently support starting more than one runtime with immediate startup behavior. Atm it will only start the first "immediate" runtime registered, which is most often R (since it registers much faster than Python). I think we'd need to file that as a separate feature request/bug if it's something we think is important. Perhaps we want to start at most one runtime per language instead of globally, as it currently is.

Given that, I'm surprised that Python started even though there was a .Rproj file! Since .Rproj could mean either R or Python, maybe we should remove it from the R startup heuristic?

@juliasilge
Copy link
Contributor

If we don't add .Rproj as one of the triggers to start R, then it won't start in projects for RStudio users that are all .qmd files. (Some of those other files like .Rprofile are not as commonly used, and things like .Rbuildignore are only for R packages, which have .R files anyway.) My guess is that the situation of an RStudio user having no signals but .Rproj and wanting to use R much be way more common than people who use RStudio as their IDE for Python projects.

@seeM
Copy link
Contributor Author

seeM commented Sep 18, 2023

@juliasilge great, I agree. In that case, are you happy with us closing this as "done" or is there still anything outstanding?

@juliasilge
Copy link
Contributor

Yes! From my perspective, looks great. 👍

wesm pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants