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

Notebook cell execution model #105847

Closed
jrieken opened this issue Sep 1, 2020 · 4 comments
Closed

Notebook cell execution model #105847

jrieken opened this issue Sep 1, 2020 · 4 comments
Assignees
Labels
api feature-request Request for new features or functionality notebook verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 1, 2020

Somewhat a continuation of #103713

Today, execution results are parts metadata and output and execution itself is done via kernels which come from kernel providers. We should try to model execution results into a single entity, e.g execution status (idle, running, scheduled? etc)

@jrieken jrieken added api notebook under-discussion Issue is under discussion for relevance, priority, approach labels Sep 1, 2020
@jrieken jrieken added this to the September 2020 milestone Sep 1, 2020
@jrieken
Copy link
Member Author

jrieken commented Sep 21, 2020

One thing that we should do is to allow the executeCell-functions to return a promise. The promise should resolve that execution has been successfully started. It would allow to show feedback on the run button in cases in which the kernel needs to be resolved or isn't idle.

Screenshot 2020-09-21 at 11 41 29

@jrieken jrieken modified the milestones: September 2020, October 2020 Sep 28, 2020
@jrieken jrieken modified the milestones: October 2020, November 2020 Oct 23, 2020
@jrieken jrieken removed this from the November 2020 milestone Nov 30, 2020
@roblourens roblourens added this to the February 2021 milestone Feb 11, 2021
@jrieken jrieken modified the milestones: February 2021, March 2021 Feb 19, 2021
@roblourens
Copy link
Member

roblourens commented Mar 4, 2021

Some execution API TODOs that I didn't have time to put into code, including some suggestions from Kai a few weeks ago

  • Rethink naming to be more explicit in the intent. Write all the jsdoc to explain the usage to check whether a normal person can follow the flow. Get someone else on the team to review it. Check which words are used in the explanation and use those in names.
    • Should the execution object be called "execution", maybe it is a "task".
    • "executeCells" could be "executeCellsRequest" because it really is just a request
  • Normalize "run" vs "execution" name
  • createNotebookCellExecution - only a kernel can access this, and only the active kernel can trigger an execution.
    • The kernel is provided some factory somehow, how does this work?
  • Because of [Notebook] Execution timer starts with negative value #117460, maybe we can't use absolute times to drive the timer

@roblourens roblourens added feature-request Request for new features or functionality and removed under-discussion Issue is under discussion for relevance, priority, approach labels Mar 22, 2021
roblourens added a commit that referenced this issue Mar 22, 2021
Implement new execution task API
#106744
Fix #105847
@roblourens
Copy link
Member

roblourens commented Mar 22, 2021

Two things not in the PR

  • Preemptively setting a cell as "pending" as soon as it is clicked, not having to wait for a Task to be created. We discussed this some, but in trying to implement it and explain it in the API jsdoc, I was wondering whether it's really worth the extra complexity. Even if there are a few hundred ms of latency, I don't know that it's worth the effort. Will leave it alone for now until we really think it's an issue.
  • Canceling executions when a cell is deleted. Just because I'm not sure of the right place to implement this. Cancel cell execution task when cell is deleted #119504

@roblourens roblourens added the verification-needed Verification of issue is requested label Mar 23, 2021
@roblourens
Copy link
Member

@jrieken maybe you can call this verified since you implemented it and filed some issues

@jrieken jrieken added the verified Verification succeeded label Mar 24, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality notebook verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants