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

oldest-supported-numpy Erroneously Required in Apple Silicon Python 3.10 Wheel #201

Open
william-silversmith opened this issue Mar 7, 2023 · 30 comments

Comments

@william-silversmith
Copy link
Contributor

william-silversmith commented Mar 7, 2023

Hi zfpy team,

Thanks again for uploading my wheels! I was attempting to use them and ran into an issue that I found was my own doing. During the wheel compilation, I had added install_requires=['oldest-supported-numpy', 'cython'] to setup.py while I was experimenting, but forgot to remove it. This is causing a version conflict with some other software I am using that requires a newer version of numpy. I'm happy to produce a set of fixed wheels if that would be okay. I've developed a superior build process since I last tried this.

# build_macos_universal2_libzfp.sh
#!/usr/bin/sh

ARMDIR=build_macos/arm64
X86DIR=build_macos/x86_64

mkdir -p $ARMDIR
mkdir -p $X86DIR

cmake -B $ARMDIR -S . -DCMAKE_OSX_ARCHITECTURES=arm64 -DBUILD_TESTING=NO -DBUILD_SHARED_LIBS=OFF
cmake -B $X86DIR -S . -DCMAKE_OSX_ARCHITECTURES=x86_64 -DBUILD_TESTING=NO -DBUILD_SHARED_LIBS=OFF

DIR=$(pwd)
cd $DIR/$ARMDIR && make
cd $DIR/$X86DIR && make

lipo -create -output $DIR/libzfp.a $DIR/$ARMDIR/lib/libzfp.a $DIR/$X86DIR/lib/libzfp.a
# setup-macos.py
from setuptools import setup, Extension

class NumpyImport:
  def __repr__(self):
    import numpy as np

    return np.get_include()

  __fspath__ = __repr__

setup(
    name="zfpy",
    version="1.0.0",
    author="Peter Lindstrom",
    author_email="[email protected]",
    url="https://zfp.llnl.gov",
    description="zfp compression in Python",
    long_description="zfp is a compressed format for representing multidimensional floating-point and integer arrays. zfp provides compressed-array classes that support high throughput read and write random access to individual array elements. zfp also supports serial and parallel compression of whole arrays using both lossless and lossy compression with error tolerances. zfp is primarily written in C and C++ but also includes Python and Fortran bindings.",
    ext_modules=[
        Extension("zfpy", ["python/zfpy.pyx"],
            include_dirs=["include", NumpyImport()],
            library_dirs=['/Users/wms/code/zfp/'],
            libraries=["zfp"]
        )
    ]
)
# tox.ini
[tox]
envlist = py38,py39,py310,py311

[testenv]
platform = darwin
deps = 
	oldest-supported-numpy
	cython

commands = 
	python setup.py bdist_wheel

To actually build the wheels:

sh ./build_macos_universal2_libzfp.sh
tox

The key difference here from my previous build process is that I have finally figured out how to get tox to run on setup.py files that require numpy. This ensures a clean build.

@lindstro
Copy link
Member

@da-wad Can you please comment on this?

@da-wad
Copy link
Contributor

da-wad commented Mar 16, 2023

Hi, I'm happy to create (another) post release with updated wheels from you, but want to be confident that we don't create other problems. Ideally we want wheels which work with every supported version of numpy, so does building with a newer numpy mean it is still possible to use the wheels with an older numpy? Or is there a way to insist on the "oldest-supported-numpy" being a minimum version?

@william-silversmith
Copy link
Contributor Author

Hi,

Perhaps this is not the most rigorous way to do it, but the way I've done it in the past was to insist on using oldest-supported-numpy during the building of wheels (i.e. in the github action script) and allow the numpy requirement for installation to be free-floating. Everything was fine the way things were being done, I just did something dumb while building the 3.10 wheel manually and made it so that pip would insist on using the exact oldest-supported-numpy version during use, not just during building.

You can also just steal the current parameters from oldest-supported-numpy and put them in the requirements as numpy >= VERSION

@tasansal
Copy link

tasansal commented May 2, 2023

This is causing me a big problem with dependency resolution with Poetry. It causes a lot of unnecessary checks when generating lock files. It causes an explosion of dependency resolution checks and takes an hour to generate a lock file (instead of 5 seconds). Suprisingly, sometimes it even conflicts with numpy requirements from matplotlib and the lock file generation fails.

I also would support having a numpy >= VERSION instead of oldest-supported-numpy.

@william-silversmith anything I can help with to push this through?

@fcollman
Copy link

fcollman commented Aug 9, 2023

@da-wad @william-silversmith What can we do to resolve this concern? Installing things with zfpy is presently pretty problematic with python 3.10 right now due to this issue.

@william-silversmith
Copy link
Contributor Author

I can at least try building some universal wheels on my end and post them here.

@william-silversmith
Copy link
Contributor Author

Hi all,

Here are some updated wheels with the version bumped to 1.0.1 so that if you re-upload them (along with a new build) people will automatically have them installed as the newest version. Let me know if you'd like to try something different re: versioning.

zfpy-1.01-universal2-wheels.zip

@tasansal
Copy link

tasansal commented Sep 2, 2023

@lindstro @da-wad How can we resolve this? 1.0.0 dependency resolution doesn't even work with Python 3.11.

Like @fcollman mentioned it does cause many issues with installation.

I believe the universal wheels by @william-silversmith will solve the issue given Zfp releases a 1.0.1 version.

Thanks a lot.

@lindstro
Copy link
Member

lindstro commented Sep 2, 2023

I don't know all the issues involved, though I'd be willing to cut a 1.0.1 release if that fixes the problem. It's however unclear to me what changes to zfp would be needed. Is it just a matter of bumping the version, or do we also need to make changes to the zfpy build scripts? Are there any interactions with #210 that we need to consider?

@tasansal
Copy link

tasansal commented Sep 13, 2023

I don't know all the issues involved, though I'd be willing to cut a 1.0.1 release if that fixes the problem. It's however unclear to me what changes to zfp would be needed. Is it just a matter of bumping the version, or do we also need to make changes to the zfpy build scripts? Are there any interactions with #210 that we need to consider?

Just to push this forward, let me add some comments :) @william-silversmith please chime in if I'm wrong.

  1. Shouldn't affect any ZFP business logic. This is purely for Python wrapper on Mac wheels systems.
  2. I think the build scripts may need to be updated. @william-silversmith has the scripts he used to build the ARM64 Mac wheels in the issue description. I'm not sure how these play with existing build scripts.
  3. Shouldn't affect Numpy 2.0 #210; the erroneous oldest-supported-numpy addition only makes things break, and it has no upside in this issue. Zfpy can make a specific numpy version minimum/maximum without the need for oldest-supported-numpy.

@william-silversmith
Copy link
Contributor Author

Yes, essentially it just needs a new release with the new binaries uploaded. The previous version of them suffered from a mistake I made while building manually. The wheels are built with oldest-supported-numpy (and so are compatible with future numpy versions) but no longer require using the oldest supported numpy version during installation.

@tasansal
Copy link

@lindstro @william-silversmith, can we please fix this one way or another? I am unsure what exactly needs to be done if you all tell me I can make a PR! Or is it just a matter of making new wheels?

@william-silversmith
Copy link
Contributor Author

It's really just a matter of bumping the version and releasing new wheels, incl. the ones I posted above.

@lindstro
Copy link
Member

Just bumping the version is of course easy enough. However, assuming we'd release what's currently on develop, this branch has a fair number of commits since the last 1.0.0 release. Some additions may or may not yet be rigorously tested, documented, or even complete, and it would take some time to go through them to ensure develop is in a releasable state. I don't have time for that this week, but I'm hoping to work on it next week.

@da-wad
Copy link
Contributor

da-wad commented Oct 3, 2023

It looks like the fix is coming soon: https://github.blog/2023-10-02-introducing-the-new-apple-silicon-powered-m1-macos-larger-runner-for-github-actions/

@tasansal
Copy link

Just bumping the version is of course easy enough. However, assuming we'd release what's currently on develop, this branch has a fair number of commits since the last 1.0.0 release. Some additions may or may not yet be rigorously tested, documented, or even complete, and it would take some time to go through them to ensure develop is in a releasable state. I don't have time for that this week, but I'm hoping to work on it next week.

Hi @lindstro, just wanted to check if any progress was made about this. Please let me know if there is anything I can do to help :) I can test things, or review code changes etc.

@lindstro
Copy link
Member

Sorry this is taking so long. I've been on extensive travel the past couple of months but will return to this issue tomorrow. I feel confident that we will be ready to release 1.0.1 next week.

@lindstro
Copy link
Member

zfp 1.0.1 has been released.

@tasansal
Copy link

Thanks @lindstro !

It seems like the wheels didn't make it to PyPI yet. Seems like this is where they get built: https://github.com/LLNL/zfpy-wheels. Do they need to be triggered manually? Currently it seems to point to hash f39af...

Also, like @da-wad mentioned, the Apple silicon runners seem to be available. I think by adding macos-13-xlarge to the build matrix, the ARM wheels may just build and work?

@lindstro
Copy link
Member

@tasansal You're right--they need to be triggered manually.

@da-wad Are you available to assist with this?

@da-wad
Copy link
Contributor

da-wad commented Dec 19, 2023

Will have time to fix later this week.

@da-wad
Copy link
Contributor

da-wad commented Dec 21, 2023

setup.py is not actually valid Python... see:

zfp/setup.py

Line 14 in 3f8de26

libraries=["zfp"], library_dirs=["build/lib64", "build/lib/Release"]), language_level = "3"]

@GarrettDMorrison I guess there was a reason to add language_level = "3" there? Is this supposed to be an argument to the Extension constructor, or? zfpy_wheels builds seem to run again when I remove it...

@GarrettDMorrison
Copy link
Member

@da-wad thank you for catching this, not sure how that ended up in there. There were some zfpy test build issues and this was done while figuring them out. It looks more like a snapshot from when we were in the middle of modifying setup.py while debugging though and not anything meant to be committed. I'm not sure how it made it through but it should probably be reverted. Not sure what the best way to do that is since it is technically in the 1.0.1 release.

@lindstro
Copy link
Member

With my very limited Python knowledge, I don't know what's impacted by this. I'd hate to have to cut another release with just a handful of characters changed. Suggestions?

@lindstro
Copy link
Member

@da-wad I assume we need to fix setup.py to make progress. I can envision several approaches, such as a hot fix (perhaps applied to the release1.0.1 branch), a new patch or tweak release (I'd prefer not to), or perhaps even pointing zfpy-wheels at a commit on develop. Do you have a sense of whether this is the only issue stopping us from building wheels?

@tasansal
Copy link

tasansal commented Mar 7, 2024

Hi everyone :) Sorry to bother you again about this, but any progress made here?

We are still having issues with Macs and dependency resolution.

@lindstro
Copy link
Member

lindstro commented Mar 9, 2024

I'm afraid all this Python stuff is beyond my expertise. I would welcome any suggestions on how to move forward.

@tasansal
Copy link

I can take a stab at it after spring break this week. @lindstro, who maintains the Python wheels for ZFP?

@lindstro
Copy link
Member

@lindstro, who maintains the Python wheels for ZFP?

@da-wad has historically been the one doing this on a volunteer basis. As the zfp team currently does not have a Python expert, we have to lean on external contributors, so I really appreciate your offer to help with this. To be frank, I don't know exactly what the issues are, e.g., whether it's just a fix needed for setup.py or if there are other changes needed to how we generate Python wheels, e.g., to support Apple silicon.

@lindstro
Copy link
Member

@da-wad @tasansal I want to make sure this does not fall off our radar. If either of you could help out, I would greatly appreciate it.

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

No branches or pull requests

6 participants