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

Add support for python 3.12 #626

Merged
merged 29 commits into from
Feb 22, 2024
Merged

Add support for python 3.12 #626

merged 29 commits into from
Feb 22, 2024

Conversation

h-mayorquin
Copy link
Contributor

As in the title. Fingers cross is this easy.

@h-mayorquin h-mayorquin self-assigned this Oct 31, 2023
@CodyCBakerPhD
Copy link
Member

@h-mayorquin I heard that GitHub added Mac silicon runners, too, would you be able to add that (if it's true?)

@h-mayorquin
Copy link
Contributor Author

@CodyCBakerPhD Did a quick google but it seems that Mac is a paid service. Anyway, I think we should do outside of this future.

Back to the PR, aiohttp is failing. It is related to the long comming deprecation of disutils. It seems that it will be fixed soon in aiohttp side though.

But unless we want to pin to the pre-release:
aio-libs/aiohttp#7639 (comment)

We will need to wait a bit for their latest wheels:
aio-libs/aiohttp#7675

I am not sure wher this dependency is coming from.

@CodyCBakerPhD
Copy link
Member

I'm OK to wait a bit on this then, thanks for digging into it

I am not sure wher this dependency is coming from.

It's from the NWB Inspector: https://github.com/NeurodataWithoutBorders/nwbinspector/blob/dev/requirements.txt#L5

But it would be needed and used by any environment capable of streaming data from DANDI using fsspec

@h-mayorquin
Copy link
Contributor Author

OK, this will take a while. python 3.12 is supported in the latest version of numpy but some of the packages that we install have put an upper bound on numpy. Probably should put to rest until we consolidate dependencies so this can be figured out without parsing the dependency tree.

@bendichter
Copy link
Contributor

It looks like numba, which is required by SpikeInterface, is causing problems, as they still have not pypi released a version that supports py3.12,

@h-mayorquin
Copy link
Contributor Author

Seems that numba intends to get this sorted soon.
numba/numba#9197

Python 3.12 was a special jump for a lot of scientific software because distutils was droped. Here is an interesting article from a couple of months that describes some of the problems that I guess Numba is still facing:
https://labs.quansight.org/blog/building-scipy-with-flang

@h-mayorquin
Copy link
Contributor Author

Numba is done, next blocker is hdmf-zarr:

hdmf-dev/hdmf-zarr#162

@bendichter
Copy link
Contributor

bendichter commented Feb 21, 2024

edit: nevermind, numcodecs is still giving us trouble

With hdmf-dev/hdmf-zarr#162 closed we are unblocked!

@h-mayorquin
Copy link
Contributor Author

edit: nevermind, numcodecs is still giving us trouble

With hdmf-dev/hdmf-zarr#162 closed we are unblocked!

I thought that it was solved with:
hdmf-dev/hdmf-zarr#162

I did not see that causing problems after, where are you seeing the problems? The problem that I saw after is that there is some problem with the edf depedency:

pyedflib>=0.1.30
numpy<1.25.0;python_version>="3.11"

Because we have an upper bound for numpy but python 3.12 requires numpy to be larger than that. I tried some quick fixes but it did not work out and did not have time to dig further.

@CodyCBakerPhD
Copy link
Member

I did not see that causing problems after, where are you seeing the problems?

Different problems per platform

Ubuntu & Windows, minor test case that needs updating due to a slight error type change: https://github.com/catalystneuro/neuroconv/actions/runs/7987728897/job/21810807661
Mac, numcodecs build issue: https://github.com/catalystneuro/neuroconv/actions/runs/7987728897/job/21810808203

@h-mayorquin
Copy link
Contributor Author

@CodyCBakerPhD Thanks. I did not checked all the CI actions. So this is a Mac problem. I don't know why the Mac runner is trying to build a wheel for numcodecs, there are wheels available:
https://pypi.org/project/numcodecs/#files

@h-mayorquin
Copy link
Contributor Author

Also, to add to our discussion about python EOL, zarr already dropped python 3.8 and numpy smaller than 1.20 already:
zarr-developers/zarr-python#1557

@CodyCBakerPhD
Copy link
Member

Ubuntu and Windows passing on 3.12!

Now just the Mac numcodecs thing https://github.com/catalystneuro/neuroconv/actions/runs/7994819629/job/21833777077?pr=626

Copy link
Member

Choose a reason for hiding this comment

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

Heh, well this is an interesting one

I'm not quite sure what to do with the MaxOne interface or MaxTwo #221 because various platforms on the CI have always had trouble getting the custom HDF5 plugin to work properly to decompress data contained in the files

I had gotten the tests and functionality to work on local devices, so it is/was somewhat functional in practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean, what are the options here?

Copy link
Member

Choose a reason for hiding this comment

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

(a) Keep Maxwell interfaces around despite no CI tests

(b) Deprecate and remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add?
h5py<=3.9.0; python_version<='3.12'

Aren't they still tested on ubuntu for 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.

Aren't they still tested on ubuntu for python <3.12?

I even had to exclude ubuntu a while back

https://github.com/catalystneuro/neuroconv/blob/main/tests/test_on_data/test_recording_interfaces.py#L187

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.

My vote: I say let's keep it and we pin down the correct CI testing when and if we get a job / contract related to it.

Meanwhile, probably not worth figuring it out at this very moment.

@h-mayorquin
Copy link
Contributor Author

This seems ready @CodyCBakerPhD and @bendichter

Thanks @CodyCBakerPhD for taking the last mile of this one : )

@CodyCBakerPhD
Copy link
Member

Any idea what that issue was for mac and numcodecs?

@h-mayorquin
Copy link
Contributor Author

It seems to me that some configuration of the full dependencies was making pip not pick the wheel for Mac.

@bendichter
Copy link
Contributor

This looks great!

@bendichter bendichter self-requested a review February 21, 2024 22:28
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (22318d9) 92.60% compared to head (8e60912) 92.63%.
Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
+ Coverage   92.60%   92.63%   +0.02%     
==========================================
  Files         115      115              
  Lines        6221     6216       -5     
==========================================
- Hits         5761     5758       -3     
+ Misses        460      458       -2     
Flag Coverage Δ
unittests 92.63% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...tainterfaces/ecephys/spike2/spike2datainterface.py 55.17% <ø> (ø)

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) February 21, 2024 23:56
@CodyCBakerPhD CodyCBakerPhD merged commit 16de46b into main Feb 22, 2024
27 of 43 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the add_support_to_python_312 branch February 22, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants