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

Fix outputarea package from not detecting updates #15642

Merged
merged 4 commits into from Jan 17, 2024

Conversation

MFA-X-AI
Copy link
Contributor

References

#15633 #15004
Changes introduced in #14911

Code changes

The code updates the renderer retrieval in the OutputArea widget by directly accessing the renderer from the layout.widgets array.

User-facing changes

Output panels are still able to display plots as raised in #14564 (comment) while still able to detect ongoing output streams.

fixed.output.mp4

Backwards-incompatible changes

None

Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

Copy link

welcome bot commented Jan 15, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@krassowski
Copy link
Member

Thank you for working on it! Would you be able to take a look at the unit tests (for example in https://github.com/jupyterlab/jupyterlab/blob/main/packages/outputarea/test/ but possible in other packages too) to see if we a test ensuring correct behaviour could be added easily?

@MFA-X-AI
Copy link
Contributor Author

@krassowski looks like the test runs on a different environment than the browser, I can't seem to replicate the original bug. Currently investigating.

In the mean time, could you rerun the UI test action again for this PR? Seems that it ran into some of the test errors that you ran here, wanted to know if it's because of this change or just flaky tests.

Thanks!

@krassowski
Copy link
Member

Thank you!

In the mean time, could you rerun the UI test action again for this PR? Seems that it ran into some of the test errors that you ran here, wanted to know if it's because of this change or just flaky tests.

Yes, these were just flaky tests.

@MFA-X-AI
Copy link
Contributor Author

@krassowski A few changes:

  • the updated code preserved the original way of accessing the renderer by having a fallback on panel, but now additionally checking if the renderModel exists on the widget (previously panel.widgets[0] was always True, even if if it didn't have a renderModel). Although the previous commit didn't seem to break anything, seeing how the outputarea is used everywhere, it's better to be safe. 😅
  • We managed to reproduce the error on the unit test. 🥳
    Turns that it happens in SimplifiedOutputArea, as both our extension and the kernel extension example was using it.
    Narrowing that down, the new unittest can now catch the mismatch between what is held by the model and what is rendered by the widget.

Here's the output from the pre-fixed outputarea.

 outputarea/widget › OutputArea › .execute() › should continuously display delayed outputs

    expect(received).toBe(expected) // Object.is equality

    - Expected  - 1
    + Received  + 3

    - Hello Jupyter! 0
    + Hello Jupyter! 0
    + Hello Jupyter! 1
    + Hello Jupyter! 2
      ↵

      458 |         ].join('\n');
      459 |         await SimplifiedOutputArea.execute(code, widget0, ipySessionContext);
    > 460 |         expect(model0.toJSON()[0].text).toBe(widget0.node.textContent)
          |                                         ^
      461 |       });
      462 |     });
      463 |

      at Object.<anonymous> (test/widget.spec.ts:460:41)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 29 passed, 30 total
Snapshots:   0 total
Time:        47.987 s
Ran all test suites related to changed files.

And here with the update:

Test Suites: 1 passed, 1 total
Tests:       30 passed, 30 total
Snapshots:   0 total
Time:        47.258 s, estimated 178 s
Ran all test suites related to changed files.

Hopefully this is good enough to include in the next release. Thanks!

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @MFA-X-AI!

As discussed on today's JupyterLab call, I will merge it and release in 4.1.0b1. It looks like it could be backported to 4.0.x - I will open a backport but wait with merging until we hear more feedback from testing of the new beta. Thanks again!

@krassowski krassowski added this to the 4.0.x milestone Jan 17, 2024
@krassowski krassowski merged commit 0922e74 into jupyterlab:main Jan 17, 2024
76 of 77 checks passed
Copy link

welcome bot commented Jan 17, 2024

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@krassowski
Copy link
Member

@meeseeksdev please backport to 4.0.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants