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

Support python 3.12 / notebook>=7 and drop support for python 3.9. #646

Merged
merged 5 commits into from
May 15, 2024

Conversation

JoepVanlier
Copy link
Member

@JoepVanlier JoepVanlier commented May 8, 2024

Why this PR?
It's good to stay relatively up to date with Python versions. This PR drops support for Python 3.9 and adds support for 3.12.

We could consider keeping 3.9 for one more release and immediately drop it on the next (to ease any transition issues) or just rip the band-aid off now. I have a mild preference for the latter, since 3.9 is old, and all the dependency changes we are already doing for jupyter notebook might require a new environment for some anyway.

Jupyter
With 3.12 comes a necessary change, which is the switch to jupyter notebook>=7. Reason being that notebook<7 pins us to pyzmq<25. For a while, they added this pin officially, but they have since dropped this since binaries for pyzmq<25 are not being created for python 3.12 and this effectively locks you into old versions of python. That said, I did do a bunch of testing, and for us, without the pin, I still get Uncaught exception in ZMQStream callback after a while on windows, after which the interactive widgets stop working responding entirely (this is the issue that was the original reason for the pin).

We should update. I have tested the latest installation instructions with notebook>7 on conda/windows, pip/windows and pip/macand I think we should just do it. If there are issues, we will resolve them as they come. Staying with something that is essentially deprecated is just kicking the can down the road.

Interactive widget
Since we do want interactive widgets, I have added ipympl as an extra dependency.
Note that this means using the magic %matplotlib widget rather than %matplotlib notebook.

conda
I have also dropped some of the special conda instructions in the installation guide, as it seems they are also no longer needed.

Docs build here. Most relevant is probably the FAQ.

Note that the tests aren't green because the 3.9 action isn't being run any more. I will flip the switch on that once I get a greenlight on this PR.

@JoepVanlier JoepVanlier marked this pull request as ready for review May 13, 2024 17:24
@JoepVanlier JoepVanlier requested review from a team as code owners May 13, 2024 17:24
@JoepVanlier JoepVanlier changed the title python 3.12 (wip) Support python 3.12 / notebook>=7 and drop support for 3.9. May 13, 2024
@JoepVanlier JoepVanlier changed the title Support python 3.12 / notebook>=7 and drop support for 3.9. Support python 3.12 / notebook>=7 and drop support for python 3.9. May 13, 2024
Copy link
Member

@dean0x7d dean0x7d left a comment

Choose a reason for hiding this comment

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

The updates to the docs look great! Nice to be moving forward.

changelog.md Outdated
@@ -4,6 +4,7 @@

#### New features

* Support Python `3.12`.
Copy link
Member

Choose a reason for hiding this comment

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

You may want to mention dropping 3.9 in the same line. Feels kinda natural. "Added support for Python 3.12, dropped support for 3.9."

setup.py Outdated
"jupyter_client<8", # https://github.com/jupyter/notebook/issues/6748
"pyzmq<25", # https://github.com/jupyter/notebook/issues/6748
"jupyter_client>=8",
"ipympl",
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have some minimum defined here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good catch on this front actually, since we need the latest ipympl for the version of matplotlib that we are using.

setup.py Outdated
"pyzmq<25", # https://github.com/jupyter/notebook/issues/6748
"jupyter_client>=8",
"ipympl",
"pyzmq",
Copy link
Member

Choose a reason for hiding this comment

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

Is pyzmq even needed here anymore? It's not a direct dependency and we don't pin it. So probably doesn't even need to be in the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Removing...

Copy link
Collaborator

@aafkevandenberg aafkevandenberg left a comment

Choose a reason for hiding this comment

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

Looking good. It would be good to test the update on several laptops internally, MacOS and Windows, before release.

docs/install.rst Outdated Show resolved Hide resolved
docs/install.rst Show resolved Hide resolved
@JoepVanlier
Copy link
Member Author

JoepVanlier commented May 14, 2024

Looking good. It would be good to test the update on several laptops internally, MacOS and Windows, before release.

Is there a particular configuration or installation procedure that you are worried about?

I have tested this on three machines. My desktop at home (Windows, w/pip), my DELL at work (Windows, both pip and conda) and my work mac (MacOS with pip inside a conda environment and with conda itself). I also tried installing the dependencies for the last version of Pylake on Windows and then updating to the latest and that also worked.

To me it seems like it would lead to diminishing returns unless we know specifically what configuration we want to test.

Especially since that to install latest (prior to release), the installation method is always going to be slightly different than what we would truly have on a release.

Copy link
Collaborator

@aafkevandenberg aafkevandenberg left a comment

Choose a reason for hiding this comment

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

Having it tested on 3 devices with 2 different operating systems should be sufficient. I will then test it right after release as well.

Copy link
Contributor

@rpauszek rpauszek left a comment

Choose a reason for hiding this comment

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

fantastic! 🚀🚀🚀🚀

@JoepVanlier JoepVanlier merged commit c91aa7c into main May 15, 2024
8 checks passed
@JoepVanlier JoepVanlier deleted the py312 branch May 15, 2024 15:02
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.

4 participants