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 registry state lookup #3653

Merged

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Dec 18, 2022

References

Changes

  • use _registry.get instead of register.get
  • update official actions/* to latest versions to squash deprecation warnings
  • split up and upload failing results of Test: JavaScript job

Motivation

This is relatively difficult to demonstrate, as it requires a widget that creates a new client-side widget. For a reproducer see this in-development notebook:

  • run all cells
  • try to add a new .ipynb

Screenshot from 2022-12-18 13-22-47

File "/lib/python3.10/site-packages/ipykernel/comm.py", line 201, in comm_open
    f(comm, msg)
  File "/lib/python3.10/site-packages/ipywidgets/widgets/widget.py", line 421, in handle_comm_opened
    widget_class = register.get(state['_model_module'],
AttributeError: 'function' object has no attribute 'get'
  • it's possible to update the file with this patch, by downloading it and replacing /lib/python3.10/site-packages/ipywidgets/widgets/widget.py

Future Work

Likely this should probably just get reviewed, and hopefully merged, as it's fairly clearly an improvement by inspection.

A test would emulate a client creating a new, pre-registered Widget.

However, a better long-term check would be applying mypy, pyflakes or the new-fangled (but very fast) ruff or some related tool which would do a better job at statically analyzing the code for catching these kinds of issues, but is probably out of scope for this one-line fix.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch bollwyvl/ipywidgets/fix-register-is-a-function

@maartenbreddels
Copy link
Member

Thanks @bollwyvl !
I've pushed a test that will avoid regressions.

And I agree, this could have been avoided. I'll put it on the agenda for next time we have a widget meeting.
(cc @jasongrout who was interested in this as well)

@@ -74,3 +78,27 @@ def test_compatibility():
caller_path = inspect.stack(context=0)[1].filename
assert all(x.filename == caller_path for x in record)
assert len(record) == 6


def test_create_from_frontend():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

@bollwyvl
Copy link
Contributor Author

Beyond static analysis: it's probably worth pulling off the band-aid and applying black, isort, etc. Heck, I've even been using ssort recently on some projects... just more highly subjective things not to have to make a decision/talk about on PRs (after talking about the initial PR). I think just doing it, with a single git history ignore thing, rather than the slow darken process IPython has been following, is easier for most involved.

@maartenbreddels
Copy link
Member

Yes, I think we all totally agree with you. I'd love to see it. What's your opinion on using pre-commit?

@maartenbreddels
Copy link
Member

assert 'application/vnd.jupyter.widget-view+json' in mime_bundle, "widget should have have a view"

Also had this error locally...

@jasongrout
Copy link
Member

it's probably worth pulling off the band-aid and applying black, isort

FYI, #3566 moves to precommit with all those things applied.

@bollwyvl
Copy link
Contributor Author

Fire, pre-commit is great... in CI. If it makes PRs to fix stuff it finds. I don't let it touch my machine, too many unmanaged environments. 🤣

@maartenbreddels
Copy link
Member

Hmm, some CI rot? Two seemingly unrelated workflows are failing. Is this something you touched in this PR @bollwyvl, or do you happen to know how to fix this?

@bollwyvl
Copy link
Contributor Author

seemingly unrelated workflows

Well, they both fail around playwright, of which I can't claim a lot of knowledge... I'm more concerned about the persist OoM on RTD, and the lack of meaningful output on the karma tests.

Without some of these things pinned (or at least archived) during a build, it's hard to say what has shifted. Perhaps a kicked run of main would shed some light on whether this is localized to this PR... I can roll back a bunch of changes, as I don't think there's much doubt about the critical line of the initial commit on this PR.

@bollwyvl
Copy link
Contributor Author

I've slimmed this back down to just the fix and the tests, with de23fa6 being the last commit with the CI changes.

@@ -12,29 +12,20 @@
# A widget with simple traits
class SimpleWidget(Widget):
a = Bool().tag(sync=True)
b = Tuple(Bool(), Bool(), Bool(), default_value=(False, False, False)).tag(sync=True)
b = Tuple(Bool(), Bool(), Bool(), default_value=(False, False, False)).tag(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can i roll these back, too?

@bollwyvl
Copy link
Contributor Author

Even though the error is being discovered by playwright, i'll wager the issue lies with jupyter_server 2.0.2... haven't had a chance to look through the changes:

jupyter-server/jupyter_server@v2.0.1...v2.0.2

@bollwyvl
Copy link
Contributor Author

@bollwyvl
Copy link
Contributor Author

@bollwyvl
Copy link
Contributor Author

@bollwyvl
Copy link
Contributor Author

That was merged... maybe a release forthcoming? Then we can get back to the other CI problems here, i suppose...

@bollwyvl bollwyvl closed this Dec 21, 2022
@bollwyvl bollwyvl reopened this Dec 21, 2022
@bollwyvl
Copy link
Contributor Author

ok, that cleared up those two emergent fails. i'm still not sure if blocking this on fixing the docs/js CI fails is the right play, and would prefer to start a second PR to investigate further... though this work did lead to finding two upstream issues, so at least there's that.

@maartenbreddels
Copy link
Member

Really appreciate the help here Nick!
I'm ok with merging this with docs failing (although that also needs fixing). If we can get the javascript failure sorted, that would make a lot of people happy (and I could do a new release).

@bollwyvl
Copy link
Contributor Author

Ok, tried a lot of things, but turns out it was just ubuntu-latest vs ubuntu-20.04. I rolled back everything except that change, and it appears to be passing.

I've removed the firefox install, as i don't know (or really care) what karma needs to be happy, but it's likely if it does need other things like geckodriver, that they might not be properly updated when updating firefox. It can be hard to reason about the compatibility windows of these things. On a side note: i've been using firefox and geckodriver from conda-forge for some years now, and they've only rarely lead me astray: we try to keep both the latest and the latest LTS available. Firefox in particular is a binary repack, so can cause issues (like missing x11) on e.g. lightweight containers, but github runners have 50gb of crap already installed, so it hasn't been an issue. Also, they aren't snaps (buh, gross).

@maartenbreddels
Copy link
Member

Awesome work. I'm happy to merge this from this PR. I think I'll rebase/squash it into 2 or 3 commits somewhere tomorrow. Or, if you have your own opinion on how many commits there should be, feel free to do it yourself (I'm ok with force pushing)

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Dec 21, 2022 via email

@maartenbreddels maartenbreddels merged commit eacd961 into jupyter-widgets:master Dec 22, 2022
@maartenbreddels
Copy link
Member

I don't question authority 🙇 😄

@maartenbreddels
Copy link
Member

This is released in ipywidgets 8.0.4

@bollwyvl
Copy link
Contributor Author

Thanks all! I can take a look at the docs as well, but might get a little tinker-y with it.

Thoughts on going to pydata-sphinx-theme and myst-nb? I have basically given up trying to get nbsphinx to keep running.

@maartenbreddels
Copy link
Member

I have basically given up trying to get nbsphinx to keep running.

Why is that?

I haven't followed that development much, but I think you are saying myst-nb should replace nbsphinx?

I kind of like the pydata-sphinx-theme, looks clean. Maybe opening an issue on this so we can get some opinions in?

@bollwyvl bollwyvl deleted the fix-register-is-a-function branch December 22, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants