Numpy 2.0 #437
Numpy 2.0 #437
Conversation
Current ones are deprecated now.
From v4 artifacts must have unique names or we get "Conflict: an artifact with this name already exists on the workflow run".
It's been more than a year since Python 3.7 EOL.
Looks like we are not affected by changes in numpy.
macOS 11 support has been dropped. Updating to macos-13 as latest standard image that is Intel MacOS. Changing macos-13-xlarge to macos-14 as it is arm [1] and doesn't require xlarge runner. [1] https://github.com/actions/runner-images?tab=readme-ov-file#available-images
| @@ -2,7 +2,6 @@ | |||
|
|
|||
| import os | |||
| import skbuild | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, agree.
But which exact wording in the commit message you don't like then?
There was a problem hiding this comment.
"(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."
There was a problem hiding this comment.
- "(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.
- 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.
There was a problem hiding this comment.
-
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.
-
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"?
There was a problem hiding this comment.
- 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.
- 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 😄
There was a problem hiding this comment.
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.
Originally skbuild supplied its own test command which called "develop" before calling original setuptools test command. In order to avoid calling "develop", we directly exchanged this skbuild.test command with setuptools.command.test.test Note that in setup.py we only imported setuptools, not its submodules like setuptools.command.test. In skbuild they supply own version of test command to invoke and there they imported setuptools.command.test. So we happened to use this import indirectly. In 0.18 release skbuild removed custom test command in [1]. As setuptools.command.test import went along with it, we started getting errors like module 'setuptools.command' has no attribute 'test' Our use of setuptools.command.test.test might be remnant of the past though. By defining pytest as an alias of test and running python setup.py test, we shouldn't have been running setuptools test at all. Also, setuptools test seems to use unittest internally, which we don't do. As test command is removed in setuptools 72.0.0 and reason for our own override in setup.py is gone, we just drop the line altogether. [1]scikit-build/scikit-build@dde5e79
.github/workflows/wheels.yaml
Outdated
| @@ -68,7 +68,7 @@ jobs: | |||
| # musllinux arch skip: unlikely someone wants to run dlisio on alpine on non-standard architecture | |||
| # musllinux 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 cp38*-musllinux* cp38*-macosx_*:arm64 cp36-* cp37-* | |||
| CIBW_SKIP: pp* *-musllinux_i686 *-musllinux_aarch64 cp38*-musllinux* cp38*-macosx_*arm64 cp36-* cp37-* | |||
There was a problem hiding this comment.
Are we not building and testing on MacOS arm64?
There was a problem hiding this comment.
Only for python 3.8: as commend above says, "cibuildwheel can't test it"
Wheel is still build, even though it can't be tested. Looks like wheel indication we used in cibuildwheel doesn't match the real wheel. Original intent was not to build such wheels.
This is not the same as other workflows: - core.yaml builds with python turned off - wheels.yaml builds cpp core separately and then builds wheels - docs.yaml uses python build system, but no tests - bigendian.yaml is the one also doing tests, but it is slow and runs on some oldish dependencies So additional workflow is the best option to test python build.
Using setup.py to run commands is deprecated. Currently remove test command only as it is explicitly deprecated. pytest-runner was used to run pytests through setuptools, but now it is no longer needed.
Python is in pre-release version, cibuildwheel already included it in the matrix, but not all dependencies are yet in place (looks like numpy wheels are not yet released for python 3.13).
Planning to test wheels before making a release, but think it should be ok.
Update:
Change is in 72.0, now 70.1 or something near it is picked up, so surprise might yet to come
https://pytest-runner.readthedocs.io/en/latest/index.html
https://packaging.python.org/en/latest/discussions/setup-py-deprecated/
Last two points rather not in this PR.
Update 2:
Unknown distribution option: 'tests_require', but I am not sure if it is our test_require problem or if it is skbuild problem or if it is still setuptools didn't revert something back. I think that even if I don't pin setuptools, nothing should fail here (bigendian anyway using old version). But the issue should be addressed later anyway.Update 3:
Seems like it is our test-require that breaks setuptools. If whole "remove python setup.py test" thing is done, looks like new setuptools works fine.