explosion / cython-blis

💥 Fast matrix-multiplication as a self-contained Python library – no system dependencies!
Other
219 stars 37 forks source link

Infer architecture using platform.machine() #22

Closed bennuttall closed 3 years ago

bennuttall commented 4 years ago

I wondered if it could be possible to infer the architecture in setup.py by using platform.machine() from the standard library?

The environment variable could remain as an override option.

Example proposed change:

def get_arch_name(self):
    if "BLIS_ARCH" in os.environ:
        return os.environ["BLIS_ARCH"]
    else:
        return platform.machine()

I noticed that platform is imported in setup.py but never used, so I looked at the commit (28cfda44905c46d951727bb1516fd1bb26636a6e) that added it and found that you used to do something similar:

def get_arch_name(self):
    if 'BLIS_ARCH' in os.environ:
        return os.environ['BLIS_ARCH']
    processor = platform.processor()
    if processor == 'x86_64':
        return 'haswell' # Best guess?
    else:
        return 'reference'

Background: I run a Raspberry Pi package repo at piwheels.org and it was reported that the blis package had failed builds. Looking into it I saw that BLIS_ARCH="generic" is required (not really doable for piwheels' automation). Issue: https://github.com/piwheels/packages/issues/12

bennuttall commented 4 years ago

I ran platform.machine() on Mac and Linux before posting this, and they returned x86_64. I've just run it on Windows and it returned AMD64 for me. So I guess that might be why you don't do it. But if we could be sure we knew this was the only edge case (or we knew of any others), maybe we could dict lookup this instead?

def get_arch_name(self):
    supported_archs = ('x86_64', )
    archs = {
        'AMD64': 'x86_64',
    }
    if "BLIS_ARCH" in os.environ:
        return os.environ["BLIS_ARCH"]
    else:
        platform = platform.machine()
        arch = archs.get(platform, arch)
        if arch in supported_archs:
            return arch
        else:
            return 'generic'
Impelon commented 4 years ago

If I am not mistaken this should be merged with #13 There I mentioned platform.machine as a way to extract some crude information about the used platform along with some other alternatives.

honnibal commented 4 years ago

I think I'm opposed to this, or at least I'd like it to be behind a different environment variable like BLIS_ARCH="auto"

Can you just set the BLIS_ARCH environment variable explicitly if you're compiling from source? Most users use the library from the wheel, and so they don't get benefit from this either way.

The problem is it's just really hard to test the logic that does the hardware detection, as basically nobody is going to have the test rig that exercises all the combinations of OS and hardware. The person submitting the patch normally can't test it, the CI can't test it, and I can't test it --- which means it's really likely to break.

bennuttall commented 4 years ago

Can you just set the BLIS_ARCH environment variable explicitly if you're compiling from source? Most users use the library from the wheel, and so they don't get benefit from this either way.

I run a project which builds Arm platform wheels for raspberry pi. It's automated to build everything by running pip wheel so not feasible to use env vars for some projects.

honnibal commented 4 years ago

Hmm okay. I see how having to do special-case stuff per library is bad. Would it help if we made it a setup arg? I think that can be put into a requirements file.

The good news is that the Python packaging ecosystem has now rolled out multilinux2014 containers, so I've managed to update our wheel-building machinery to use the new format. I'm hoping this means we'll be able to build and distribute ARM wheels on pip.

adrianeboyd commented 3 years ago

I think we can do a better job here for linux at least. I definitely don't think the default should be "x86_64", which doesn't even compile for gcc 8 currently. I think we can detect whether the compiler supports particular flags and do reasonably well here: https://github.com/explosion/cython-blis/pull/44

I also can't test with every possible combination and this may still break in some cases, but I think it should work for a much larger percentage of typical users who are compiling from source without requiring extra settings. I tested it for the conda-forge builds (x86_64, aarch64, ppc64le) and the arch detection and flag detection worked as expected (x86_64_no_znver2 for gcc 7 / cortexa57 / power9).

The current status where the default pip install blis from source fails on many typical x86_64 systems is particularly bad.

bennuttall commented 3 years ago

Thanks @adrianeboyd! I will keep an eye out when a release is made and see if the piwheels build succeeds. Others can keep track here: https://www.piwheels.org/project/blis/

adrianeboyd commented 3 years ago

I can build wheels without issues locally on a Raspberry Pi 4, so I hope it's all much easier now!