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 numpy 2 in tests #997

Merged
merged 5 commits into from
Jul 31, 2024
Merged

Support numpy 2 in tests #997

merged 5 commits into from
Jul 31, 2024

Conversation

cbkerr
Copy link
Member

@cbkerr cbkerr commented Jun 20, 2024

Description

Apply changes from https://numpy.org/doc/stable//numpy_2_0_migration_guide.html#changes-to-namespaces

Motivation and Context

Checklist:

@cbkerr cbkerr requested review from a team as code owners June 20, 2024 17:22
@cbkerr cbkerr requested review from b-butler and klywang and removed request for a team June 20, 2024 17:22
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.01%. Comparing base (24035c6) to head (fd76019).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #997   +/-   ##
=======================================
  Coverage   86.01%   86.01%           
=======================================
  Files          20       20           
  Lines        3503     3503           
  Branches      770      770           
=======================================
  Hits         3013     3013           
  Misses        333      333           
  Partials      157      157           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bdice
Copy link
Member

bdice commented Jun 20, 2024

Test failures appear to be related to PyTables/PyTables#1172

It's fine with me if you want to add an exception to the warnings-as-errors options in pyproject.toml to ignore these. edit: Hmm, these appear to be actual errors and not warnings. I thought this was typically raised as a warning. Not sure.

@bdice
Copy link
Member

bdice commented Jun 20, 2024

Overall, our dependency on PyTables has been really hard to keep working. PyTables is usually the last of signac's dependencies to support new Python versions and such. I'd be okay with softening this dependency and not requiring it in our tests. Let me know if you want more specific input on that possibility.

@cbkerr
Copy link
Member Author

cbkerr commented Jun 21, 2024

I see PyTables has a PR for fixing this, so I vote we wait a couple weeks and check back

@joaander
Copy link
Member

joaander commented Jul 15, 2024

I see PyTables has a PR for fixing this, so I vote we wait a couple weeks and check back

The latest pull request on PyTables refactors the CI and specifically pins numpy < 2. The numpy 2 support PR has been closed and a new one has been created by an outside contributor. It appears that we will be waiting MANY weeks, if not months.

I suggest you update the signac tests to optionally skip pytables tests so that we can move forward.

@cbkerr
Copy link
Member Author

cbkerr commented Jul 30, 2024

The tests already have pandas and tables in a separate list requirements-test-optional.txt, and we see that only the "newest" tests fail. "Minimal" and "oldest" tests work, but those do not include numpy 2.0 either.

I considered making yet another list of requirements like requirements-test-optional-tables.txt, but I'm not sure how that affects building the wheels in the publish-packages github action:

    - name: Install dependencies
      run: |
        # We must explicitly install the requirements so that we can force
        # installation of the local wheel below in case the version conflicts
        # with published wheels (typically only possible during testing).
        python -m pip install \
        -r requirements.txt \
        -r requirements/requirements-test.txt \
        -r requirements/requirements-test-optional.txt

@joaander
Copy link
Member

The failure to import pytables is a catchable ValueErorr exception.

The simplest fix is to use

tables = pytest.importorskip("tables")

in place of

import tables

https://docs.pytest.org/en/6.2.x/reference.html#pytest.importorskip

However, it appears that exceptions other than ModuleNotFoundError will be considered errors in a future version of pytest: https://github.com/pytest-dev/pytest/blob/8509fe8be58ea1c093b4b0ac89382bfdacea0083/src/_pytest/outcomes.py#L288-L298

The more complex alternative is

try:
    import tables
    skip_tables = False
except ValueError:
    skip_tables = True

skip_tables = pytest.mark.skipif(skip_tables,
                              reason="The tables package is non-functional.")

Then, decorate individual test functions (and/or classes) that use tables with:

@skip_tables

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks!

@cbkerr cbkerr enabled auto-merge (squash) July 31, 2024 15:16
@cbkerr cbkerr merged commit 3da6261 into main Jul 31, 2024
16 checks passed
@cbkerr cbkerr deleted the fix/numpy-2 branch July 31, 2024 15:18
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