-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
TST: Make ARM build work (not in the CI) #41739
Conversation
ci/setup_env.sh
Outdated
if [[ "$UNAME_OS" == 'Linux' ]]; then | ||
DEFAULT_CONDA_URL="https://repo.continuum.io/miniconda/Miniconda3-latest" | ||
if [[ "$(uname -m)" == 'aarch64' ]]; then | ||
CONDA_URL="https://github.com/conda-forge/miniforge/releases/download/4.8.5-1/Miniforge3-4.8.5-1-Linux-aarch64.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use the latest version https://github.com/conda-forge/miniforge/releases/download/4.10.1-4/Miniforge3-4.10.1-4-Linux-aarch64.sh
any idea why this is not showing up on the checks? |
The configuration file for CircleCI needs to be in master before it appears. I can add this in two steps. A hello world CI first that activates CircleCI, and this one later, so we can see it running before merging. |
sure that would be great |
@datapythonista Are you sure we'll have enough credits for this? Looking at the plan, we get 2500 credits/week, which would be 250 minutes on Arm? Assuming a build takes 40 min, that would only be ~ 6 builds before we run out. |
Thanks for making the numbers. I didn't check in detail, but I assumed with the volume of pandas the plan credits could not be enough. But since CircleCI seems to be the only option, I think it's worth getting started, and then if we need to move to nightly builds or something, it'd be better than not having anything, as we've got now. I hope we have at least enough credits to run the tests once per day. Not as good as running on PRs, but I think that would be good enough. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
nightly builds should be happening on python-wheels. but is disabled to save credits for the releases and other projects. If we do nightly builds, shouldn't this be on the wheels repo. |
and because the job was rebuilding the release version and not from master MacPython/pandas-wheels#102 |
Ok, since this is now green, and there is no obvious way to move forward with having ARM builds, I think the best is to remove the CircleCI configuration in this PR. We can still merge the rest of changes, and have ARM passing as of today. And if a solution is found to automatically test on ARM, whether it's at each PR, nightly, or whenever, things should be easier to add with the xfails and other changes here. Does it sound good? |
I've not looked in detail, but maybe the xfails should not be strict. IIUC the latest arm build for python 3.8 and 3.9 on pandas-wheels passed |
we have arm builds for conda for both mac (azure) and linux (drone). (IIUC no testing only build package for linux on drone), we have arm builds for PyPI for linux only (travis) (which run tests and fail to upload wheels if tests fail) from #33971 (comment)
we need some testing on ci and consistency between pandas ci, conda and pip |
@simonjayhawkins we don't have any arm builds drone doesn't work for us and azure is emulated and very slow yes actual wheels are built but w/o CI the only thing u can do us build the wheels and ignore the tests (not great but no option) |
Our basic problem is that the tests are failing on Travis in pandas-wheels on release and no nightly builds in pandas-wheels. The credits issue is part of the problem, but not having visibility of failing tests (on pandas ci and nightly builds) is probably more of an issue. If we cannot solve this issue with the limited credits, then we should also consider moving the pandas-wheels builds to CircleCI for consistency. |
ignore the arm tests entirely in pandas-wheels we likely cannot move the wheel builds easily |
that's also an option |
if we ignore the tests in pandas-wheels, this won't be an issue. |
I contacted CircleCI to check if they could be interested in sponsoring our ARM builds. Will update if I hear from them. For now looks like we're getting the 2,500 credits that @lithomas1 mentioned (not the 400,000 for open source projects for Linux builds). So, not much we can do with those. I updated this PR so we don't add the CircleCI build, but still, it has everything else so the build passes when it can be added wherever. Only thing is that we now need to disable Circle CI, so the build doesn't fail after we remove the configuration. But only an admin of the pandas-dev organization can do it. @jreback do you mind going to https://app.circleci.com/settings/project/github/pandas-dev/pandas?return-to=https%3A%2F%2Fapp.circleci.com%2Fpipelines%2Fgithub%2Fpandas-dev%2Fpandas%2F30%2Fworkflows%2Ff9a6db17-510a-4eaf-a10c-b3a800e0a6c4%2Fjobs%2F30872 and clickig on "Stop builds" please? |
ok did that |
thanks @datapythonista cc @simonjayhawkins maybe could backport to skip these tests on arm |
sure. IIUC we want to update pandas-wheels, #41739 (comment) @meeseeksdev backport 1.2.x |
This comment has been minimized.
This comment has been minimized.
@@ -92,6 +92,18 @@ def is_platform_mac() -> bool: | |||
return sys.platform == "darwin" | |||
|
|||
|
|||
def is_platform_arm() -> bool: | |||
""" | |||
Checking if he running platform use ARM architecture. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo for follow-up
@@ -750,7 +752,7 @@ def test_to_numeric_from_nullable_string(values, nullable_string_dtype, expected | |||
"UInt64", | |||
"signed", | |||
"UInt64", | |||
marks=pytest.mark.xfail(reason="GH38798"), | |||
marks=pytest.mark.xfail(not is_platform_arm(), reason="GH38798"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test added in #38746, not on 1.2.x
@@ -168,6 +170,7 @@ def test_subtype_integer_with_non_integer_borders(self, subtype): | |||
) | |||
tm.assert_index_equal(result, expected) | |||
|
|||
@pytest.mark.xfail(is_platform_arm(), reason="GH 41740") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is already xfailed on 1.2.x, xref #38719
Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
@@ -1072,6 +1073,7 @@ def test_rolling_sem(frame_or_series): | |||
tm.assert_series_equal(result, expected) | |||
|
|||
|
|||
@pytest.mark.xfail(is_platform_arm(), reason="GH 41740") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only fails on ARM64 Linux. It passes on M1 Mac. #38921
This seems to be working, see:
https://app.circleci.com/pipelines/github/datapythonista/pandas/11/workflows/2ca0487f-5a63-4f01-936c-e83827db714f/jobs/11
But tests are failing. I opened #41740 the the failures.