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

Add execution timing to cells #6864

Merged
merged 6 commits into from Sep 9, 2019
Merged

Add execution timing to cells #6864

merged 6 commits into from Sep 9, 2019

Conversation

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jul 21, 2019

Closes #3320

Moves forward @saulshanabrook's work in #5009

Changes from earlier PR - rebase on master, fix tests and movie 'timing' into its own subdict. There was also a bug which prevented the iopub hook of outputarea from being invoked in case recordTiming was on - here was where it was overridden.

I had a query in #5009 (comment), I chose to not use a nested dict, I can change it if people feel otherwise

Can we also consider making the command to record timings sticky? Through the settings registry.
I believe there is a plan to make this a pattern for all commands? This PR - #5659

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jul 21, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@Madhu94 Madhu94 changed the title Timing [wip]Timing Jul 21, 2019
@Madhu94 Madhu94 changed the title [wip]Timing [wip] Add execution timing to cells Jul 21, 2019
packages/cells/src/widget.ts Outdated Show resolved Hide resolved
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jul 22, 2019

I had a query in #5009 (comment), I chose to not use a nested dict, I can change it if people feel otherwise

That's fine with me. Could you give a listing of all the possible keys of the timing metadata now?

It would be good to also open a PR against the nbformat repo to suggest this change to the official notebook spec (suggested by @rgbkrk in #5009 (comment)).

The idea is that the timing object would be a mapping of arbitrary keys to date strings. The keys represent event names.

Different UIs are free to display and record whatever events they find relevant. This intentionally punts on answering the question "How long did this cell take to execute?" leaving that to answered by different frontends however they like, using these keys. Why? Because there are many different timings we could use for this duration and it isn't clear there is one right one we should always expose.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jul 22, 2019

The idea is that the timing object would be a mapping of arbitrary keys to date strings. The keys represent event names.

Like this?

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jul 22, 2019

Like this?

That looks good! I might make the type a little more specific, so that we specify the keys and values are both strings.

Use registerMessageHook to avoid overriding the default
iopub callback for outputarea
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jul 31, 2019

Also a sample of the timing dict's keys -

iopub.execute_input
iopub.status.busy
shell.execute_reply.started
shell.execute_reply
iopub.status.idle

I created jupyter/nbformat#144 for the nbformat changes.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 5, 2019

@saulshanabrook Do you think we should consider @williamstein 's suggestion ? Seems like a good idea to me.

@Madhu94 Madhu94 changed the title [wip] Add execution timing to cells Add execution timing to cells Aug 5, 2019
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 7, 2019

For the record, he suggested:

Note that we always record (and display in a hover) which kernel was used in evaluating the cell. Is there something in the spec (or envisioned for it) that records this? People can change the kernel at any time while going through and evaluating cells...

@Madhu94 This does make some sense, but I would like to cover this in a separate issue, since I think this can be done without that feature.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 8, 2019

Okay, then let's proceed with review/merge ? (taken care of the comments above)

@saulshanabrook saulshanabrook self-requested a review Aug 8, 2019
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 12, 2019

I am going to bring this up at this week's dev meeting. Feel free to attend if you would like!

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 13, 2019

I am going to bring this up at this week's dev meeting. Feel free to attend if you would like!

I couldn't join. Or at least no one was there when I connected. I assume it should happen about now.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 13, 2019

@Madhu94 it should be tomorrow

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 13, 2019

Oh. I got the timezone wrong then (I did use the helpful app that was linked :) ) Wed 9 AM PST seemed to point to Tue 9.30 PM IST.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 13, 2019

Sounds good, yeah it will be 24 hours after than when you posted your comment today. If you do not attend, I will also post notes here on what is discussed. I wanted to bring up the question of storing whether to record execute timing in the notebook metadata makes sense.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 18, 2019

From the dev meeting -

  • We decided to rename the 'timing' key to 'execution' and experiment with this first, which I think means I should suspend the nbformat PR for a bit.

I'll also take a look at the existing nbextension and see how easy it would be to make a UI for displaying this (even if the UI is done in userland)

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 18, 2019

FYI @Madhu94 I just gave you write permissions to this repo. You should be able to set labels and merge PRs. Thanks for all your help!

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 26, 2019

@Madhu94 Since you renamed the timing key to execution, it should also be renamed on the nbformat PR with a description of the intent (i.e. there might be other key, value pairs now that are not timing information, like the kernel name)

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 26, 2019

Thanks for reminding me! I meant to suspend the nbformat PR since we planned to leave it as an experimental feature in lab. Let me know if that was misunderstood?

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 26, 2019

I meant to suspend the nbformat PR since we planned to leave it as an experimental feature in lab. Let me know if that was misunderstood?

That that makes sense, but it seems like since you posted that PR, other projects have opened issues to conform to it. So I don't wanna end up in a situation where nteract/cocalc switch to using the timings key while we are using the execution key.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 26, 2019

Yeah, makes sense. I'll update the nbformat PR instead of closing it.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Aug 26, 2019

@Madhu94 Thank you!

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Aug 27, 2019

Taken care ^

@mlucool
Copy link
Contributor

@mlucool mlucool commented Sep 6, 2019

@blink1073 @saulshanabrook Is there any reason not to merge this now?

Copy link
Member

@saulshanabrook saulshanabrook left a comment

OK I tested this locally again. If you are in a notebook, you can now run the "toggle recording cell timing" command:
Screen Shot 2019-09-09 at 11 29 29 AM

Then, cells that you execute will record the timing in the cell metadata, which can be seen in the inspector
Screen Shot 2019-09-09 at 11 29 20 AM
pane:

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 9, 2019

Thank you @Madhu94 for keeping on this for so long, despite the slow review on my part. I am going to merge this in. This enables third party extension to create UIs which show the timing per cell and for us to experiment with different ways of displaying this.

@saulshanabrook saulshanabrook merged commit 9debf60 into jupyterlab:master Sep 9, 2019
7 of 9 checks passed
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Sep 9, 2019

This was quick, thanks for the help:)

Also, I started attempting a UI extension for this but adding a footer to a cell widget in lab seems unnaturally hard (compared to how easy it is to extend other parts of lab)

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 9, 2019

I started attempting a UI extension for this but adding a footer to a cell widget in lab seems unnaturally hard (compared to how easy it is to extend other parts of lab)

Ah yeah, I believe that! We are in the process of refactoring the cell creation widgets anyways for the RTC work... I think though if you found out a place in core to add an extension point for a post-cell widget, we could add that as an extension point. Or just some way generally to process a cell before it is displayed.

Might be a good thing to bring up at this weeks meeting if you wanna attend.

@jakirkham
Copy link

@jakirkham jakirkham commented Sep 9, 2019

Is there an issue to track exposing this information in the UI?

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Sep 9, 2019

@jakirkham It is unclear at this point if this should be part of Lab or an extension. Note that this wasn't part of the Notebook either, it was just an extension

@jakirkham
Copy link

@jakirkham jakirkham commented Sep 9, 2019

Sure that makes sense. Am just curious if there is an issue to subscribe to about updates on the UI portion. Sounds like not ATM, which is fine.

@jasongrout jasongrout added this to the 1.2 milestone Sep 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants