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 abacusutils to downstream #1845

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Oct 9, 2024

This PR adds https://github.com/abacusorg/abacusutils to our downstream tests.

The tox environment was largely modeled on the test setup in the downstream repo:
https://github.com/abacusorg/abacusutils/blob/6e18f79fc31aea7973abbb144b78d049a2452eeb/.github/workflows/tests.yml#L84
as there are a few "difficult to install" dependencies.

The tests are currently passing but do show some warnings related to asdf:

   /home/runner/work/asdf/asdf/asdf/_asdf.py:189: AsdfWarning: copy_arrays is deprecated; use memmap instead. Note that memmap will default to False in asdf 4.0.

which are expected following the deprecation of copy_arrays #1797

Checklist:

  • pre-commit checks ran successfully

  • tests ran successfully

  • for a public change, added a towncrier news fragment

    changes/<PR#>.<changetype>.rst

    • changes/<PR#>.feature.rst: new feature
    • changes/<PR#>.bugfix.rst: bug fix
    • changes/<PR#>.doc.rst: documentation change
    • changes/<PR#>.removal.rst: deprecation or removal of public API
    • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • for a public change, updated documentation

  • for any new features, unit tests were added

@braingram braingram force-pushed the abacusutils_downstream branch from 3b2207f to 9762d28 Compare October 9, 2024 17:16
@braingram braingram marked this pull request as ready for review October 9, 2024 17:42
@braingram braingram requested a review from a team as a code owner October 9, 2024 17:42
@braingram
Copy link
Contributor Author

@lgarrison if you have a chance to take a look at this PR that would be great! Let me know if there are different tests that might be useful (and hopefully straightforward) to run as part of asdf's downstream testing. Our downstream tests run weekly, prior to release and on PRs that we expect might cause downstream warnings and failures.

So far I only see 1 warning (mentioned in the description above) due to the deprecation of copy_arrays.

pip install --no-build-isolation classy corrfunc
pip install -e abacusutils[test]
pip install -r {env_tmp_dir}/requirements.txt
bash -c "echo '' > pytest.ini"
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 had to make a blank "pytest.ini" here to prevent pytest from searching the parent directory and finding the asdf pytest configuration (in pyproject.toml in the asdf checkout). I think this should still work if abacusutils adds a pytest configuration (since that should be found before the blank pytest.ini created here).

@lgarrison
Copy link
Contributor

Cool, thanks for putting this together! abacusutils does a lot of things, many of which don't use asdf, so I think it might make sense to use a restricted set of tests so it runs faster (and maybe more reliably) in CI. I would suggest:

pytest abacusutils/tests/test_data.py

I think that doesn't need any of the source-only dependencies. All that should be needed is:

pip install abacusutils pytest

Can you try that? I don't think any of these paths/dependencies are likely to change in the near future.

@braingram
Copy link
Contributor Author

Cool, thanks for putting this together! abacusutils does a lot of things, many of which don't use asdf, so I think it might make sense to use a restricted set of tests so it runs faster (and maybe more reliably) in CI. I would suggest:

pytest abacusutils/tests/test_data.py

I think that doesn't need any of the source-only dependencies. All that should be needed is:

pip install abacusutils pytest

Can you try that? I don't think any of these paths/dependencies are likely to change in the near future.

Thanks! I gave this a try and it ran into a few issues that I think are related to numpy 2.
https://github.com/asdf-format/asdf/actions/runs/11262583334/job/31318601800?pr=1845#step:10:864
It installed abacusutils from pypi but then ran the tests from source. I did this because I'm not seeing the tests in the packaged wheel. Do the tests get packaged with the wheel?
The failures with this mode (package from pypi, tests from source) at a glance look to be numpy 2.0 issues which are likely fixed on main and in the tests but not yet on pypi.

I'm going to try switching back to installing from source (which I think was working) and then running just the tests you mentioned to see what that does.

Also I tried to run this locally on a mac (while working on the change) and got a number of errors since os.sched_getaffinity is only available on unix. Let me know if it's helpful to open an abacusutils issue for that (it shouldn't cause an issue here since the CI runs in linux).

@braingram braingram force-pushed the abacusutils_downstream branch from e6cbb77 to 1683cb4 Compare October 9, 2024 20:40
@lgarrison
Copy link
Contributor

Oh that's interesting, a recent PR I made seems to have had the side effect of fixing NumPy 2 installs. I don't even think I noticed that NumPy 2 was broken because we were testing against NumPy 1 for Classy! I think a source install from GitHub (e.g. pip install -e ./abacusutils pytest) is the way to go; we do try to keep the main branch functional, so testing against it hopefully won't cause any problems.

We haven't been packaging the tests in the wheels partly because they rely on data files that inflate the wheel size. I'm happy to revisit that in the future, especially if you prefer to test against released versions.

Last time I checked, the tests on Mac were failing in a non-trivial way (abacusorg/abacusutils#59). I don't have a Mac, so I didn't get very far in debugging it, unfortunately.

@braingram braingram force-pushed the abacusutils_downstream branch from 1683cb4 to 7db6470 Compare October 9, 2024 21:46
@braingram braingram merged commit a4e7961 into asdf-format:main Oct 11, 2024
51 checks passed
@braingram braingram deleted the abacusutils_downstream branch October 11, 2024 15:10
@lgarrison
Copy link
Contributor

For the future, I think this:

git clone https://github.com/abacusorg/abacusutils.git
pip install -vU setuptools wheel scipy Cython 'numpy<2'  # for classy and corrfunc
pip install --no-build-isolation classy corrfunc
pip install -e abacusutils[test]

could be simplified to:

git clone https://github.com/abacusorg/abacusutils.git
pip install -e ./abacusutils pytest

Since the tests are working, it's fine to leave it as-is, but Corrfunc and classy can take a while to build/can be fragile, so I just wanted to note this possible simplification.

@braingram braingram mentioned this pull request Dec 20, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants