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

[RFC] Keep building for py37 #896

Closed
wants to merge 3 commits into from
Closed

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Dec 4, 2022

This was explicitly requested by @rxm7706, and also asked about by upstream dev @raulcd in the same PR.

I said at the time:

It's technically possible. Assuming the CI passes it would not add an extreme maintenance burden, though it depends also whether the other maintainers agree (w.r.t. precedent etc.). But this will be a topic for a separate PR.

This PR is to see if it passes CI cleanly (thankfully, the host deps don't carry much python-deps that are migration-sensitive, so this might just work), and then gather feedback from @conda-forge/arrow-cpp if we actually want to do this.

@conda-forge-linter
Copy link
Contributor

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.

@h-vetinari
Copy link
Member Author

So the problem here is that despite 3d73473 being purely additive (in terms of config), it somehow disables the builds for py311:

anaconda upload \
    /home/conda/feedstock_root/build_artifacts/linux-64/apache-arrow-proc-4.0.0-cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/libarrow-10.0.1-h9ff0032_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/arrow-cpp-10.0.1-h3e2b116_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/arrow-cpp-proc-4.0.0-cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-10.0.1-py37he8182db_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-10.0.1-py310ha7322a8_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-10.0.1-py39h4d4a1a0_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-10.0.1-py38h1fd4ed9_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-tests-10.0.1-py37h82fcdeb_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-tests-10.0.1-py310h50e40b8_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-tests-10.0.1-py39h947458c_0_cuda.conda \
    /home/conda/feedstock_root/build_artifacts/linux-64/pyarrow-tests-10.0.1-py38hbf57a63_0_cuda.conda
anaconda_upload is not set.  Not uploading wheels: []

This has nothing to do with this feedstock, but is a problem of conda-build it seems. FWIW, I've tried the following patch to cbc, and nothing changes (the rerender is already correct):

diff --git a/recipe/conda_build_config.yaml b/recipe/conda_build_config.yaml
index cd4b5d8..ed8457f 100644
--- a/recipe/conda_build_config.yaml
+++ b/recipe/conda_build_config.yaml
@@ -5,13 +5,16 @@ python:
   - 3.8.* *_cpython
   - 3.9.* *_cpython
   - 3.10.* *_cpython
+  - 3.11.* *_cpython
 python_impl:
   - cpython           # [not (osx and arm64)]
   - cpython
   - cpython
   - cpython
+  - cpython
 numpy:
   - 1.20              # [not (osx and arm64)]
   - 1.20
   - 1.20
   - 1.21
+  - 1.23

As it stands, someone can fix what's causing the missing py311 builds (not me), or this PR is dead.

@rxm7706
Copy link

rxm7706 commented Dec 4, 2022

So the problem here is that despite 3d73473 being purely additive (in terms of config), it somehow disables the builds for py311:

@conda-forge/help-python could you help here, is this a knows issue with a workaround.

@mbargull
Copy link
Member

mbargull commented Dec 4, 2022

@conda-forge-admin, please rerender

@h-vetinari
Copy link
Member Author

Hey @mbargull, thanks for taking a look!

FWIW, I've tried the following patch to cbc [equivalent to 55678c7 ], and nothing changes (the rerender is already correct):

As I mentioned, it's not a question of the rerendering; 3d73473 already has the correct set of python versions to build.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/arrow-cpp-feedstock/actions/runs/3613223780.

@mbargull
Copy link
Member

mbargull commented Dec 4, 2022

As I mentioned, it's not a question of the rerendering; 3d73473 already has the correct set of python versions to build.

Yes, I don't expect the rerendering to yield any changes. I only set off that bot request for good measure.
For the actual explanation why 3.11 variants weren't build and why you need 3.11 in cbc.yaml, see conda/conda-build#4666 (comment) :

Values from the recipe's conda_build_config.yaml take precedence over those from other config files.
Note they are not additive but replace higher priority ones.

@mbargull
Copy link
Member

mbargull commented Dec 4, 2022

I said at the time:

It's technically possible. Assuming the CI passes it would not add an extreme maintenance burden, though it depends also whether the other maintainers agree (w.r.t. precedent etc.). But this will be a topic for a separate PR.

Yes, this can indeed create additional maintenance burden in case dependencies are updated/rebuilt.
Since we do not build for Python 3.7 by default anymore on conda-forge any Python package dependencies will likely be stuck on older versions if their newer versions are built without Python 3.7 variants.
(This feedstock doesn't seem to have too many Python package dependencies, so probably not a big deal; but just a thing to generally keep in mind.)

@h-vetinari
Copy link
Member Author

h-vetinari commented Dec 4, 2022

For the actual explanation why 3.11 variants weren't build and why you need 3.11 in cbc.yaml, see conda/conda-build#4666 (comment) :

Ah, so I got tricked into ignoring that diff because the rerender didn't change, but conda build still needs it, got it. Thanks for looking at this so quickly!

Since we do not build for Python 3.7 by default anymore on conda-forge any Python package dependencies will likely be stuck on older versions if their newer versions are built without Python 3.7 variants.

I'm aware; I also looked at the (python) deps and they don't look too abi-migration-exposed, but yeah, my standpoint is essentially: python 3.7 is dead in conda-forge, but as long as it only adds a small amount of CI runtime and no other complications, I'm fine with building those packages in the face of an explicit request (also, since we're now running the test suite, it's unlikely that broken packages would sneak past).

@h-vetinari
Copy link
Member Author

@conda-forge/arrow-cpp
There's an explicit user request to provide python 3.7 builds for a while longer. With the new CI setup it doesn't cost much (just a few minutes runtime within the same job), so I'm leaning towards accommodating that request.

If there are any opposing voices, please speak up, otherwise I'm planning to merge this in a week. :)

@xhochy
Copy link
Member

xhochy commented Dec 12, 2022

Fine with me, we probably want to drop this in 6-12 months latest though ;)

@jakirkham
Copy link
Member

I'd prefer skipping this. Conda-forge already dropped it. We will eventually start feeling the pain of not having other Python 3.7 packages not get updated. Easier to let it go.

@rxm7706
Copy link

rxm7706 commented Dec 12, 2022

I'd prefer skipping this. Conda-forge already dropped it. We will eventually start feeling the pain of not having other Python 3.7 packages not get updated. Easier to let it go.

@jakirkham @h-vetinari
Apologies for even asking and putting the maintainers through this discussion.

Would it be possible to support Py37 for a bit, its on us for being behind on our python upgrades for multiple reasons. But so far PyArrow has provided us a great path to get off many legacy vendor / 3rd party integrations.

Two big reason we were hoping for PyArrow >=10.0 upgrades while using python 3.7 was to leverage all the work w.r.t JDBC drivers and OpenTelemetry Integration.

https://arrow.apache.org/blog/2022/11/01/arrow-flight-sql-jdbc/
apache/arrow#11920

Thank you.

@jakirkham
Copy link
Member

The challenge that we tend to see as maintainers is folks will raise issues about incompatibilities between different packages. For example in issue ( #695 ) a user raised an issue about an incompatibility between Arrow and Tensorflow. More of these kinds of incompatibilities can creep in when continuing to maintain a legacy Python version for one package and not others. This in turn invites requests to readd the legacy Python version in more places. It's easier to stop this scope creep and increased maintenance burden by simply being clear with users about what is supported/not after making a decision.

Would also add conda-forge is far slower than other parts of the ecosystem to drop Python versions. Consider for example NEP 29, which is followed by many projects, encouraged dropping Python 3.7 ~1yr ago. Whereas conda-forge dropped Python 3.7 near the end of October ( conda-forge/conda-forge-pinning-feedstock#3573 ) (even when some folks were interested in us dropping it quicker ( conda-forge/conda-forge-pinning-feedstock#2623 )).

@h-vetinari
Copy link
Member Author

We will eventually start feeling the pain of not having other Python 3.7 packages not get updated.

It's mostly @rxm7706 that will feel this, but I checked, and the host deps here are not very migration-exposed.

I tend to agree with you, but given that it's essentially no effort (not even an increase in CI jobs), I think it's fine to help out for such an explicit request (though with no promises or guarantees for the future, as I noted in cbc.yaml).

@h-vetinari
Copy link
Member Author

Ah, it seems we cross-posted. Any issues w.r.t. py37 would clearly be won't-fix, it should be clear IMO from the discussion (and the comment in the cbc I mentioned) that this is purely on a courtesy basis and not policy.

But then, I also fully agree with you. I certainly don't feel strongly enough about this to push against sustained opposition, I just thought it's a good thing to have people engaging us on the feedstock, and not hard to help out in this case.

@h-vetinari
Copy link
Member Author

I certainly don't feel strongly enough about this to push against sustained opposition

@jakirkham, can you qualify how strongly you feel about this somewhere between don't-like-it, opposed, and strongly opposed? Could you live with an exception here? Otherwise I'll close this PR.

@h-vetinari
Copy link
Member Author

@jakirkham, can you qualify how strongly you feel about this somewhere between don't-like-it, opposed, and strongly opposed? Could you live with an exception here? Otherwise I'll close this PR.

@jakirkham Could you please comment?

@h-vetinari
Copy link
Member Author

h-vetinari commented Jan 19, 2023

sorry, this one's an ex-parrot...

@h-vetinari h-vetinari closed this Jan 19, 2023
@h-vetinari h-vetinari deleted the py37 branch January 19, 2023 23:08
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.

6 participants