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

allow numpy 2.0 to run #4829

Merged
merged 9 commits into from
Sep 6, 2024
Merged

allow numpy 2.0 to run #4829

merged 9 commits into from
Sep 6, 2024

Conversation

ahnitz
Copy link
Member

@ahnitz ahnitz commented Jul 30, 2024

The previous PR #4716 allowed python 3.12 tests to run and for that we needed to build against numpy2.0. However, we had not lifted the restriction to require older numpy versions during installation (e.g. after the build). This was originally due to potential incompatibilities and upstream library support.

I am opening this now to see where things stand and if tests look ok, we should lift the restriction or being addressing numpy 2.0 related issues.

@ahnitz
Copy link
Member Author

ahnitz commented Jul 30, 2024

List of packages upstream that need to be rebuilt against numpy 2.0 before we can enable support.

  • lalsuite
  • ????

@lpsinger
Copy link
Contributor

The latest versions of LALSuite support Numpy 1.x and 2.x. Would you please retry this?

@lpsinger
Copy link
Contributor

It's failing to install pypmc from source, but it's trying to build an old version when there are newer ones with wheels available.

@ahnitz
Copy link
Member Author

ahnitz commented Sep 4, 2024

Rerunning after rebasing from #4863, If this works, then this can be merged afterwards.

@ahnitz
Copy link
Member Author

ahnitz commented Sep 4, 2024

@spxiwh Ok, I think this may now be working. There are a couple fixes that were needed. I'll draw attention to the following issue that appeared

We had a few places where we did something like

a = numpy.array(b, copy=False).

In some cases the type of b might be a list or something that actually has to be copied. In older numpy versions it would silently copy it and continue on. In numpy 2.0 this is properly enforced so a few errors were popping up in the tests.

@ahnitz
Copy link
Member Author

ahnitz commented Sep 5, 2024

@spxiwh I don't see anything obvious in the templatebank workflow logs. Do you have any insight? I've tried to reproduce offline, but it works locally for me. I haven't worked through all he variables yet though, so maybe there is a more subtle problem.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 5, 2024

@ahnitz I see two failures, the search workflow and the template bank workflow. The search workflow is failing with:

        Traceback (most recent call last):
          File "/opt/hostedtoolcache/Python/3.10.14/x64/bin/pycbc_coinc_findtrigs", line 491, in <module>
            ldatas = list(map(process_template, list(template_ids)))
          File "/opt/hostedtoolcache/Python/3.10.14/x64/bin/pycbc_coinc_findtrigs", line 380, in process_template
            cstat = rank_method.rank_stat_coinc(
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pycbc/events/stat.py", line 1783, in rank_stat_coinc
            logr_s = self.logsignalrate(stat, slide * step, to_shift)
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pycbc/events/stat.py", line 711, in logsignalrate
            logsignalrateinternals_compute2detrate(
          File "pycbc/events/eventmgr_cython.pyx", line 95, in pycbc.events.eventmgr_cython.logsignalrateinternals_compute2detrate
        ValueError: Buffer dtype mismatch, expected 'float' but got 'double'

but only for one of the pycbc_coinc_findtrigs jobs. I wonder if this is a "no triggers = wrong dtpye" type error, but why is it getting no triggers, if so?

The template bank workflow failed with:

        A module that was compiled using NumPy 1.x cannot be run in
        NumPy 2.1.1 as it may crash. To support both 1.x and 2.x
        versions of NumPy, modules must be compiled with NumPy 2.0.
        Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

        If you are a user of the module, the easiest solution will be to
        downgrade to 'numpy<2' or try to upgrade the affected module.
        We expect that some modules will need time to support NumPy 2.

        Traceback (most recent call last):  File "/opt/hostedtoolcache/Python/3.10.14/x64/bin/sbank", line 41, in <module>
            from sbank.bank import Bank
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/sbank/bank.py", line 28, in <module>
            from .overlap import SBankWorkspaceCache
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/sbank/overlap.py", line 8, in <module>
            from .overlap_cpu import SBankCythonComputeMatch
        Traceback (most recent call last):
          File "/opt/hostedtoolcache/Python/3.10.14/x64/bin/sbank", line 41, in <module>
            from sbank.bank import Bank
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/sbank/bank.py", line 28, in <module>
            from .overlap import SBankWorkspaceCache
          File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/sbank/overlap.py", line 8, in <module>
            from .overlap_cpu import SBankCythonComputeMatch
          File "sbank/overlap_cpu.pyx", line 1, in init sbank.overlap_cpu
        ImportError: numpy.core.multiarray failed to import (auto-generated because you didn't call 'numpy.import_array()' after cimporting numpy; use '<void>numpy._import_array' to disable if you are certain you don't need it).

I'm guessing this is an issue in the sbank wheel (is it expected that we need all dependencies compiled against numpy >=2?). Do I need to make a new release of sbank with wheels built against numpy >= 2?

@lpsinger
Copy link
Contributor

lpsinger commented Sep 5, 2024

Do I need to make a new release of sbank with wheels built against numpy >= 2?

Yes. If you build against Numpy >= 2, then the wheel will be compatible with Numpy 1.x and 2.x.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 5, 2024

Thanks @lpsinger. I'll look at this within sbank.

@ahnitz
Copy link
Member Author

ahnitz commented Sep 5, 2024

@spxiwh I think I understand the search issues and will work through those. If you can do a new sbank release that would be good. As Leo said, you just want them built against numpy >= 2.0, you don't need to change the actual install requirements -> https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice

@spxiwh
Copy link
Contributor

spxiwh commented Sep 5, 2024

I'm struggling with the sbank build. It looks like the previous packaging stuff (https://github.com/gwastro/sbank/blob/master/.github/workflows/packaging.yml) was using EL7, which now seems to not work at all. Updating this to EL8 is not working, and I don't really understand why yet.

@spxiwh
Copy link
Contributor

spxiwh commented Sep 5, 2024

Okay, after some help from @duncanmmacleod, a new sbank release has been made with wheels built against numpy 2 (for python > 3.8). The template bank tests are now passing, so over to @ahnitz for the last bit.

@ahnitz
Copy link
Member Author

ahnitz commented Sep 5, 2024

@spxiwh I think we are goo to go now. All the tests pass.

@ahnitz ahnitz enabled auto-merge (squash) September 5, 2024 22:44
Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ahnitz ahnitz merged commit 8363708 into gwastro:master Sep 6, 2024
30 checks passed
prayush pushed a commit to prayush/pycbc that referenced this pull request Nov 21, 2024
* allow numpy 2.0 to run

* fix for np2

* fix np2 bug

* tmpltbank fixes

* fixes

* fix string issue

* be explicit about types in cases where the input might use type prediction

* this place too

* cc
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.

3 participants