Add an option to show the cell outputs per second #116

flaviomartins · 2024-03-27T15:55:09Z

Closes #115

The cell outputs per second calculated using the last execution time and cell.model.outputs.length.

flaviomartins · 2024-04-02T14:43:52Z

Fixed the eslint/prettier error/warnings. Rebased on master.

krassowski

Thank you, it looks CI is green! It seems that the numerator here is always fixed to 1 because each time there is only one execution. I am slightly worried that this might confuse users initially: they may expect it to count some actual execution cycles, like iterations or operations, which is rather difficult to do in a language-agnostic way in this extension. At a minimum I would recommend using a more precise wording.

Alternatively, slight variations of this PR with a non-constant numerator could include counting the number of outputs/time, or number of output updates/time.

krassowski · 2024-04-02T17:18:42Z

schema/settings.json

+    "showExecutionsPerSecond": {
+      "type": "boolean",
+      "title": "Show Executions Per Second",
+      "description": "Show the executions per second calculated from the last execution time.",


I think this should be explicit that number of executions is always one.

I have changed this to explicitly say Cell Executions Per Second, but I understand it might not resolve the issue completely.

I'm in favour of outputs/time or updates/time (keeping it language-agnostic).

@krassowski can it be done?

@krassowski do tou know if there is a listener/hook exposed to jupyterlab extensions I can use? (for each write to stdout/output)

There is a outputLengthChanged signal in output area which you should be able to access via cell.outputArea.outputLengthChanged.

mlucool · 2024-04-02T21:53:04Z

@flaviomartins thanks for the contribution. Can you share a gif?

Also, did you sign the CLA mentioned here?

flaviomartins · 2024-04-04T10:17:45Z

@flaviomartins thanks for the contribution. Can you share a gif?

Also, did you sign the CLA mentioned here?

Here is a GIF:

Signed the CLA today.

flaviomartins · 2024-04-11T12:54:59Z

Here is a GIF:

krassowski · 2024-04-11T13:25:00Z

@flaviomartins it looks like the lint is failing

flaviomartins · 2024-04-11T13:40:07Z

@flaviomartins it looks like the lint is failing

It should work now. Forgot a simple: jlpm run eslint:check

krassowski

Tested locally, works well! I left a small suggestion on reducing the length of the text. One question, @flaviomartins would you be interested in adding a test case for this as well to ensure it works as other changes are made to the codebase in the future?

This test case could be a slight modification of:

jupyterlab-execute-time/ui-tests/tests/timing_outcomes.spec.ts

Lines 33 to 42 in 9c56a05

    
           test('"Last executed at" state', async ({ page }) => { 
        
             const cell = await page.notebook.getCell(4); 
        
             // Execute cell and wait for it to complete 
        
             await page.notebook.runCell(4); 
        
             const widget = await cell.waitForSelector('.execute-time'); 
        
             expect(await widget.textContent()).toContain('Last executed at'); 
        
             expect(await maskedScreenshot(widget)).toMatchSnapshot('last-executed.png'); 
        
           });

but after toggling the settings for showing the outputs/second:

jupyterlab-execute-time/ui-tests/tests/timing_outcomes.spec.ts

Lines 10 to 11 in 9c56a05

    
           test.use({ 
        
             mockSettings: {

krassowski · 2024-04-19T10:48:30Z

src/ExecuteTimeWidget.ts

+          const numberOfOutputs = cell.model.outputs.length;
+          if (this._settings.showOutputsPerSecond && numberOfOutputs > 0) {
+            const outputsPerSecond = executionsPerSecond / numberOfOutputs;
+            msg += ` and generated ${numberOfOutputs} output(s)`;


This is getting a bit long:

Can you modify this line to only append "s" when the number of outputs is >1? This will also remove the parentheses making it shorter in both cases.

additional idea: "and generated" could be replaced by shorter "displaying"

Makes sense. It looks like this now. Much shorter 👍

@krassowski I am not familiar with ui-tests. How can we get something like last-executed.png?

It will be auto-generated when running the test for the first time: https://github.com/deshaw/jupyterlab-execute-time/tree/master/ui-tests#run-the-tests

But in general I would just push the code and download the artifacts from CI

flaviomartins · 2024-04-24T12:42:26Z

Added a new notebook for ui-tests since the Timing_outcomes.ipynb cells don't produce any output and the display of outputs per second only happens when the numberOfOutputs > 0.

krassowski

Thank you @flaviomartins, looks good to me!

mlucool · 2024-04-26T12:38:48Z

src/ExecuteTimeWidget.ts

+          endTime,
+          startTime
+        );
+        if (this._settings.minTime <= executionTimeMillis) {


I think there is a bug here - minTime is in s and now this is compared in ms

I added the conversion back in a new commit. Thanks!

mlucool · 2024-04-26T12:41:31Z

schema/settings.json

+    "showOutputsPerSecond": {
+      "type": "boolean",
+      "title": "Show Outputs Per Second",
+      "description": "Show the outputs per second calculated from the last cell execution time and number of outputs.",


It's not clear from the name if this is in real time. How about: "After a cell has finished running, show the outputs per second calculated from the last cell execution time and number of outputs."

Sounds good. Updated the description with your copy.

metric after the cell is executed and not while the cell is running

mlucool · 2024-04-26T18:02:49Z

Thanks for the PR!

Show the executions per second calculated from the last execution time.

b9ef0a5

krassowski reviewed Apr 2, 2024

View reviewed changes

Improve description using a more precise wording.

61a9240

krassowski self-requested a review April 9, 2024 16:19

flaviomartins changed the title ~~Create an option to show the cell executions per second calculated using the last execution time.~~ Create an option to show the cell outputs per second calculated using the last execution time and cell.model.outputs.length Apr 11, 2024

Now shows the number of cell outputs per second.

cd6d9a3

krassowski reviewed Apr 19, 2024

View reviewed changes

flaviomartins added 4 commits April 19, 2024 16:12

Shorten the message displaying outputs/s

69303e7

Further improve outputs per second message

cca94f0

small fix to outputPerSecond option description

6b5bb0a

add timing_outputs_outcomes.spec.ts for ui-tests

771f5ac

krassowski changed the title ~~Create an option to show the cell outputs per second calculated using the last execution time and cell.model.outputs.length~~ Create an option to show the cell outputs per second Apr 26, 2024

krassowski added the enhancement New feature or request label Apr 26, 2024

krassowski changed the title ~~Create an option to show the cell outputs per second~~ Add an option to show the cell outputs per second Apr 26, 2024

krassowski added 3 commits April 26, 2024 12:51

Do not test execution started again, remove unused import and cells

cb935d0

Add changelog entry

009e7f9

Adjust cell index after removing unused cells

d553ca0

krassowski approved these changes Apr 26, 2024

View reviewed changes

mlucool reviewed Apr 26, 2024

View reviewed changes

flaviomartins added 2 commits April 26, 2024 18:11

fix comparison of millis with seconds

3927b85

clarify in the description that showOutputsPerSecond option displays the

5d84534

metric after the cell is executed and not while the cell is running

mlucool approved these changes Apr 26, 2024

View reviewed changes

mlucool merged commit 7ebb8dc into deshaw:master Apr 26, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to show the cell outputs per second #116

Add an option to show the cell outputs per second #116

flaviomartins commented Mar 27, 2024 •

edited by krassowski

Loading

flaviomartins commented Apr 2, 2024

krassowski left a comment

krassowski Apr 2, 2024

flaviomartins Apr 4, 2024

flaviomartins Apr 9, 2024 •

edited

Loading

krassowski Apr 10, 2024

mlucool commented Apr 2, 2024

flaviomartins commented Apr 4, 2024

flaviomartins commented Apr 11, 2024

krassowski commented Apr 11, 2024

flaviomartins commented Apr 11, 2024

krassowski left a comment

krassowski Apr 19, 2024

flaviomartins Apr 19, 2024

flaviomartins Apr 20, 2024

krassowski Apr 20, 2024

krassowski Apr 20, 2024

flaviomartins commented Apr 24, 2024 •

edited

Loading

krassowski left a comment

mlucool Apr 26, 2024

flaviomartins Apr 26, 2024

mlucool Apr 26, 2024

flaviomartins Apr 26, 2024

mlucool commented Apr 26, 2024

	test('"Last executed at" state', async ({ page }) => {
	const cell = await page.notebook.getCell(4);

	// Execute cell and wait for it to complete
	await page.notebook.runCell(4);

	const widget = await cell.waitForSelector('.execute-time');
	expect(await widget.textContent()).toContain('Last executed at');
	expect(await maskedScreenshot(widget)).toMatchSnapshot('last-executed.png');
	});

Add an option to show the cell outputs per second #116

Add an option to show the cell outputs per second #116

Conversation

flaviomartins commented Mar 27, 2024 • edited by krassowski Loading

flaviomartins commented Apr 2, 2024

krassowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviomartins Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlucool commented Apr 2, 2024

flaviomartins commented Apr 4, 2024

flaviomartins commented Apr 11, 2024

krassowski commented Apr 11, 2024

flaviomartins commented Apr 11, 2024

krassowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviomartins commented Apr 24, 2024 • edited Loading

krassowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlucool commented Apr 26, 2024

flaviomartins commented Mar 27, 2024 •

edited by krassowski

Loading

flaviomartins Apr 9, 2024 •

edited

Loading

flaviomartins commented Apr 24, 2024 •

edited

Loading