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

Installation problem in mipsel and mips64el (caused by cpuinfo) #257

Closed
avalentino opened this issue Nov 17, 2020 · 11 comments
Closed

Installation problem in mipsel and mips64el (caused by cpuinfo) #257

avalentino opened this issue Nov 17, 2020 · 11 comments

Comments

@avalentino
Copy link

Minimal, reproducible code sample, a copy-pastable example if possible

$ python3.8 setup.py config 
Traceback (most recent call last):
  File "setup.py", line 4, in <module>
    import cpuinfo
  File "/<<PKGBUILDDIR>>/cpuinfo.py", line 2406, in <module>
    _check_arch()
  File "/<<PKGBUILDDIR>>/cpuinfo.py", line 231, in _check_arch
    raise Exception("py-cpuinfo currently only works on X86 and some ARM/PPC/S390X CPUs.")
Exception: py-cpuinfo currently only works on X86 and some ARM/PPC/S390X CPUs

Problem description

Some of the tests run on the Debian CI infrastructure are failing on mipsel and mips64el architectures [1, 2, 3].
The problem is related to the installation phase and clearly caused by cpuinfo the does not support those architectures.

Not sure if updating cpuinfo would help.

My plan is to prepare a small patch for the Debian packaging script to ignore the failure and set both have_sse2 and have_avx2 to False.

I would appreciate some comments/hints form developers.

Also if it is considered of interested for the project I can submit a PR (assuming that the fix works on Debian).

[1] https://buildd.debian.org/status/fetch.php?pkg=numcodecs&arch=mipsel&ver=0.7.2%2Bds-1&stamp=1601287342&raw=0
[2] https://buildd.debian.org/status/fetch.php?pkg=numcodecs&arch=mips64el&ver=0.7.2%2Bds-1&stamp=1601287778&raw=0
[3] https://buildd.debian.org/status/package.php?p=numcodecs

Version and installation information

Please provide the following:

  • Value of numcodecs.__version__: 0.7.2
  • Version of Python interpreter: 3.8.6
  • Operating system (Linux/Windows/Mac): GNU/Linix (Debian sid)
  • How NumCodecs was installed (e.g., "using pip into virtual environment", or "using conda"): setup.py
@jeromekelleher
Copy link
Member

I can't comment on the technical details here @avalentino, but I just wanted to weigh in and say thanks. It's super exciting to see numcodecs going in to Debian!

@joshmoore
Copy link
Member

joshmoore commented Nov 18, 2020

Last update seems to be:

commit c5becae
Author: John Kirkham
Date: Sun Nov 3 18:43:48 2019 -0500

for s390x support, but I don't see any further additions in https://github.com/workhorsy/py-cpuinfo/blob/master/ChangeLog.

@jakirkham, for my own benefit, could you explain the background of the vendoring itself?

@jakirkham
Copy link
Member

It’s used by the setup.py script at build time. Historically Python hasn’t had a good mechanism for requesting build time dependencies be installed. Also the project that this file comes from has everything contained in this file. So it’s pretty easy to vendor.

If there’s been an update upstream that fixes this issue, a PR upgrading this file would be welcome. If not, we might want to raise an upstream issue with them as well.

@jakirkham
Copy link
Member

Also this is the PR ( #202 ) where that commit came from for more context

@jakirkham
Copy link
Member

cc @QuLogic (in case you have any thoughts here 🙂)

avalentino added a commit to avalentino/numcodecs that referenced this issue Nov 21, 2020
avalentino added a commit to avalentino/numcodecs that referenced this issue Nov 21, 2020
@vincentsarago
Copy link

It’s used by the setup.py script at build time. Historically Python hasn’t had a good mechanism for requesting build time dependencies be installed. Also the project that this file comes from has everything contained in this file. So it’s pretty easy to vendor.

👋 sorry, and feel free to ignore if you want. but using pyproject.toml you can easily add build time dependencies

More on pyproject.toml https://snarky.ca/clarifying-pep-518/

e.g https://github.com/mapbox/rasterio/blob/master/pyproject.toml#L1-L2

[build-system]
requires = ["setuptools", "cython", "py-cpuinfo"]

@jakirkham
Copy link
Member

We vendored it to save people from installing it and to make sure we get a version (or commit) that works for us and avoid any that have issues. It's one file so this is pretty easy to update

Cython isn't required as we include the C files

Setuptools comes with pip and is in basically every Python environment these days

@vincentsarago
Copy link

@jakirkham I understand, feel free to close the PR.

TBH, I'm hitting #268 and I was confuse where to fix. there is workhorsy/py-cpuinfo#161 which might fix the issue ... but then it will have to be updated also on this repo. To me because, it's only a one file python module, I think it's a good reason why not vendoring it ... but that's my personal opinion.

Cython isn't required as we include the C files
Setuptools comes with pip and is in basically every Python environment these days

sure this is just an example

@jakirkham
Copy link
Member

Right so that is exactly the sort of situation where I would expect vendoring to help. Does using those changes fix the issue for you? If so, would you be able to open a PR including those changes in the vendored file?

@jakirkham
Copy link
Member

jakirkham commented Apr 8, 2021

Should add Matthew just reviewed and merged that py-cpuinfo PR

@jakirkham
Copy link
Member

Think this should have been addressed by PR ( #280 ). Please feel free to open a new issue if other problems persist. Thanks!

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

5 participants