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 MacOS CI workflow using mamba #1865

Merged
merged 25 commits into from
Dec 6, 2024
Merged

Conversation

dlakaplan
Copy link
Contributor

@dlakaplan dlakaplan commented Nov 27, 2024

This implements the full CI test on MacOS using mamba to ensure an x86 processor. It also removes the old MacOS tests from the testing matrix. Note that the new tests are not done through tox, because tox makes its own virtual environment, so they are just called directly. Hopefully this is sufficient. Otherwise more development (likely using tox-conda) would be needed.

There was additionally one test that failed repeatedly. As far as I could tell this was just due to random chance, although I am not sure. But I fixed it by putting in a fixed random seed.

@dlakaplan
Copy link
Contributor Author

@JPGlaser : I am trying to add a MacOS environment using micromamba like the one you did in https://github.com/ipta/pulsar-env/blob/patch/py-3.11/.github/workflows/test_CondaEnv.yml
To start I am using ubuntu, but will try to transition to x86 macOS.

@dlakaplan
Copy link
Contributor Author

OK, I think I got this working under macos-latest. It's not integrate with the matrix of other tests, but I can probably do that if people (@abhisrkckl , @paulray , @JPGlaser ) think that this is a useful direction.

@JPGlaser
Copy link

OK, I think I got this working under macos-latest. It's not integrate with the matrix of other tests, but I can probably do that if people (@abhisrkckl , @paulray , @JPGlaser ) think that this is a useful direction.

Looks good to me. It's much easier to control these kinds of tests and package deployments with the micromamba action.

~ Joe G.

@abhisrkckl
Copy link
Contributor

abhisrkckl commented Nov 28, 2024

I think the macos test using mamba is too much of a special case to belong in the matrix. It will be more readable if it's separate, IMO.

@dlakaplan
Copy link
Contributor Author

@JPGlaser : I thought I was running the precision tests here before, but I guess I wasn't. When I try to run the whole test suite it fails on the require_longdouble_precision check in conftest.py --- not sure why it wasn't called before. I thought I was using the x86 environment with --platform osx-64 but maybe not? Can you look this over and see if there are any obvious issues? Have you checked the precision explicitly like this elsewhere?

@dlakaplan
Copy link
Contributor Author

I added some more verbosity to the tests, and it looks OK:

os posix.uname_result(sysname='Darwin', nodename='Mac-1733160363517.local', release='23.6.0', version='Darwin Kernel Version 23.6.0: Thu Sep 12 23:34:00 PDT 2024; root:xnu-10063.141.1.701.1~1/RELEASE_ARM64_VMAPPLE', machine='x86_64')
processor i386
eps: 1.0842021724855044e-19

that value for np.longdouble eps is the same as I get on my M1 machine running rosetta. And it should be enough to pass the test in pint.utils.

@dlakaplan
Copy link
Contributor Author

And then when I add a check of the precision to the actual test, it gives:

2024-12-02 17:39:41.393 | DEBUG    | pint.utils:check_longdouble_precision:166 - longdouble eps: 2.220446049250313e-16

so somehow the right python/numpy is not being used/loaded here.

@dlakaplan
Copy link
Contributor Author

Yes, somehow within the tox environment I see:

024-12-02 17:46:03.808 | DEBUG    | pint.utils:check_longdouble_precision:166 - os posix.uname_result(sysname='Darwin', nodename='Mac-1733160850129.local', release='23.6.0', version='Darwin Kernel Version 23.6.0: Thu Sep 12 23:34:00 PDT 2024; root:xnu-10063.141.1.701.1~1/RELEASE_ARM64_VMAPPLE', machine='arm64')
2024-12-02 17:46:03.832 | DEBUG    | pint.utils:check_longdouble_precision:167 - processor i386
2024-12-02 17:46:03.832 | DEBUG    | pint.utils:check_longdouble_precision:168 - Python 3.13.0 (main, Oct  7 2024, 05:02:14) [Clang 16.0.0 (clang-1600.0.26.4)]
2024-12-02 17:46:03.832 | DEBUG    | pint.utils:check_longdouble_precision:169 - eps: 2.220446049250313e-16
2024-12-02 17:46:03.832 | DEBUG    | pint.utils:check_longdouble_precision:170 - longdouble eps: 2.220446049250313e-16
2024-12-02 17:46:04.021 | DEBUG    | pint.utils:check_longdouble_precision:171 - Info: # Created: 2024-12-02T17:46:03.834905
# PINT_version: 0+untagged.1.g3293987
# User: runner
# Host: Mac-1733160850129.local
# OS: macOS-14.7.1-arm64-i386-64bit-Mach-O
# Python: 3.13.0 (main, Oct  7 2024, 05:02:14) [Clang 16.0.0 (clang-1600.0.26.4)]

when it should be python 3.11. So it's not actually loading within the mamba environment.

@dlakaplan
Copy link
Contributor Author

For some reason the CI has failed several times on:
https://github.com/nanograv/PINT/blob/master/tests/test_phase_offset.py#L38
in the new MacOS tests. When I run only that test in CI it passes. When I run it locally it (usually) passes: about 1/100 fails. Not sure of why it is not passing here. Will see if adding an explicit random seed makes a difference.

@dlakaplan dlakaplan changed the title WIP: Add CI workflow using mamba Add MacOS CI workflow using mamba Dec 4, 2024
@dlakaplan dlakaplan added the awaiting review This PR needs someone to review it so it can be merged label Dec 4, 2024
.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
.github/workflows/ci_test.yml Outdated Show resolved Hide resolved
@abhisrkckl
Copy link
Contributor

Also please update changelog.

@dlakaplan
Copy link
Contributor Author

I'm not sure what the original requirement was for, but we had included typed_ast in requirements_dev. The build failed for me with macos-latest and python 3.13. But I also see:
python/typed_ast#179
saying that it was primarily for python 2 and <=3.8, so I'm trying to see if we can remove it entirely, or potentially replace with ast if needed.

@abhisrkckl
Copy link
Contributor

This looks good. Shall I merge this?

@dlakaplan
Copy link
Contributor Author

I think this is ready, but would appreciate a look-through from @JPGlaser

@JPGlaser
Copy link

JPGlaser commented Dec 6, 2024

Yep, everything looks good and things seem to be operating correctly on the tests. I'm good with it getting merged!

@abhisrkckl abhisrkckl merged commit f3362ef into nanograv:master Dec 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants