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

[Fix] Update Cell execution result and align client and server state #5561

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shibbas
Copy link
Contributor

@shibbas shibbas commented Jun 5, 2021

Checklist

  • I have read the Contributor Guide
  • I have updated the changelogs/current_changelog.md file with some information about the change that I am making the appropriate file.
  • I have validated or unit-tested the changes that I have made.
  • I have run through the TEST_PLAN.md to ensure that my change does not break anything else.

Context

Related Issue - #5549

This PR addresses a few things:

  • BREAKING: Uses the jupyter messaging protocol defaults for execute_request.stop_on_error.
  • BREAKING: Removes the EXECUTE_CANCEL_ALL and instead uses the kernel returned state in execute_reply status to determine whether to cancel a cell or not.
  • Adds the execute_reply status to the transient metadata via a new action that can be used by applications to determine the success / failure of the cell or whether the cell was aborted.

Proposal

It looks like client and kernel states on the cell execution get out of alignment around cell errors. Jupyter defaults stop_on_error for execute_request to true which stops execution on the server for the queued up cells. However, the default in nteract/messaging is set to false, so even though on error the client cells are no longer shown as executing, the execution is till happening on the kernel causing the client and kernel to get out of sync.

Before

Due to the error in cell 3, execution on cells 4 and 5 is stopped, but the execution is still happening on the kernel. So when cell 5 is executed, the printed value is something that was never technically executed by the client. (Expected: 1, got 2)
cellstate-before

After

Server sends "aborted" for cells 4, 5 which cancels the cell execution and re-running the cell 5 shows the expected output keeping client and kernel in sync.
cellstate-after

@vercel
Copy link

vercel bot commented Jun 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nteract/site/Hp8zdfYHJGovWJX56xSjmprDdex2
✅ Preview: https://site-git-fork-shibbas-cellexecutionreplaystatus-nteract.vercel.app

@shibbas shibbas marked this pull request as ready for review June 5, 2021 02:46
@shibbas
Copy link
Contributor Author

shibbas commented Jun 10, 2021

Putting this back into draft to think through using errors message type vs. execute_reply.status for error.

@shibbas shibbas marked this pull request as draft June 10, 2021 18:48
@captainsafia
Copy link
Member

Heads up that automatic releases are now enabled on the repo (see #5568 for more info). To support automated releases, please make sure that the commits in this PR adhere to the conventional commit format (see https://www.conventionalcommits.org/en/v1.0.0/). Once this PR is merged, a release will be triggered for the modified packages based on the commit message.

@MSeal
Copy link
Member

MSeal commented Jun 29, 2021

@shibbas any input needed to help get the last issue you mentioned putting it back into draft status?

@willingc willingc added the workflow: wip draft work label Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow: wip draft work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants