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 python=3.8 #625

Open
jpolton opened this issue Oct 13, 2023 · 11 comments · Fixed by #643
Open

Update python=3.8 #625

jpolton opened this issue Oct 13, 2023 · 11 comments · Fixed by #643
Assignees

Comments

@jpolton
Copy link
Collaborator

jpolton commented Oct 13, 2023

Review the conda / pip build.
Currently builds at python=3.8
Can we move this on?
E.g. Pydap doesn't like python=3.10 yet

@thogar-computer
Copy link
Member

Pydap might now work (ticket content is 12 months old) - plan try 3.10

@thogar-computer
Copy link
Member

look to update git action to include 3.10 if it works and maybe drop 3.8

@thogar-computer
Copy link
Member

My name is Martin Y and I work in a team at the Met Office who support scientists with their use of Scientific Python. I hear you are looking to maximise accessibility for installing future versions of the COAsT package.

The good news is that version 3.2.1 is easy to install at the Met Office, although I wasn’t sure how serious the Python 3.8.10 requirement mentioned on the website was – I had to modify the published command to make this a reality: conda install -c bodc python=3.8.10 coast

You have correctly understood that our installations are behind a security layer – an Artifactory mirror – so it was originally not possible to follow the standard installation instructions as we were not mirroring your bodc channel. Rumours of difficulty is probably worse because our bodc channel mirror was initially set up incorrectly, but it is working fine now. So the important thing for maintaining accessibility is to keep using the same bodc channel, or if you do switch channels then conda-forge would be preferable; if you are hoping to set up a new channel then please let us know so we can arrange another Artifactory mirror.

We also maintain a core environment of common packages (the Scientific Software Stack), so that scientists can all work with a shared experience. If you are picturing COAsT becoming a mainstream package in ocean science then we will likely get requests for COAsT to be included. For this we would need: publishing on the conda-forge channel, and a less restrictive Python requirement (needs to play nicely with a lot of other packages in the same environment).

That’s all. It’s great to have COAsT on Conda as this is the tool of choice here, and something scientists are becoming familiar with. A much smoother experience than some packages from other institutions that need installing from source!

@roje-bodc roje-bodc self-assigned this Nov 2, 2023
@roje-bodc
Copy link
Collaborator

pydap is no longer incompatible with python 3.10.
However following the update to 3.10, the version of xarray moves from 2023.1.0 to 2023.1.10, and this breaks some unit tests.

If we pin the version to 2023.1.0 then the unittests run as they did on 3.8.

@roje-bodc
Copy link
Collaborator

The error thrown is

Image

@roje-bodc
Copy link
Collaborator

roje-bodc commented Nov 3, 2023

@jpolton My branch is https://github.com/British-Oceanographic-Data-Centre/COAsT/tree/feature/python3.10-update .

How I have installed the package is by creating and activating an blank conda environment with python 3.10 installed and then running pip install . from the project root to install the package into it.

I also had to run conda install cartopy separately afterwards , since that package always causes me issues for some reason.

@jpolton
Copy link
Collaborator Author

jpolton commented Nov 3, 2023

@jpolton My branch is https://github.com/British-Oceanographic-Data-Centre/COAsT/tree/feature/python3.10-update .

How I have installed the package is by creating and activating an blank conda environment with python 3.10 installed and then running pip install . from the project root to install the package into it.

I also had to run conda install cartopy separately afterwards , since that package always causes me issues for some reason.

@roje-bodc I've pushed a fix back to your branch.

@roje-bodc
Copy link
Collaborator

@jpolton okay that's great. I've managed to run all the unit tests successfully on my local machine now :)

I do have one question about the plots in the tests though. The visual aspect of them occasionally causes issues on my machine (some weird threading issue).

Image

This goes away if you remove the visual part of the plotting in the test. Are the plot visuals important for the unit tests, or can we do without them? It also means there would be no user interaction required to close each plot when running the tests.
The issue doesn't always happen, but it is annoying when it does!

There is a fix mentioned here:
https://stackoverflow.com/questions/52839758/matplotlib-and-runtimeerror-main-thread-is-not-in-main-loop

@jpolton
Copy link
Collaborator Author

jpolton commented Nov 3, 2023

@jpolton okay that's great. I've managed to run all the unit tests successfully on my local machine now :)

I do have one question about the plots in the tests though. The visual aspect of them occasionally causes issues on my machine (some weird threading issue).

Image

This goes away if you remove the visual part of the plotting in the test. Are the plot visuals important for the unit tests, or can we do without them? It also means there would be no user interaction required to close each plot when running the tests. The issue doesn't always happen, but it is annoying when it does!

There is a fix mentioned here: https://stackoverflow.com/questions/52839758/matplotlib-and-runtimeerror-main-thread-is-not-in-main-loop

@roje-bodc I'm am not sure how to handle this plotting issue. As it happens the routine that caused the above problem shouldn't be making plots to screen but plots to file; I had hoped that this would remove the problem but clearly it didn't.
The solution you propose to specify the backend might be the thing to do. I have resisted this approach so far as my impression is that different IDEs and/or workflows seem to require different Matplotlib backends, so I didn't want to specify it in COAsT. My impression is also that backend requirements seem to be a thing in flux in Matplotlib so I'd be inclined to see how the dust settles and just persevere for now. On the flip side, turning off the tests that break is far from an ideal way to get "all" the tests to pass!

@roje-bodc
Copy link
Collaborator

@jpolton As it is an intermittent issue and not one that affects the packages actual functionality, we could progress this ticket without changing anything for matpotlib?

I could create a separate pull request with the change for altering the backend used within the unit tests, and then that can be merged if you decide to go that route? I've made the change locally to get the tests working consistently, so this won't be much effort.

@jpolton
Copy link
Collaborator Author

jpolton commented Nov 6, 2023

@jpolton As it is an intermittent issue and not one that affects the packages actual functionality, we could progress this ticket without changing anything for matpotlib?

I could create a separate pull request with the change for altering the backend used within the unit tests, and then that can be merged if you decide to go that route? I've made the change locally to get the tests working consistently, so this won't be much effort.

Yes. Let's do that. I think it requires a bit of group think to try and figure out who it is working for and who it is not working for.

@roje-bodc roje-bodc linked a pull request Nov 6, 2023 that will close this issue
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged to Develop
Development

Successfully merging a pull request may close this issue.

3 participants