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

Update ecliptic grid color default and update to work with notebook 7.x #356

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

Carifio24
Copy link
Member

With all the colors getting exposed in #355, I accidentally gave the ecliptic grid the wrong starting color (maybe got it mixed up with the ecliptic color?) - it should be green, rather than blue, to match the engine. This PR fixes that.

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #356 (b694915) into master (225b413) will decrease coverage by 0.08%.
The diff coverage is 42.85%.

❗ Current head b694915 differs from pull request most recent head fe5691f. Consider uploading reports for the commit fe5691f to get more accurate results

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
- Coverage   56.58%   56.51%   -0.08%     
==========================================
  Files          25       25              
  Lines        2861     2870       +9     
==========================================
+ Hits         1619     1622       +3     
- Misses       1242     1248       +6     
Files Changed Coverage Δ
pywwt/core.py 79.73% <ø> (ø)
pywwt/jupyter_relay.py 0.00% <0.00%> (ø)
pywwt/jupyter_server.py 35.41% <60.00%> (+2.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Carifio24
Copy link
Member Author

The CI failures are unrelated, and seem to be an issue with the Jupyter notebook package - version 7.0.0 of that package released yesterday.

There's a migration guide for Notebook 7, but updating sounds like it could be a real task - maybe we should pin for now?

@pkgw
Copy link
Contributor

pkgw commented Jul 22, 2023

Well, Notebook 7 is a pretty major update, but we don't actually use the notebook module much — just some infrastructure references in jupyter_server and jupyter_relay, for the main package. It looks like the build errors are mostly the .check_enabled.py helper script, which hopefully should be easy to update to be compatible with both major versions.

@Carifio24
Copy link
Member Author

So from what I can see, we're still getting errors because jupyter_contrib_core (pulled in through jupyter_contrib_extensions here) hasn't been updated for Notebook 7 (i.e. they need to do what you just added in the last commit). See e.g. here (their _compat.nbextensions also imports notebook directly).

@pkgw
Copy link
Contributor

pkgw commented Jul 26, 2023

Things seemed to be working for me with notebook >= 7 and without that package installed, so I think the question is, can we perhaps now avoid installing it altogether? Maybe we only need one of its dependencies?

@Carifio24
Copy link
Member Author

That explicit installation inside the CI workflow is the only reference I can find to jupyter_contrib_nbextensions. It was added in this commit, so I'm assuming the jupyter nbextension list command was failing without it? (the logs are no longer around). From looking at their list of included extensions, it's not immediately clear to me why that would be, but yeah, maybe we could find the relevant extension and only install that.

@pkgw
Copy link
Contributor

pkgw commented Jul 27, 2023

OK, I think I may have a solution for the next set of issues.

Current build failure is a conda problem unrelated to this PR that's kicking in before the CI can test it, though; I think the problem will be resolved over the next day or so. (I think the current release of conda needs to be yanked and replaced.)

@pkgw pkgw closed this Jul 27, 2023
@pkgw pkgw reopened this Jul 27, 2023
@pkgw
Copy link
Contributor

pkgw commented Jul 27, 2023

Cool, yes, looks like this problem is dealt with now. The Linux py310 build failed with a segfault while running the test suite (!) but hopefully that is a once-in-a-blue-moon problem ...

@pkgw pkgw merged commit 449f88a into WorldWideTelescope:master Jul 28, 2023
@pkgw pkgw changed the title Update ecliptic grid color default Update ecliptic grid color default and update to work with notebook 7.x Jul 28, 2023
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.

2 participants