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

Support Update Display Data #10874

Merged
merged 19 commits into from
Mar 31, 2020
Merged

Support Update Display Data #10874

merged 19 commits into from
Mar 31, 2020

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Mar 30, 2020

For #10873

We've never handled the update_display_data message correctly as it can affect other cells. This is necessary to get some ipywidgets to work correctly.

@rchiodo rchiodo marked this pull request as ready for review March 30, 2020 23:03
@rchiodo rchiodo self-assigned this Mar 30, 2020
@rchiodo
Copy link
Author

rchiodo commented Mar 30, 2020

Debating with myself if I should add a functional test for this. Probably makes sense to

const data: nbformat.ICodeCell = c.cell.data as nbformat.ICodeCell;
const changedOutputs = data.outputs.map(o => {
if (
o.output_type === 'display_data' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Display data can update both execute_result and display_data outputs. I think you would need that here as well.
https://jupyter-client.readthedocs.io/en/stable/messaging.html#display-data

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

@rchiodo rchiodo force-pushed the rchiodo/aml_widgets_2 branch from fdbc150 to caeddae Compare March 30, 2020 23:24
@rchiodo rchiodo requested a review from IanMatthewHuff March 30, 2020 23:35
@codecov-io
Copy link

codecov-io commented Mar 30, 2020

Codecov Report

Merging #10874 into master will decrease coverage by 34.65%.
The diff coverage is 18.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #10874       +/-   ##
===========================================
- Coverage   60.68%   26.02%   -34.66%     
===========================================
  Files         580      328      -252     
  Lines       31525    17530    -13995     
  Branches     4479     2498     -1981     
===========================================
- Hits        19130     4563    -14567     
- Misses      11425    12933     +1508     
+ Partials      970       34      -936     
Impacted Files Coverage Δ
...rc/client/common/application/webPanels/webPanel.ts 11.84% <ø> (ø)
...ent/common/application/webPanels/webPanelServer.ts 12.67% <ø> (-61.98%) ⬇️
.../datascience/interactive-common/interactiveBase.ts 5.55% <0.00%> (-0.07%) ⬇️
...ience/interactive-common/interactiveWindowTypes.ts 100.00% <100.00%> (ø)
src/client/debugger/constants.ts 0.00% <0.00%> (-100.00%) ⬇️
src/client/language/braceCounter.ts 5.40% <0.00%> (-94.60%) ⬇️
src/client/language/textBuilder.ts 8.69% <0.00%> (-91.31%) ⬇️
src/client/language/characterStream.ts 4.41% <0.00%> (-89.71%) ⬇️
src/client/language/tokenizer.ts 4.08% <0.00%> (-89.22%) ⬇️
...t/terminals/codeExecution/terminalCodeExecution.ts 14.54% <0.00%> (-85.46%) ⬇️
... and 462 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89e964f...5f1b982. Read the comment docs.


private async handleKernelMessage(msg: KernelMessage.IIOPubMessage, _requestId: string) {
// Only care about one sort of message, UpdateDisplayData
if (KernelMessage.isUpdateDisplayDataMsg(msg)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a case where we need to lazy load jupyterlab services.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with lazy load of Jupyter, at least from my perspective.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rchiodo do we need these changes even after my new ipywidget implementation?
I can see AML widget working.
However we have changes in our react code to deal with update display data. Is that still required?

@rchiodo
Copy link
Author

rchiodo commented Mar 31, 2020

@rchiodo do we need these changes even after my new ipywidget implementation?
I can see AML widget working.
However we have changes in our react code to deal with update display data. Is that still required?

Yes for other reasons. This code doesn't work without it but it does in jupyter:

# %%
import itertools
import time

from IPython.display import Markdown


# %%

dh = display(display_id=True)


# %%
display(Markdown('# Display Update Fun'))
dh.display(Markdown(''))


# %%
for i in itertools.cycle(range(0x1F600, 0x1F650)):
    dh.update(Markdown(f'## &#{i}; {hex(i)}'))
    time.sleep(0.5)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

@rchiodo rchiodo merged commit 7a58ce8 into master Mar 31, 2020
@rchiodo rchiodo deleted the rchiodo/aml_widgets_2 branch March 31, 2020 18:08
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants