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

Use a task queue to ensure code blocks are executed sequentially #510

Merged
merged 10 commits into from
Sep 9, 2024

Conversation

DavisVaughan
Copy link
Collaborator

Closes #509

Joint work with @lionel-

This ensures that all of the blocks are sent over in a single sequential batch, with no way for anyone else to execute their own block between two of ours, which would cause ordering issues

This ensures that all of the blocks are sent over in a single sequential batch, with no way for anyone else to execute their own block between two of ours, which would cause ordering issues
@DavisVaughan
Copy link
Collaborator Author

DavisVaughan commented Aug 6, 2024

While I have not tested it, I believe this problem also exists for the Jupyter executor, as it also awaits in a loop, giving someone else the opportunity to inject a block between two executions - so you may want to file an issue for that if you agree

execute: async (blocks: string[]) => {
// if there is a cell magic then we need to execute cell-by-cell
const hasMagic = blocks.find((block) => !!block.match(/^\s*%%\w+\s/));
if (hasMagic) {
for (const block of blocks) {
await commands.executeCommand(
"jupyter.execSelectionInteractive",
block
);
}
} else {
const code = blocks.join("\n");
await commands.executeCommand("jupyter.execSelectionInteractive", code);
}
},
});

@lionel-
Copy link
Collaborator

lionel- commented Aug 7, 2024

Since this is tricky to get right, it seems like vsc-quarto should be in charge of executing the commands so that it can make sure these are run "atomically", i.e. all commands are run before yielding back to the event loop. To this end there could be a new API where language extensions provide an executeCode() callback to execute a block that vsc-quarto would then call in a loop structured similarly to the one we implemented here.

@DavisVaughan DavisVaughan changed the title Only await on the last code block Use a task queue to ensure code blocks are executed sequentially Sep 5, 2024
@cscheid
Copy link
Contributor

cscheid commented Sep 5, 2024

(just letting you know that this change looks a lot safer now, from what I can tell!)

@DavisVaughan
Copy link
Collaborator Author

DavisVaughan commented Sep 5, 2024

@cscheid that's great to hear! Yea I think this is a much saner approach. I've also realized that this is a problem in more than Positron #509 (comment), so I've devised TaskQueueManager in such a way that we can use it for all of the other languages' execute() hooks too. Each language will get its own individual task queue.

Copy link
Collaborator

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

I like this new setup!

I think we could use an existing async queue? In the positron-r extension we use PQueue.

apps/vscode/src/host/manager.ts Outdated Show resolved Hide resolved
apps/vscode/src/host/manager.ts Outdated Show resolved Hide resolved
@DavisVaughan
Copy link
Collaborator Author

@lionel- it seems like PQueue was actually exactly what we needed. I took a look at their implementation and it seems extremely similar to my queue.ts approach, I basically reimplemented it without knowing. Can you take one more look please?

@DavisVaughan DavisVaughan merged commit f15c1f9 into quarto-dev:main Sep 9, 2024
1 check passed
@DavisVaughan DavisVaughan deleted the fix/execute-blocks branch September 9, 2024 16:44
DavisVaughan added a commit to DavisVaughan/quarto that referenced this pull request Sep 13, 2024
DavisVaughan added a commit that referenced this pull request Sep 13, 2024
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.

execute() hook for Positron can run blocks out of order
3 participants