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.

Sign up for GitHub

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

Jump to bottom

Numpy 2.0 #437

Merged
merged 11 commits into from
Aug 8, 2024
Merged

Numpy 2.0 #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
acc71ec
Update versions of github actions
achaikou Jan 26, 2024
c0f1caa
Migrate to v4 in download/upload artifact
achaikou Jul 26, 2024
b209476
Drop support for python 3.7
achaikou Jul 12, 2024
8642698
Unpin numpy version
achaikou Jul 12, 2024
cea3efe
Update macos github runners
achaikou Jul 12, 2024
fed74d3
Add note about i686
achaikou Jul 12, 2024
05bfef5
Remove reference to setuptools.command.test.test
achaikou Jul 12, 2024
877c8d5
Fix macos 3.8 wheel skip
achaikou Jul 26, 2024
62eaddb
Add python CI build
achaikou Aug 7, 2024
c5b76a1
Drop setup.py test command
achaikou Aug 7, 2024
007d5e1
Temporary skip wheels for python 3.13
achaikou Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/analyzers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
cppcheck:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: './.github/actions/build_core_dependencies'
with:
Expand Down Expand Up @@ -41,7 +41,7 @@ jobs:
scan_build:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: './.github/actions/build_core_dependencies'
with:
Expand Down Expand Up @@ -69,7 +69,7 @@ jobs:
valgrind:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: './.github/actions/build_core_dependencies'
with:
Expand All @@ -96,7 +96,7 @@ jobs:
debug:
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: './.github/actions/build_core_dependencies'
with:
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/bigendian.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@ jobs:

steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up QEMU
uses: docker/setup-qemu-action@v2
uses: docker/setup-qemu-action@v3

- name: Set up Docker Buildx
id: buildx
uses: docker/setup-buildx-action@v2
uses: docker/setup-buildx-action@v3

- name: Build
uses: docker/build-push-action@v4
uses: docker/build-push-action@v5
with:
builder: ${{ steps.buildx.outputs.name }}
context: .
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/core.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:

strategy:
matrix:
os: [macos-11]
os: [macos-13, macos-14]
include:
- os: ubuntu-20.04

Expand All @@ -27,7 +27,7 @@ jobs:
cmake_generator: "-G \"Visual Studio 16 2019\" -A Win32"

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- name: Elevate privileges
if: ${{ !contains( matrix.os, 'windows') }}
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/docs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: actions/setup-python@v4
- uses: actions/setup-python@v5
with:
python-version: '3.10'

Expand Down
62 changes: 62 additions & 0 deletions .github/workflows/python.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
name: Build and test python
#Build and test with python turned on

on:
push:
branches: [master]
pull_request:
branches: [master]
workflow_dispatch:

jobs:
build_python:
runs-on: ${{ matrix.os }}

strategy:
matrix:
include:
- os: ubuntu-20.04

steps:
- uses: actions/checkout@v4

- uses: "./.github/actions/build_core_dependencies"
with:
privileges: "sudo"

- uses: actions/setup-python@v5
with:
python-version: "3.11"

- name: Create python venv
shell: bash
run: |
python -m venv venv
. ./venv/bin/activate
python -m pip install --upgrade pip
python -m pip install --upgrade setuptools
python -m pip install -r python/requirements-dev.txt
which python

- name: Configure dlisio
shell: bash
run: |
. ./venv/bin/activate
cmake -S . -B build \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DBUILD_PYTHON=ON

- name: Build dlisio
shell: bash
run: |
sudo cmake \
--build build \
--parallel \
--config Release

- name: Test dlisio
shell: bash
run: |
cd build
ctest -C Release --output-on-failure --verbose
30 changes: 18 additions & 12 deletions .github/workflows/wheels.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,25 @@ jobs:
- os: ubuntu-20.04
arch: aarch64
- os: ubuntu-20.04
arch: i686
- os: macos-11
arch: i686 #builds from cibuildwheel, qemu not needed
- os: macos-13
arch: x86_64
- os: macos-13-xlarge
- os: macos-14
arch: arm64

steps:
- name: Disable autocrlf
run: |
git config --global core.autocrlf false

- uses: actions/checkout@v3
- uses: actions/checkout@v4

# Used to host cibuildwheel
- uses: actions/setup-python@v4
- uses: actions/setup-python@v5
with:
python-version: '3.10'

- uses: docker/setup-qemu-action@v2
- uses: docker/setup-qemu-action@v3
if: ${{ matrix.arch == 'aarch64' }}
name: Set up QEMU

Expand All @@ -66,15 +66,17 @@ jobs:

CIBW_ARCHS: ${{ matrix.arch }}
# musllinux arch skip: unlikely someone wants to run dlisio on alpine on non-standard architecture
# musllinux cp37 and cp38 skip: latest available numpy doesn't provide musslinux wheels, so it is unlikely useful
# macosx 38 skip: cibuildwheel can't test it
CIBW_SKIP: pp* *-musllinux_i686 *-musllinux_aarch64 cp37*-musllinux* cp38*-musllinux* cp38*-macosx_*:arm64 cp36-*
# musllinux cp38 skip: latest available numpy doesn't provide musslinux wheels, so it is unlikely useful
# macosx arm 38 skip: cibuildwheel can't test it
# 36, 37, 313 skip: python versions not supported
CIBW_SKIP: pp* *-musllinux_i686 *-musllinux_aarch64 cp38*-musllinux* cp38*-macosx_*arm64 cp36-* cp37-* cp313-*

run: |
python -m cibuildwheel --output-dir wheelhouse python/

- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
with:
name: wheel-${{ matrix.os }}-${{ matrix.arch }}
path: ./wheelhouse/*.whl

publish:
Expand All @@ -83,9 +85,13 @@ jobs:
runs-on: ubuntu-latest
if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags')
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- uses: actions/download-artifact@v3
- uses: actions/download-artifact@v4
with:
pattern: wheel-*
merge-multiple: true
path: ./wheelhouse/

- name: Publish wheels to PyPI

Expand Down
5 changes: 2 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ pip install dlisio

| | macOS Intel | macOS ARM | Windows 64bit | Windows 32bit | manylinux x86_64 | manylinux aarch64 | manylinux i686 | musllinux x86_64
|---------------|----|-----|-----|----|----|----|----|----|
| CPython 3.7 | ✅ | - | ✅ | ✅ | ✅ | ✅ | ✅ | - |
| CPython 3.8 | ✅ | - | ✅ | ✅ | ✅ | ✅ | ✅ | - |
| CPython 3.9 | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ |
| CPython 3.10 | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ |
Expand Down Expand Up @@ -97,13 +96,13 @@ To develop dlisio, or to build a particular revision from source, you need:

* A C++11 compatible compiler (tested on gcc, clang, and msvc 2019)
* [CMake](https://cmake.org/) version 3.5 or greater
* [Python](https://python.org) version 3.7 or greater
* [Python](https://python.org) version 3.8 or greater
* [fmtlib](http://fmtlib.net/) tested mainly with 7.1.3
* [mpark_variant](https://github.com/mpark/variant)
* [pybind11](https://github.com/pybind/pybind11) version 2.6 or greater
* [setuptools](https://pypi.python.org/pypi/setuptools) version 28 or greater
* [layered-file-protocols](https://github.com/equinor/layered-file-protocols)
* python packages pytest, pytest-runner, and numpy
* python packages pytest and numpy

If you do not have pybind11 installed on your system, the easiest way to get a
working copy is to `pip3 install pybind11` (NP! pybind11, not pybind)
Expand Down
2 changes: 1 addition & 1 deletion python/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,6 @@ endif()
# different args to setup.py, rebuilding the python lib (and wrongly so as it
# either won't find dlisio or picked up on a system installed one)
add_test(NAME python.unit
COMMAND ${python} ${setup.py} --skip-cmake test
COMMAND ${python} -m pytest tests
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
)
20 changes: 5 additions & 15 deletions python/dlisio/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,8 @@
from . import lis
from . import dlis

import sys

# remove else once support for python 3.7 is over
if sys.version_info >= (3, 8):
try:
import importlib.metadata
__version__ = importlib.metadata.version(__name__)
except importlib.metadata.PackageNotFoundError:
pass
else:
try:
import pkg_resources
__version__ = pkg_resources.get_distribution(__name__).version
except pkg_resources.DistributionNotFound:
pass
try:
import importlib.metadata
__version__ = importlib.metadata.version(__name__)
except importlib.metadata.PackageNotFoundError:
pass
1 change: 0 additions & 1 deletion python/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ requires = [
"scikit-build",
"wheel",
"pybind11",
"pytest-runner",
]

[tool.cibuildwheel]
Expand Down
3 changes: 1 addition & 2 deletions python/requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@

bandit
numpy<2.0
ajaust marked this conversation as resolved.
Show resolved Hide resolved
numpy
setuptools
setuptools_scm
pytest
pytest-runner
pybind11
hypothesis
sphinx
Expand Down
3 changes: 0 additions & 3 deletions python/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,2 @@
[metadata]
version = 1.0.1

[aliases]
test = pytest
8 changes: 1 addition & 7 deletions python/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import os
import skbuild
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the commit message: I thought we concluded that skbuild only brought setuptoools.command.test into the symbol table by importing it and never overwrote it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We concluded that it now doesn't override setuptools, but they before supplied their own custom test command.
https://github.com/scikit-build/scikit-build/blob/ff9cde427e2ed8f6f683139a23286c063d510352/skbuild/command/test.py

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but they did not overwrite setuptoools.command.test in the symbol table, so you would still access setuptools test command when referring directly to setuptools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree.
But which exact wording in the commit message you don't like then?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"(It is though unclear if test command from setuptools was ever called by
any parts of our solution.)" and "However setuptools.command.test was
imported directly in the skbuild, where they exchanged 'test' with their
own command. So we happened to use it indirectly."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. "(It is though unclear if test command from setuptools was ever called by
    any parts of our solution.)"

It was imported, but was it ever called? Did we just overrode this stuff because we didn't like it and was it ever part of our test execution?
Because I would assume that if us calling test either through skbuild or setuptools was somehow important, then something would have broke down once we removed the override line because some code that is the essence of the setuptools..test method would never be called.

  1. I don't understand what is wrong
  • setuptools.command test was imported directly in the skbuild by using from setuptools.command.test import test as _test
  • it was done in place where they exchanged test with their own command - aka in the skbuild.command.test file
  • we happened to use that import (of setuptools.command.test.test) indirectly because we never imported it, yet skbuild did.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, I do think the setuptools test command was used when running our tests. I think skbuild test command was changed so that It behaves the same as the setuptools test command.

  2. I don't think they exchange the setuptools test command. They just define their own. I may have misunderstood your wording here though. I read it as "we were using skbuild test command indirectly", but I think what you ment is "we use the import statement indirectly"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Setuptools.test is deprecated, that's why I doubt it is used any longer. And I am not sure it was used because deprecation issue is from 2019. Because of this uncertainty I added this comment as I feel it is important note.
  2. Yes, by exchanged I didn't mean "go inside their code and substitute theirs with custom", but "we have extended their version and exchanged the original call with ours through kw["cmdclass"], so that whenever something in setuptools calls test, our test will be called".
    And yes, I mean that we use import statement indirectly.

I suggest you to propose your own wording here, because otherwise we will spend another 10 comments with me trying to figure out how to word that so it pleases you 😄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not see any reason why setuptools test command would not be called, so I think it was.

I don't think it is correct to say that anything in setuptools is replaced, rather that skbuild is used in place of setuptools (the setup() function from skbuild is called).

You can have a look at the wording in my PR to segyio (equinor/segyio#576) and see if you agree with it.

import setuptools

class get_pybind_include(object):
def __init__(self, user=False):
Expand Down Expand Up @@ -36,13 +35,11 @@ def get_long_description():
packages = ['dlisio', 'dlisio.dlis', 'dlisio.lis', 'dlisio.common', 'dlisio.dlis.utils'],
license = 'LGPL-3.0',
platforms = 'any',
install_requires = ['numpy < 2.0'],
install_requires = ['numpy'],
ajaust marked this conversation as resolved.
Show resolved Hide resolved
setup_requires = ['setuptools >= 28',
'pybind11 >= 2.3',
'setuptools_scm',
'pytest-runner',
],
tests_require = ['pytest'],
# we're building with the pybind11 fetched from pip. Since we don't rely on
# a cmake-installed pybind there's also no find_package(pybind11) -
# instead, the get include dirs from the package and give directly from
Expand All @@ -54,7 +51,4 @@ def get_long_description():
# supported OS X release 10.9
'-DCMAKE_OSX_DEPLOYMENT_TARGET=10.9',
],
# skbuild's test imples develop, which is pretty obnoxious instead, use a
# manually integrated pytest.
cmdclass = { 'test': setuptools.command.test.test },
)
Loading