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

(#12705) OpenBlas: Add arch target option #13478

Closed

Conversation

aborzunov
Copy link
Contributor

Specify library name and version: openblas/*

Now we can specify target architecture with recipe options and not rely on hardcoded in openblas cmake auto detection utilities.

closes #12705


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@aborzunov aborzunov force-pushed the openblas-target-arch-#12705 branch 2 times, most recently from b34dc8b to 6513bb2 Compare October 14, 2022 12:32
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Failure in build 4 (fc9f86bd9a24d95bd6ca56c20c7b783feabed1b3):

  • openblas/0.3.15@:
    Didn't run or was cancelled before finishing

  • openblas/0.3.17@:
    Didn't run or was cancelled before finishing

  • openblas/0.3.20@:
    CI failed to create some packages (All logs)

    Logs for packageID 49722bfe29e816b31c8cb5aefe12da1791066439:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=9
    os=Linux
    [options]
    openblas:shared=False
    
    [...]
        $ conan profile update settings.compiler.libcxx=libstdc++11 default
    
    Or edit '/home/conan/w/prod/BuildSingleReference/.conan/profiles/default' and set compiler.libcxx=libstdc++11
    
    ************************************************************************************
    
    
    
    Default settings
    	os=Linux
    	os_build=Linux
    	arch=x86_64
    	arch_build=x86_64
    	compiler=gcc
    	compiler.version=9
    	compiler.libcxx=libstdc++
    	build_type=Release
    *** You can change them in /home/conan/w/prod/BuildSingleReference/.conan/profiles/default ***
    *** Or override with -s compiler='other' -s ...s***
    
    
    Configuration:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=9
    os=Linux
    [options]
    openblas:shared=False
    [build_requires]
    [env]
    [conf]
    tools.system.package_manager:mode=install
    tools.system.package_manager:sudo=True
    
    openblas/0.3.20: Forced build from source
    Installing package: openblas/0.3.20
    Requirements
        openblas/0.3.20 from local cache - Cache
    Packages
        openblas/0.3.20:49722bfe29e816b31c8cb5aefe12da1791066439 - Build
    
    Installing (downloading, building) binaries...
    [HOOK - conan-center.py] pre_source(): [IMMUTABLE SOURCES (KB-H010)] OK
    openblas/0.3.20: Configuring sources in /home/conan/w/prod/BuildSingleReference/.conan/data/openblas/0.3.20/_/_/source
    ERROR: openblas/0.3.20: Error in source() method, line 202
    	destination=self._source_subfolder
    	TypeError: get() missing 1 required positional argument: 'conanfile'
    
  • openblas/0.3.13@:
    Didn't run or was cancelled before finishing

  • openblas/0.3.10@:
    Didn't run or was cancelled before finishing

  • openblas/0.3.12@:
    Didn't run or was cancelled before finishing


Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

I'm not convinced about this change. It is not a configuration to be applied as -march or -mtune to the compiler? It looks more a setting than an option. In case it's parsed directly as a compiler flags, it can be configured directly by global.conf with tools.build

@nenomius
Copy link

Agreed with @uilianries

Conan needs something like this: https://spack.readthedocs.io/en/latest/basic_usage.html#support-for-specific-microarchitectures

Let's implement it in this PR. I think you can do this in a weekend or two.

@uilianries
Copy link
Member

@aborzunov Microarch is a complex feature listed here: conan-io/conan#847 Please, feel free to comment there your thought. So far, the best approach is customizing your settings.yml: https://docs.conan.io/en/latest/extending/custom_settings.html#adding-new-sub-settings

@aborzunov
Copy link
Contributor Author

aborzunov commented Oct 17, 2022

@uilianries

As OpenBlas documentation says, they have special option for cmake (and make) for case, when compiling for specific architecture is required. If this option is not passed to cmake, OpenBlas build scripts will ignore anything and sets compile flags as it wish. Thus, if our build server has AVX512 it will deploy broken code to clients (e.g. with haswell).

I don't quite understand why should we substitute openblas author's cmake option with settings and how should I modify recipe in this PR to deploy arch choice functionality for other conan users, if I'll be modifying local .settings.yml.

I can do something like this in _configure_cmake(self):

if self.settings.get_safe("micro_arch_target") != "None":
             cmake.definitions["TARGET"] = self.settings. micro_arch_target

And leave a comprehensive in-code comment about what is this and that user should modify local setting.yml to make it work.
This approach will hide all this functionality until user obtain broken package and dive into source code to examine it.

@uilianries
Copy link
Member

@aborzunov Adding a custom setting in the recipe is not an option, as it's not documented.

However, you could use validate() to check if self.info.settings.arch is aligned with that option. I don't think makes sense using option value POWER10 and arch x86_64

@stale
Copy link

stale bot commented Nov 19, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 19, 2022
@stale
Copy link

stale bot commented Dec 20, 2022

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Dec 20, 2022
@lukaumi lukaumi mentioned this pull request Feb 5, 2023
3 tasks
@tttapa tttapa mentioned this pull request Jun 6, 2024
3 tasks
@elvisdukaj
Copy link
Contributor

Agreed with @uilianries

Conan needs something like this: https://spack.readthedocs.io/en/latest/basic_usage.html#support-for-specific-microarchitectures

Let's implement it in this PR. I think you can do this in a weekend or two.

Agreed with @uilianries

Conan needs something like this: https://spack.readthedocs.io/en/latest/basic_usage.html#support-for-specific-microarchitectures

Let's implement it in this PR. I think you can do this in a weekend or two.

Maybe it’s worth considering creating an MR on Conan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenBLAS] Target CPU autodetection makes binaries undeployable
5 participants