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

Using the new Comm Python package #3533

Merged
merged 6 commits into from
Mar 21, 2023

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Aug 1, 2022

Related discussions:

Currently working on a Python comm package on https://github.com/martinRenou/comm for now, but it should probably be moved in the IPython org

This has the benefit of being able to:

@github-actions
Copy link

github-actions bot commented Aug 1, 2022

Binder 👈 Launch a binder notebook on branch martinRenou/ipywidgets/move_comm

@@ -421,7 +421,7 @@ def get_view_spec(self):

_view_count = Int(None, allow_none=True,
help="EXPERIMENTAL: The number of views of the model displayed in the frontend. This attribute is experimental and may change or be removed in the future. None signifies that views will not be tracked. Set this to 0 to start tracking view creation/deletion.").tag(sync=True)
comm = Instance('ipykernel.comm.Comm', allow_none=True)
Copy link
Member

Choose a reason for hiding this comment

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

See #3602 for removing this as a trait.

@SylvainCorlay
Copy link
Member

This has the benefit of being able to:

remove the ipykernel dependency in ipywidgets #3533
remove the ipykernel monkey patch in xeus-python jupyter-xeus/xeus-python#548
remove the ipykernel package mock in jupyterlite's pyolite
remove the ipykernel package mock in emscripten-forge

Wow. I think it is quite an improvement.

@martinRenou martinRenou changed the title POC: Creation of a Comm Python package Using the new Comm Python package Feb 10, 2023
Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Great work.

I do worry we do not have a test yet that exposes ipython/ipykernel#1090

I'm pretty sure this fixes it, but #3653 also didn't pick up this issue, which leaves me a bit worries that we're vulnerable for regressions.

python/ipywidgets/ipywidgets/__init__.py Outdated Show resolved Hide resolved
python/ipywidgets/ipywidgets/widgets/tests/utils.py Outdated Show resolved Hide resolved
python/ipywidgets/ipywidgets/widgets/tests/utils.py Outdated Show resolved Hide resolved
python/ipywidgets/ipywidgets/widgets/widget_output.py Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member Author

Thanks a lot for your review @maartenbreddels ! I will resolve your comment and get the CI green hopefully :D

@martinRenou
Copy link
Member Author

@maartenbreddels it was difficult to get green but now it seems good!

Thankfully, the Python 3.7 build requires an old version of ipykernel (latest ipykernel does not support 3.7?) so we have tests against the old versions and new versions.

@martinRenou
Copy link
Member Author

Would you have time to make it a review @maartenbreddels ?

Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

awesome work, some minor comments

python/ipywidgets/ipywidgets/__init__.py Show resolved Hide resolved
@@ -229,7 +229,7 @@ def test_update_dynamically(self, send_state): #pylint: disable=no-self-use
assert box.layout.grid_template_areas == ('"top-left top-right"\n' +
'"bottom-left bottom-right"')
# check whether frontend was informed
send_state.assert_called_once_with(key="grid_template_areas")
send_state.assert_called_with(key="grid_template_areas")
Copy link
Member

Choose a reason for hiding this comment

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

why is it not called once? is it called multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think the test was faulty from the beginning. Though it's unclear to me as to why my PR fixes it.

Setting bottom_left would not only trigger changing grid_template_areas but also a couple more properties from layout

python/ipywidgets/ipywidgets/widgets/widget.py Outdated Show resolved Hide resolved
python/ipywidgets/ipywidgets/widgets/widget.py Outdated Show resolved Hide resolved
python/ipywidgets/ipywidgets/widgets/widget_output.py Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member Author

Thanks for the review! I resolved your comments

@pllim
Copy link
Contributor

pllim commented Mar 20, 2023

Hello. Is there a plan to merge and release this soon? We are seeing deprecation warning from stable version of ipykernel downstream. Thank you!

@maartenbreddels
Copy link
Member

@martinRenou do you know what caused that change?
I'm fine removing the newlines, but maybe good to be aware what is responsible for this change?

Hello. Is there a plan to merge and release this soon?

Yes, I think we should get this released asap, which I could do tomorrow if we manage to get this in.

martinRenou and others added 5 commits March 21, 2023 09:21
This test was supposed to fail, as setting the bottom_left value will
trigger a ipywidgets.Layout.send_state on `child.layout.grid_area = position`
(ipywidgets.widgets.widget_template.py L419) and on
`self.layout.grid_template_areas = grid_template_areas_css`
(ipywidgets.widgets.widget_template.py L448)
Co-authored-by: Maarten Breddels <[email protected]>
@martinRenou
Copy link
Member Author

@maartenbreddels all green!

@maartenbreddels maartenbreddels merged commit 785d159 into jupyter-widgets:main Mar 21, 2023
@maartenbreddels
Copy link
Member

@martinRenou great work, i guess we also need to backport this to 7.x
do you want to try the bot?

@martinRenou martinRenou deleted the move_comm branch March 21, 2023 09:07
@martinRenou
Copy link
Member Author

@meeseeksdev please backport to 7.x

@lumberbot-app
Copy link

lumberbot-app bot commented Mar 21, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 7.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 785d1598b38f6fb43917489f530d14e28876fc24
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3533: Using the new Comm Python package'
  1. Push to a named branch:
git push YOURFORK 7.x:auto-backport-of-pr-3533-on-7.x
  1. Create a PR against branch 7.x, I would have named this PR:

"Backport PR #3533 on branch 7.x (Using the new Comm Python package)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

martinRenou added a commit to martinRenou/ipywidgets that referenced this pull request Mar 21, 2023
* Move comm package

* Fix wrongly written test

This test was supposed to fail, as setting the bottom_left value will
trigger a ipywidgets.Layout.send_state on `child.layout.grid_area = position`
(ipywidgets.widgets.widget_template.py L419) and on
`self.layout.grid_template_areas = grid_template_areas_css`
(ipywidgets.widgets.widget_template.py L448)

* Fix Python 3.7 test

Co-authored-by: Maarten Breddels <[email protected]>
(cherry picked from commit 785d159)
martinRenou added a commit that referenced this pull request Mar 21, 2023
@bollwyvl
Copy link
Contributor

If seeing errors like this in pytest:

    def _create_comm(*args, **kwargs):
        """Create a Comm.
    
        This method is intended to be replaced, so that it returns your Comm instance.
        """
>       raise NotImplementedError("Cannot ")
E       NotImplementedError: Cannot

Adding:

import ipykernel.ipkernel  # noqa

Is enough to trigger the side-effects to overload that manager.

Seems like it should go in the docs... or even in that unhelpful traceback itself, cuz 99% of the time, i reckon people are testing with ipykernel lying around.

lkeegan added a commit to ssciwr/moralization-analyzer that referenced this pull request Mar 23, 2023
- ModelManager features
  - import a spacy model created by DataManager
  - set metadata such as author, license, etc
  - publish the model to hugging face
- add `test_model_manager` notebook with example of use
- add `spacy-huggingface-hub` and `wheel` to dependencies

also

- refactor tests
  - simplify fixtures code in conftest.py
  - add `model_path` fixture to conftest.py
- pin ipywidgets<8.0.5 for now to avoid test failures in CI
  - looks due to this change: jupyter-widgets/ipywidgets#3533
lkeegan added a commit to ssciwr/moralization-analyzer that referenced this pull request Mar 24, 2023
- ModelManager features
  - `load()` imports a spacy model created by DataManager
  - `metadata` dict exposes metadata the user should edit such as author, license, etc
  - `save()` saves changes (currently only the updated metadata can be changed)
  - `publish()` publishes the model to hugging face
- add `test_model_manager` notebook with example of use
- add `spacy-huggingface-hub` and `wheel` to dependencies

also

- refactor tests
  - simplify fixtures code in conftest.py
  - add `model_path` fixture to conftest.py that provides a trained spacy model
- pin ipywidgets<8.0.5 for now to avoid test failures in CI
  - looks like it is due to this change: jupyter-widgets/ipywidgets#3533
marcofavoritobi pushed a commit to bancaditalia/black-it that referenced this pull request Mar 27, 2023
This change is required by the upgrade of ipywidgets.

  jupyter-widgets/ipywidgets#3533

One of the changes in this new version is the removal of the dependency
ipykernel, which we require to add it as explicit dependency.
marcofavoritobi pushed a commit to bancaditalia/black-it that referenced this pull request Mar 27, 2023
This change is required by the release of ipywidgets at version 8.0.5.

  jupyter-widgets/ipywidgets#3533

One of the changes in this new version is the removal of the dependency
ipykernel, which we would require to add it as explicit dependency.

Due to potential breaking changes, we fix its maximum version below 8.0.5.
marcofavoritobi pushed a commit to bancaditalia/black-it that referenced this pull request Mar 27, 2023
This change is required by the release of ipywidgets at version 8.0.5.

  jupyter-widgets/ipywidgets#3533

One of the changes in this new version is the removal of the dependency
ipykernel, which we would require to add it as explicit dependency.

Due to potential breaking changes, we fix its maximum version below 8.0.5.
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.

5 participants