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

Some R commands are skipped when executing multiple commands after each other #4068

Closed
LanderDC opened this issue Jul 18, 2024 · 8 comments
Closed
Assignees
Labels
area: console Issues related to Console category. bug Something isn't working lang: r support

Comments

@LanderDC
Copy link

Positron Version:

Positron Version: 2024.07.0 (Universal) build 67
Code - OSS Version: 1.91.0
Commit: 58e22e1
Date: 2024-07-18T04:44:17.582Z
Electron: 29.4.0
Chromium: 122.0.6261.156
Node.js: 20.9.0
V8: 12.2.281.27-electron.0
OS: Darwin x64 23.5.0

Steps to reproduce the issue:

Running multiple commands after each other with Cmd + Enter from an Rscript (without waiting for them to finish), skips the execution of some commands, giving errors downstream. It sometimes even seems that multiple commands are running at the same time.

In the screenshot below you can see the order in which I executed the commands (script) and the output of the console showing which commands ran (white) and which didn't (grey).
afbeelding

What did you expect to happen?

I expected a similar behaviour as Rstudio where you can execute commands from an Rscript, while another command is still running, that are put into a "queue" and executed in order.

Were there any error messages in the Output panel or Developer Tools console?

No

@LanderDC LanderDC added the bug Something isn't working label Jul 18, 2024
@DavisVaughan
Copy link
Contributor

Can you please paste the code that you ran in as text so it is easy for us to try?

@LanderDC
Copy link
Author

The screenshot was from a script with my own data, but here is the same code with an example dataset, which also behaves funky:

library(tidyverse)
library(phyloseq)
library(decontam)
data(GlobalPatterns)

sample_data(GlobalPatterns)

#' ### Remove contamination
#' #### Visualize library sizes of samples and negative controls
decontam <- as.data.frame(sample_data(GlobalPatterns))
decontam
decontam$LibrarySize <- sample_sums(GlobalPatterns)
decontam <- decontam[order(decontam$LibrarySize),]
decontam$Index <- seq(nrow(decontam))
ggplot(data=decontam, aes(x=Index, y=LibrarySize, color=SampleType)) + 
  geom_point()+
  ggtitle("Library sizes")

ggplot(data=decontam, aes(y="libraries", x=LibrarySize, color=SampleType)) + 
  geom_jitter(height = .01)+
  scale_x_log10()+
  ggtitle("Library sizes")

#' #### Detect contaminants
sample_data(GlobalPatterns)$is.neg <- sample_data(GlobalPatterns)$SampleType == "Mock"
sample_data(GlobalPatterns)$is.neg
contamdf.prev <- decontam::isContaminant(GlobalPatterns, method="prevalence", neg="is.neg")


#' **Number of contaminating contigs:**
table(contamdf.prev$contaminant)

contaminants <- rownames(contamdf.prev[contamdf.prev$contaminant==T,])

FYI, first selecting all code that needs to be run and then Cmd+Enter does not give any issues.

@DavisVaughan
Copy link
Contributor

Okay I was able to reproduce. It does look like if you hit CMD + Enter really really fast and there is some longer running code involved (like loading these large packages) then it is possible for a line to get dropped. That's not great!

library(tidyverse)
library(phyloseq)
library(decontam)

x <- 1
y <- x
z <- y
a <- z

Note that I am mashing CMD + Enter as fast as I can to get this to occur

Screen.Recording.2024-07-18.at.10.40.44.AM.mov

@LanderDC
Copy link
Author

That's exactly what I'm seeing. I would say that if there is code that runs longer than a few seconds, a "normal" speed of Cmd + Enter is enough to trigger this behaviour.

@juliasilge
Copy link
Contributor

I believe this is also coming up for folks when they do something like "Run Above" in a Quarto doc: #4104

@juliasilge juliasilge added the area: console Issues related to Console category. label Jul 22, 2024
@juliasilge juliasilge added this to the 2024.09.0 Pre-Release milestone Jul 22, 2024
@juliasilge
Copy link
Contributor

Another example with "Run above" in a Quarto document: #4231

@DavisVaughan
Copy link
Contributor

I've got a pretty good idea of what the problem is here, but I could use some advice on the best way to solve this kind of thing.

I think the crux of the issue is this await right here, which allows execution to jump out of this function and can allow for out of order execution:

const codeFragmentStatus = await this.session.isCodeFragmentComplete(codeFragment);

Imagine that we have a this._runtimeItemPendingInput containing this code, and lets call it chunk A

library(tidyverse)
library(phyloseq)
library(decontam)

x <- 1
  • In processPendingInput() for chunk A, we set this._runtimeItemPendingInput = undefined to declare we are handling it
  • In processPendingInput() for chunk A, we call await this.session.isCodeFragmentComplete(codeFragment); to determine if a fragment of chunk A is complete
  • This await allows execution to switch out of this function, in particular addPendingInput() is allowed to run and:
  • In addPendingInput(), it sees this._runtimeItemPendingInput is undefined and creates a new pending input item, with, say, chunk B, and sets this._runtimeItemPendingInput
