conda-forge / pyproj-feedstock

A conda-smithy repository for pyproj.
BSD 3-Clause "New" or "Revised" License
2 stars 15 forks source link

Enable ppc64le but disable PyPy for now #144

Closed zklaus closed 8 months ago

zklaus commented 8 months ago

We have fixed ppc64le in proj by building with newer compilers. Tests are currently failing with PyPy and we will investigate in a follow-up build.

Checklist

Fixes #139.

conda-forge-webservices[bot] commented 8 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

dopplershift commented 8 months ago

I think fixing this is necessary to unblock cartopy (and pygrib) on the Python 3.12 migration (unless we drop ppc64le over there).

zklaus commented 8 months ago

Agreed. In fact, that's why I picked it up. However, there seems to be a genuine bug here somewhere, which I can't figure out right now, so anyone who wants, feel free to tackle it without waiting for me.

djhoese commented 8 months ago

So for the record, I see one test failure in the CI job I looked at:

=========================== short test summary info ============================
FAILED test/test_transform.py::test_transform_single_point_nad83_to_nad27 - AssertionError: 
Arrays are not almost equal to 0 decimals

Mismatched elements: 2 / 2 (100%)
Max absolute difference: 4268817.53503948
Max relative difference: 1.00000083
 x: array([ 6.e+05, -4.e+00])
 y: array([ 569722, 4268814])
====== 1 failed, 995 passed, 8 skipped, 185 warnings in 346.69s (0:05:46) ======

So it isn't like a precision error, it's just completely wrong. CC @snowman to make sure this isn't something he's seen upstream. Technically I'm a maintainer of pyproj now too, but I haven't looked at this test specifically so I'm hoping there's some historic reasoning to this.

djhoese commented 8 months ago

Ah:

@pytest.mark.xfail(PROJ_911, reason="Patch applied in conda-forge changes behavior")
def test_transform_single_point_nad83_to_nad27():
snowman2 commented 8 months ago

We don't test ppc64le in the pyproj tests, so the pipelines here will need to be used. We didn't dig into it previously and just skipped the tests, so there isn't much to go on at the moment.

djhoese commented 8 months ago

It took me longer than I'd like to admit, but I think I've got ppc64le builds running locally on my PopOS/Ubuntu laptop through qemu. At first I tried getting a "normal" qemu machine running an Ubuntu image, but couldn't get anything to work. Using conda-forge's images and/or running the build locally scripts in the feedstock seem to be working. It just takes a while to do an emulated build on my machine.

Edit: Hopefully this leads to some answers if not just another data point.

djhoese commented 8 months ago

Error reproduced locally and same exact error with PROJ 9.2.1 too:

