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

MNT: Unpin maxversion of ipywidgets now that voila 0.4 is out, minversion ipywidgets now 8 #1788

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Oct 27, 2022

Description

This pull request is to fix #1613 and fix #1787 and fix #1926 and fix #1938

xref #1791 .

Multiple devs should check that things are truly fixed by this new voila release since several things were broken before. Make sure your ipywidgets is at least v8 and voila is v0.4 when you test.

Blocked by

Bugs check list

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

@pllim pllim added the no-changelog-entry-needed changelog bot directive label Oct 27, 2022
@pllim pllim added this to the 3.2 milestone Oct 27, 2022
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Base: 91.79% // Head: 91.79% // No change to project coverage 👍

Coverage data is based on head (d861561) compared to base (04a744c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1788   +/-   ##
=======================================
  Coverage   91.79%   91.79%           
=======================================
  Files         140      140           
  Lines       14951    14951           
=======================================
  Hits        13724    13724           
  Misses       1227     1227           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pllim
Copy link
Contributor Author

pllim commented Oct 28, 2022

This now conflicts with the proposed change for another bug in #1791

@pllim pllim changed the title MNT: Unpin maxversion of ipywidgets now that voila 0.4 is out MNT: Unpin maxversion of ipywidgets now that voila 0.4 is out, minversion ipywidgets now 8 Oct 28, 2022
rosteen
rosteen previously approved these changes Oct 28, 2022
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I confirmed that the stand-alone version of the app works with this, and it also fixes the recent problem I was seeing with the popped-out application viewer heights. The latter is a bug in 3.1, so I would advocated for changing the milestone of this to 3.1.1.

@rosteen
Copy link
Collaborator

rosteen commented Oct 28, 2022

I can't quite tell if the test failure is a transient connection error downloading a file or an actual issue with dev Viola install. Maybe both? 😅

@pllim
Copy link
Contributor Author

pllim commented Oct 28, 2022

HTTP Error 503: Service Temporarily Unavailable -- I will restart.

@pllim
Copy link
Contributor Author

pllim commented Oct 28, 2022

CI is all green now.

@pllim pllim modified the milestones: 3.2, 3.1.1 Oct 28, 2022
@pllim pllim added 💤backport-v3.1.x on-merge: backport to v3.1.x bug Something isn't working embed Regarding issues with front-end embedding Ready for final review labels Oct 28, 2022
kecnry
kecnry previously approved these changes Nov 1, 2022
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Standalone and popout windows seem to behave well for me. Not sure why one test isn't reporting its status though... but otherwise I say we get this in!

@pllim
Copy link
Contributor Author

pllim commented Nov 1, 2022

Ah, I updated the branch protection. Lemme rebase when I get a chance. Good catch!

@pllim
Copy link
Contributor Author

pllim commented Nov 1, 2022

Did @duytnguyendtn ever have a chance to test this on Windows? Or did you already try that? If not, I'll wait till I have time to do that before merge.

@pllim
Copy link
Contributor Author

pllim commented Nov 1, 2022

Timeout error is unrelated.

@pllim
Copy link
Contributor Author

pllim commented Nov 2, 2022

Unfortunately, I found #1598 (comment) to still be a problem, so I am turning this back into draft.

@rosteen
Copy link
Collaborator

rosteen commented Dec 16, 2022

Alright, I started from a fresh environment and installed from this PR. I confirmed that the sidecar height bug is fixed, but popout seems completely broken for me - I get a new window, but it's blank and never populates. Could someone else see if they see the same thing?

@maartenbreddels
Copy link
Collaborator

maartenbreddels commented Dec 16, 2022 via email

@javerbukh
Copy link
Contributor

I'm seeing the opposite @rosteen , sidecar height is not updating accurately but popout works just fine. Clean conda environment and ipywidgets version 8.0.3.

@javerbukh
Copy link
Contributor

This is what I see when the sidecar tab is below the notebook view
Screen Shot 2022-12-16 at 3 40 10 PM

@rosteen
Copy link
Collaborator

rosteen commented Dec 16, 2022

Interesting, I get a scrollbar on the sidecar tab so that I can see the rest of the GUI.

@javerbukh
Copy link
Contributor

@rosteen Same, is that the expected behavior?

@rosteen
Copy link
Collaborator

rosteen commented Dec 16, 2022

I thought so, but maybe we need @eteq to confirm what the expected behavior is.

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2022

On my end (Imviz on Windows 10):

✔️Spinner shows in Link Control when click on WCS.
✔️Standalone app still works.
✔️Viewer resizes display when popped out.
✔️Does sidecar work? I used "sidecar:right"

@pllim
Copy link
Contributor Author

pllim commented Dec 16, 2022

I rebased this PR, just in case. 🤞

@pllim
Copy link
Contributor Author

pllim commented Dec 20, 2022

FYI. I added the bump of ipykernel here too. Please see if your problems are gone now. Thanks!

now that voila 0.4 is out. In fact, we pin
minversion ipywidgets to its new release.

Also bump pin for sidecar to they are all compatible.
@rosteen rosteen modified the milestones: 3.1.3, 3.2.1 Jan 4, 2023
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I just re-tested with sidecar and I don't see any difference between the behavior here and on main, so I think any changes to that behavior can be separate work if needed. I think we can merge this.

@pllim pllim modified the milestones: 3.2.1, 3.3 Jan 18, 2023
@pllim pllim removed the 💤backport-v3.1.x on-merge: backport to v3.1.x label Jan 18, 2023
@pllim
Copy link
Contributor Author

pllim commented Jan 18, 2023

Okay, I'll squash-merge. We can always revert if someone finds some new problem after-the-fact. Let's not backport. Thanks!

@pllim pllim merged commit 8faf482 into spacetelescope:main Jan 18, 2023
@pllim pllim deleted the unpin-ipyw-maxver branch January 18, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working embed Regarding issues with front-end embedding no-changelog-entry-needed changelog bot directive
Projects
None yet
6 participants