y <- x
z <- y
  • In processPendingInput() for chunk B, we do the same dance as above but with chunk B. Set this._runtimeItemPendingInput = undefined and call await this.session.isCodeFragmentComplete(codeFragment); but with chunk B fragments. This switches us back to chunk A control flow.
  • In processPendingInput() for chunk A, we find that library(tidyverse) is a complete statement and plan to execute it, but first we set this._runtimeItemPendingInput to the remaining lines in that chunk. Then we execute library(tidyverse).
  • Then we switch back to processPendingInput() for chunk B, where it finds y <- x as a complete statement and tries to evaluate that, but BOOM that is an out of order code execution.

private addPendingInput(code: string) {
// If there is a pending input runtime item, remove it.
if (this._runtimeItemPendingInput) {
// Get the index of the pending input runtime item.
const index = this.runtimeItems.indexOf(this._runtimeItemPendingInput);
// This index should always be > -1, but be defensive. Remove the pending input runtime
// item.
if (index > -1) {
this._runtimeItems.splice(index, 1);
}
// Set the code.
code = this._runtimeItemPendingInput.code + '\n' + code;
}
// Create the pending input runtime item.
this._runtimeItemPendingInput = new RuntimeItemPendingInput(
generateUuid(),
this._session.dynState.inputPrompt,
code
);
// Add the pending input runtime item.
this.addRuntimeItem(this._runtimeItemPendingInput);

if (i + 1 < pendingInputLines.length) {
// Create the pending input runtime item.
this._runtimeItemPendingInput = new RuntimeItemPendingInput(
generateUuid(),
this._session.dynState.inputPrompt,
pendingInputLines.slice(i + 1).join('\n')
);
// Add the pending input runtime item.
this._runtimeItems.push(this._runtimeItemPendingInput);

This theory is supported by some console logs, which added logs right before and after that await, where you can see execution switch away. Look at Checking if fragment is complete from processPendingInput(): library(decontam) and then look how Finished checking if fragment is complete from processPendingInput(): library(decontam) is many lines down the page because of the await.

Screenshot 2024-08-05 at 5 05 31 PM

@DavisVaughan DavisVaughan self-assigned this Aug 6, 2024
DavisVaughan added a commit that referenced this issue Aug 8, 2024
…g it (#4257)

Addresses #4068

Joint work with @lionel- 


#4068 (comment)
is a detailed account of the problem that this PR addresses, so
definitely give that a close read before trying to understand this.

The reproducible example that is now fixed is
#4068 (comment)

The main challenge to deal with is the fact that `await
this.session.isCodeFragmentComplete(codeFragment)` is async and is
called in a loop inside `processPendingInput()`, allowing the "state of
the world", particularly `this._runtimeItemPendingInput`, to change out
from under us while we are finding a complete code fragment.

There are a few key changes in this PR
- We introduce a global `_pendingInputState` to ensure that we cannot
re-enter `processPendingInput()` while we are already processing input
    - If `'Idle'`, we aren't processing any input
- If `'Processing'`, we are processing input and cannot call
`processPendingInput()` again
- If `'Interrupted'`, this is a signal that can happen while we are
`'Processing'` inside `processPendingInput()` that we should bail out
and stop trying to processing any pending input.

- We no longer set `this._runtimeItemPendingInput = undefined;` in
`processPendingInput()` before all of the `await
isCodeFragmentComplete()` calls. This was particularly bad because at
the `await` point we could loop back around into `addPendingInput()` and
create an entirely new `this._runtimeItemPendingInput` because it looked
like one didn't exist at all!

- We pay _very_ careful attention in `processPendingInput()` to ensure
that we use either `this._runtimeItemPendingInput.code` or
`pendingInputLines`, both of which are guaranteed to always reflect the
current state of the world after the `await isCodeFragmentComplete()`
calls. Previously, the local variable version of `pendingInputLines` had
a strong chance to be outdated after the `await`s.

---

This might be a case where it would be useful to write a test that tries
to rapidly send some code to the console. If we can come up with a
failing test on main (even an intermittent failure would be fine), then
we can use that as a test case for this because if that _ever_ fails
after this PR then something is wrong.

This is the absolute simplest case I can come up with where mashing `Cmd
+ Enter` on each line _really really fast_ will sometimes trigger the
issue on `main`, so that is what I'd try and create a test against.

```r
Sys.sleep(.5)
print("hi there")
Sys.sleep(.5)

x <- 1
y <- x
z <- y
a <- z
```
@testlabauto
Copy link
Contributor

Verified Fixed

Positron Version(s) : 2024.08.0-24
OS Version          : OSX

Test scenario(s)

Unable to repro manually after many tries. Was able to repro manually with previous release:

library(tidyverse)
library(arrow)
library(RSQLite)

x <- 1
y <- x
z <- y
a <- z
b <- a
c <- b

Link(s) to TestRail test cases run or created:
Automated test in progress

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: console Issues related to Console category. bug Something isn't working lang: r support
Projects
None yet
Development

No branches or pull requests

4 participants