Clear execution metadata, prefer msg header date when recording times #195
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 usingiopub.execute_input
as its calculation for the cell's execution start time in the absence ofshell.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.