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

Clear execution metadata, prefer msg header date when recording times #195

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

kevin-bates
Copy link
Member

Hello,
While troubleshooting elyra-ai/elyra#2387 and discussing the issue in deshaw/jupyterlab-execute-time#60 it's probably a good idea to clear the cell metadata execution stanza prior to execution (which is the case in Lab). This is due to the fact that nbclient doesn't post a value for shell.execute_reply.started which results in incorrect duration calculations by the execute-time extension when the notebook was first executed via Lab, then an nbclient application (like papermill). Since the extension will fall back to using iopub.execute_input as its calculation for the cell's execution start time in the absence of shell.execute_reply.started, clearing the stanza is sufficient to produce valid duration results. I suspect this value is not captured because this is not specified in the NB format spec and, I'm guessing, this repo is trying to (correctly) adhere to the spec as much as possible.

The other observation is that Lab seeds this stanza's entries from the date in the corresponding message header rather than the current time, thereby removing network latency from the captured information.

This pull request clears the execution metadata when executing a cell. It also prefers the date-time from the corresponding message header, falling back to the current time if issues are encountered. (I found that the mocking used in the tests results in "messages" that don't contain a header entry. I went ahead and updated the timestamp function to tolerate that condition.)

If there's heartburn over the timestamp/header stuff, I'm okay retracting that, although it seems like the correct thing to do in light of the greater possibility of remotely-located kernels.

Looks like the traceback produced during the interrupt test has changed, causing some test failures. Looking at existing PRs, this appears to be unrelated to this PR.

@davidbrochart
Copy link
Member

Thanks a lot Kevin, that looks good to me.

Looks like the traceback produced during the interrupt test has changed, causing some test failures. Looking at existing PRs, this appears to be unrelated to this PR.

Yes, it is unrelated. I opened ipython/ipykernel#845 but between IPython 8, ipykernel 6.7.0 and jupyter-client 7.1.1, I'm not sure.

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.

2 participants