British-Oceanographic-Data-Centre / COAsT

A Coastal Ocean Assessment Tool built as an extendable python framework for nemo models
https://british-oceanographic-data-centre.github.io/COAsT/
MIT License
23 stars 11 forks source link

Update python=3.8 #625

Open jpolton opened 9 months ago

jpolton commented 9 months ago

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 commented 8 months ago

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

thogar-computer commented 8 months ago

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

thogar-computer commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

The error thrown is

Image

roje-bodc commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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.