args = (<function assert_array_almost_equal.<locals>.compare at 0x40be5580d0>, (589057.5710680408, -3.5350394839365156), (569722, 4268814))
kwds = {'err_msg': '', 'header': 'Arrays are not almost equal to 0 decimals', 'precision': 0, 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError:
E           Arrays are not almost equal to 0 decimals
E
E           Mismatched elements: 2 / 2 (100%)
E           Max absolute difference: 4268817.53503948
E           Max relative difference: 1.00000083
E            x: array([ 6.e+05, -4.e+00])
E            y: array([ 569722, 4268814])
    proj:                  9.2.1-h19dfee3_0               conda-forge

So I know @dopplershift tried a PROJ feedstock PR with an equivalent proj command sequence that should reproduce this. @snowman2 does this look right to you:

echo -92.199881 38.56694 | proj EPSG:26915 | cs2cs EPSG:26915 +to EPSG:26715

to reproduce:

    # projection 1: UTM zone 15, grs80 ellipse, NAD83 datum
    # (defined by epsg code 26915)
    p1 = Proj("epsg:26915", preserve_units=False)
    # projection 2: UTM zone 15, clrk66 ellipse, NAD27 datum
    p2 = Proj("epsg:26715", preserve_units=False)
    # find x,y of Jefferson City, MO.
    x1, y1 = p1(-92.199881, 38.56694)
    # transform this point to projection 2 coordinates.
    x2, y2 = transform(p1, p2, x1, y1)

Or are there other options/commands that should be used to reproduce it with proj?

djhoese commented 8 months ago

Ok, this is weird, adding the proj command before the pytest command makes things pass:

$ git diff
diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index 0b9a624..7b3d0c1 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -50,7 +50,8 @@ test:
     - export "PYPROJ_GLOBAL_CONTEXT=ON"  # [unix]
     - set "PROJ_NETWORK=ON"              # [win]
     - set "PYPROJ_GLOBAL_CONTEXT=ON"     # [win]
-    - python -m pytest -v -s
+    - echo -92.199881 38.56694 | proj EPSG:26915 | cs2cs EPSG:26915 +to EPSG:26715
+    - python -m pytest -v -s test/test_transform.py::test_transform_single_point_nad83_to_nad27
 about:
   home: https://github.com/pyproj4/pyproj
   license: MIT
Passing tests ``` + export PROJ_NETWORK=ON + PROJ_NETWORK=ON + export PYPROJ_GLOBAL_CONTEXT=ON + PYPROJ_GLOBAL_CONTEXT=ON + echo -92.199881 38.56694 + proj EPSG:26915 + cs2cs EPSG:26915 +to EPSG:26715 589057.58 -3.54 0.00 + python -m pytest -v -s test/test_transform.py::test_transform_single_point_nad83_to_nad27 ============================= test session starts ============================== platform linux -- Python 3.10.12, pytest-7.4.2, pluggy-1.3.0 -- $PREFIX/bin/python cachedir: .pytest_cache rootdir: $SRC_DIR collecting ... collected 1 item test/test_transform.py::test_transform_single_point_nad83_to_nad27 PASSED =============================== warnings summary =============================== test/test_transform.py::test_transform_single_point_nad83_to_nad27 $SRC_DIR/test/test_transform.py:65: FutureWarning: This function is deprecated. See: https://pyproj4.github.io/pyproj/stable/gotchas.html#upgrading-to-pyproj-2-from-pyproj-1 x2, y2 = transform(p1, p2, x1, y1) -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ========================= 1 passed, 1 warning in 1.88s ========================= + exit 0 ```

Edit: Doing just proj causes the pyproj test to still fail.

djhoese commented 8 months ago

With just the echo | proj portion of the above mentioned command, the pyproj test still fails. With just an echo and the cs2cs command the pyproj test passes BUT the cs2cs command gives the wrong answer:

+ export PROJ_NETWORK=ON
+ PROJ_NETWORK=ON
+ export PYPROJ_GLOBAL_CONTEXT=ON
+ PYPROJ_GLOBAL_CONTEXT=ON
+ echo 569704.57 4269024.67
+ cs2cs EPSG:26915 +to EPSG:26715
589057.58       -3.54 0.00
+ python -m pytest -v -s test/test_transform.py::test_transform_single_point_nad83_to_nad27
============================= test session starts ==============================
platform linux -- Python 3.10.12, pytest-7.4.2, pluggy-1.3.0 -- $PREFIX/bin/python
cachedir: .pytest_cache
rootdir: $SRC_DIR
collecting ... collected 1 item

test/test_transform.py::test_transform_single_point_nad83_to_nad27 PASSED

=============================== warnings summary ===============================
test/test_transform.py::test_transform_single_point_nad83_to_nad27
  $SRC_DIR/test/test_transform.py:65: FutureWarning: This function is deprecated. See: https://pyproj4.github.io/pyproj/stable/gotchas.html#upgrading-to-pyproj-2-from-pyproj-1
    x2, y2 = transform(p1, p2, x1, y1)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================= 1 passed, 1 warning in 2.04s =========================
+ exit 0

Here is me running it on my local environment (not ppc):

$ echo 569704.57 4269024.67 | cs2cs EPSG:26915 +to EPSG:26715
569722.40       4268814.27 0.00
$ echo -92.199881 38.56694 | proj EPSG:26915 | cs2cs EPSG:26915 +to EPSG:26715
569722.40       4268814.27 0.00
djhoese commented 8 months ago

Sorry for all the messages, but one more data point. I changed the test commands to:

$ git diff
diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index 0b9a624..537c138 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -50,7 +50,14 @@ test:
     - export "PYPROJ_GLOBAL_CONTEXT=ON"  # [unix]
     - set "PROJ_NETWORK=ON"              # [win]
     - set "PYPROJ_GLOBAL_CONTEXT=ON"     # [win]
-    - python -m pytest -v -s
+    #- echo -92.199881 38.56694 | proj EPSG:26915 | cs2cs EPSG:26915 +to EPSG:26715
+    #- echo -92.199881 38.56694 | proj EPSG:26915
+    - echo 569704.57 4269024.67 | cs2cs EPSG:26915 +to EPSG:26715
+    - echo 569704.57 4269024.67 | cs2cs EPSG:26915 +to EPSG:26715
+    - echo -92.199881 38.56694 | proj EPSG:26915 | cs2cs EPSG:26915 +to EPSG:26715
+    - echo 569704.57 4269024.67 | cs2cs EPSG:26915 +to EPSG:26715
+    #- proj
+    - python -m pytest -v -s test/test_transform.py::test_transform_single_point_nad83_to_nad27
 about:
   home: https://github.com/pyproj4/pyproj
   license: MIT

And got:

+ export PROJ_NETWORK=ON
+ PROJ_NETWORK=ON
+ export PYPROJ_GLOBAL_CONTEXT=ON
+ PYPROJ_GLOBAL_CONTEXT=ON
+ echo 569704.57 4269024.67
+ cs2cs EPSG:26915 +to EPSG:26715
589057.58       -3.54 0.00
+ echo 569704.57 4269024.67
+ cs2cs EPSG:26915 +to EPSG:26715
569722.40       4268814.27 0.00
+ echo -92.199881 38.56694
+ proj EPSG:26915
+ cs2cs EPSG:26915 +to EPSG:26715
569722.40       4268814.27 0.00
+ echo 569704.57 4269024.67
+ cs2cs EPSG:26915 +to EPSG:26715
569722.40       4268814.27 0.00

Note how the first response has the wrong -3.54, but then the later runs are all correct. Last time I saw something like this in a different project it was because dynamic library loading was finding the system glibc instead of the conda/conda-forge version.

Edit: ldd doesn't show a change between calls. Not that I'd expect it to, but still.

dopplershift commented 8 months ago

In theory with conda-forge/proj.4-feedstock#139 in, this should pass...

xylar commented 8 months ago

Are the PyPy errors new?

ocefpaf commented 8 months ago

Are the PyPy errors new?

Looks like problems downloading stuff :-/

xylar commented 8 months ago

Okay, then we just need to rerun them?

ocefpaf commented 8 months ago

Okay, then we just need to rerun them?

I wanted to restart just the failing jobs but I cannot see the right button in the GH web interface. Not sure why, maybe b/c the PR is draft? Anyway, we can restart all and closing and re-opening. However, it seems like a web certificate issue, not a temporary network problem.

xylar commented 8 months ago

I wanted to restart just the failing jobs but I cannot see the right button in the GH web interface. Not sure why, maybe b/c the PR is draft? Anyway, we can restart all and closing and re-opening.

I just restarted the failed jobs. You have to wait for all jobs to fail before you can restart any. GitHub or Azure in its infinite wisdom...

However, it seems like a web certificate issue, not a temporary network problem.

Any idea how to proceed if that's the case?

xylar commented 8 months ago

@ocefpaf, as you predicted, it's still failing with a restart:

__________________ test_sync__source_id__list[input_command0] __________________

input_command = ['pyproj']
tmpdir = local('/tmp/pytest-of-conda/pytest-0/test_sync__source_id__list_inp0')

    @pytest.mark.cli
    @pytest.mark.network
    @PYPROJ_CLI_ENDPONTS
    def test_sync__source_id__list(input_command, tmpdir):
        with tmp_chdir(str(tmpdir)):
            output = subprocess.check_output(
                input_command
                + [
                    "sync",
                    "--source-id",
                    "fr_ign",
                    "--list-files",
                    "--include-already-downloaded",
                ],
                stderr=subprocess.STDOUT,
            ).decode("utf-8")
        lines = output.strip().split("\n")
        assert len(lines) > 2
>       _check_list_files_header(lines)

test/test_cli.py:98: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

lines = ['/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem /etc/pki/tls/certs', 'filename | source_id | area_of_use', '------...CGVD2013RGSPM06.tif | fr_ign | France - Saint-Pierre et Miquelon', 'fr_ign_RAC09.tif | fr_ign | France - Corsica', ...]

    def _check_list_files_header(lines):
>       assert lines[0].rstrip("\r") == "filename | source_id | area_of_use"
E       AssertionError: assert '/etc/pki/ca-...pki/tls/certs' == 'filename | s...| area_of_use'
E         - filename | source_id | area_of_use
E         + /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem /etc/pki/tls/certs

test/test_cli.py:76: AssertionError
__________________________ test_sync__download_grids ___________________________
dopplershift commented 8 months ago

It's also all PyPy, which I'm really in the bucket of...just don't care. It should also be noted that these (well, 3/4) failed the last run too.

xylar commented 8 months ago

It's also all PyPy, which I'm really in the bucket of...just don't care.

While I'm not a PyPy user, it seems odd to put so much work in getting ppc64le working for a small number of users only to chuck PyPy (also presumably for a small number of users).

It should also be noted that these (well, 3/4) failed the last run too.

I don't quite follow. Which run was that? Everywhere I looked in recent builds (other than the most recent builds on this PR), PyPy built okay.

ocefpaf commented 8 months ago

I'm in favor of merging this without pypy, it looks like a problem with pypy certs and, when that is resolved, we can trigger the CIs to produce the packages.

@mattip, does that failure ring a bell?

dopplershift commented 8 months ago

I don't quite follow. Which run was that? Everywhere I looked in recent builds (other than the most recent builds on this PR), PyPy built okay.

If you go to the azure page for the repo itself, you can see the last run for this PR, where you can see 7 failures, including the 4 non-PyPy builds for ppc64le that are now working.

Regarding the work...well I wasn't responsible for that. In all honesty, it's just demoralizing to keep needing to play whack a mole on weird esoteric platforms when all I want is to have a fully functioning Python 3.12 environment on my Mac. People advocate for these corner cases, but then it ends up being others who have to deal with the breakage. (probably time for me to sign off since I'm getting cranky.)

xylar commented 8 months ago

Okay, @ocefpaf, I'm convinced of your approach. We can try building PyPy later.

If you go to the azure page for the repo itself, you can see the last run for this PR, where you can see 7 failures, including the 4 non-PyPy builds for ppc64le that are now working.

Ah, thanks! I missed that.

Regarding the work...well I wasn't responsible for that. In all honesty, it's just demoralizing to keep needing to play whack a mole on weird esoteric platforms when all I want is to have a fully functioning Python 3.12 environment on my Mac. People advocate for these corner cases, but then it ends up being others who have to deal with the breakage. (probably time for me to sign off since I'm getting cranky.)

Fully understood. It's mostly that each time we decide to skip something that isn't working, it feels like it ends up biting us in the butt later. Like skipping ppc64le prior to the python 3.12 release. I prefer to either consciously drop (or never add) support for a platform than to drop temporarily like we're advocating doing here and like we did with ppc64le previously. I think it just creates more work for someone in the end.

In this particular case where it seems like broken certificates that are likely to get fixed without us having to do anything but wait, I think that's fair enough. I will make a point of trying to rebuild PyPy early next week and we'll see if that can make us all happy...

xylar commented 8 months ago

@conda-forge-admin, please rerender

xylar commented 8 months ago

@zklaus, let's get this reviewed and merged.

zklaus commented 8 months ago

@zklaus, let's get this reviewed and merged.

I'm all for it. Just noticed that I am not a maintainer and could neither review nor merge. I have now added myself as a maintainer; if that's ok with you, feel free to merge.

I'll setup the pypy-reenable-pr next.

PS: Good work everyone, particularly @djhoese :) This must be the PR that has my name with the least of my work :blush:

xylar commented 8 months ago

This must be the PR that has my name with the least of my work 😊

Well, certainly thanks for getting the ball rolling!

xylar commented 8 months ago

I'll setup the pypy-reenable-pr next.

@zklaus, that would be great! I did a little poking around but couldn't figure out what is causing the line:

/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem /etc/pki/tls/certs

to be prepended to data received by pyproj sync for PyPy, and what would have caused this to change since the last successful PyPY build. Perhaps you have more experience with this kind of thing or will have better luck investigating than I did.

mattip commented 8 months ago

Is there any stdout parsing going on in processing that command? PyPy had a bug in the last release where debug cruft is printed out in _ssl. I started a new pypy feedstock build conda-forge/pypy3.6-feedstock#112 to clean that up.

github-actions[bot] commented 8 months ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!