openblas: Improve recipe plus conan v2 #17173
Conversation
joakimono
force-pushed
the
openblas/modernize-for-v2
branch
from
7061848 to
fc62160
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit fc62160openblas/0.3.17openblas/0.3.20openblas/0.3.10openblas/0.3.13openblas/0.3.15openblas/0.3.12openblas/0.3.23 |
| @@ -19,134 +23,221 @@ class OpenblasConan(ConanFile): | |||
| "fPIC": [True, False], | |||
| "build_lapack": [True, False], | |||
| "use_thread": [True, False], | |||
| "use_openmp": [True, False], | |||
| "dynamic_arch": [True, False], | |||
| } | |||
| default_options = { | |||
| "shared": False, | |||
| "fPIC": True, | |||
| "build_lapack": False, | |||
There was a problem hiding this comment.
I'd suggest if it's possible to build lapack without fortran (I thought this was just a C api that ultimately still needed to link to the fortran libs?), then we should build it by default.
Because of the requirement that all CCI packages must build with the defaults of their dependencies, this opens the door to a number of scientific packages that choke without lapack support.
| "build_lapack": False, | |
| "build_lapack": True, |
There was a problem hiding this comment.
Since openblas v0.3.21, it is possible to build with LAPACK support without Fortran. A f2c (fortran to c) converted copy of LAPACK 3.9.0 is bundled, which completely avoids the need for a Fortran runtime, see https://github.com/xianyi/OpenBLAS/releases/tag/v0.3.21 why 3.9.0 and not newer. This allows us to provide LAPACK on CCI without a Fortran compiler. One related issues is #4509.
There was a problem hiding this comment.
I have added a check in config_options() that changes the default build_lapack to True if the version is new enough. Note that the recipe then will have different default options depending on package version. I do not know if this against CCI policy.
There was a problem hiding this comment.
If that's the case, I'm not sure it's desirable to introduce different defaults across versions. My intuition would be to keep it stable across versions (so keep "build_lapack": False), but this has to be balanced with the benefits of having lapack available to support other conan center packages. @uilianries @prince-chrismc can either of you weigh in here?
There was a problem hiding this comment.
Reminder to possibly weigh in regarding this suggested change. @uilianries @prince-chrismc.
There was a problem hiding this comment.
This has been scheduled for discussion with the team, thanks a lot for your patience
There was a problem hiding this comment.
Any update on this topic? I'm willing to forego setting "build_lapack": True for >=0.3.21, alternatively, build both [True, False] (if possible) if this could expedite the merging of this pull request.
joakimono
force-pushed
the
openblas/modernize-for-v2
branch
from
fc62160 to
b58023d
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Allow disabling AVX512 via env variable, workaround for conan-io#12705 - Only make gfortran a dependency if actually used, related to conan-io#5910, conan-io#1867, conan-io#5338 - Add latest OpenBLAS release version 0.3.23 - Move replace_in_file to patches instead - Fix intel fortran compiler identification in OpenBLAS cmake - Allow build_lapack without fortran using C_LAPACK since 0.3.23
- Disable AVX512 by default: builld servers usually have this, but most consumer PCs do not
- Set build_lapack=True when it can be built without Fortran
joakimono
force-pushed
the
openblas/modernize-for-v2
branch
from
3daa05b to
7c334c4
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 7c334c4openblas/0.3.20openblas/0.3.15openblas/0.3.17openblas/0.3.10openblas/0.3.13openblas/0.3.12openblas/0.3.23 |
- Allow disabling AVX512 via env variable, workaround for conan-io#12705 - Only make gfortran a dependency if actually used, related to conan-io#5910, conan-io#1867, conan-io#5338 - Add latest OpenBLAS release version 0.3.23 - Move replace_in_file to patches instead - Fix intel fortran compiler identification in OpenBLAS cmake - Allow build_lapack without fortran using C_LAPACK since 0.3.23
- Disable AVX512 by default: builld servers usually have this, but most consumer PCs do not
- Set build_lapack=True when it can be built without Fortran
joakimono
force-pushed
the
openblas/modernize-for-v2
branch
from
7c334c4 to
99f8b27
Compare
|
@joakimono I think force pushing impacts review state and makes it difficult to see the changes you're making, so i think it's preferable to just fix with commits over the top. It's all squashed when merged anyway |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit b508230openblas/0.3.20openblas/0.3.17openblas/0.3.15openblas/0.3.12openblas/0.3.10openblas/0.3.13openblas/0.3.23 |
This comment has been minimized.
This comment has been minimized.
|
I am very happy to see that all the checks have passed. I have been following for a long time, and I am really looking forward to this PR being merged as soon as possible. |
This comment has been minimized.
This comment has been minimized.
|
@uilianries @RubenRBS looks like the ci on this one has been down for a fair while - any chance of a nudge? Looks like it's ready for re-review as well. Would be good to get this one in. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Hi!
Thank you for your patience and contributions to improve the recipe.
@RubenRBS and I have been taking a closer look at this and came up with the following ideas:
- Changing the default option "build_lapack" is something that should be avoided, as this is changing the expectations for consumers.
- Adding the Fortran dependency, even the Fortran runtime, adds a lot of complexity and the compiler is something that is not covered on CCI so it will be difficult to test and maintain.
- Changes in the recipe are so big and affect the old versions of the library with too many patches that make it difficult to review and maintain.
Our suggestions go more along the line:
- Creating a different conanfile.py for the new changes to avoid dealing with the different quirks of older versions.
- As commented above, since version 0.3.21 the requirement for Fortran is not needed, so this new conanfile should support 0.3.21 onwards only to make things easier.
We hope that the suggestions make sense and thanks again for your patience.
|
Again, thanks for your patience, I'd like to mention that in the future, it's usually faster to stagger PR changes into multiple PRs instead of a big one like this, as it takes us less time to understand them if they their size is smaller :) |
|
@danimtb, @RubenRBS : Should I apply the suggested simplifications here, or create a separate PR? |
Conan v1 pipeline ✔️All green in build 4 (
Conan v2 pipeline ✔️
All green in build 4 (
|
@joakimono given the response times for the conan team recently, my suggestion would be:
For what it's worth this decision doesn't make a lot of sense to me - this isn't a fork of openblas, it just enables an alternate configuration. The only real change as far as the user is concerned is the change in default parameter (which doesn't have to be the case). I think that creating a new recipe will be more confusing to users rather than less, especially given the inability to document what recipes are for or how/when they should be used. My personal opinion is that the benefit provided by changing the default parameter in the latest version far outweighs the downside of changing a default parameter. Still, I think it's pretty important to get a conan 2 compatible version of openblas in ASAP, so my suggestion would be to proceed as above and we can defer a more detailed conversation about where fortran compatibility belongs to a separate PR. |
|
@samuel-emrys thanks for your suggestions above and the general intention to move this PR forward. Just for clarification, I am not suggesting creating a new reference with the fortran improvements, but instead, keep them under the same I know that could lead to having to maintain 2 conanfiles.py, but I think the chanches since 0.3.21 are significantly important to start with a new recipe and it will ease the maintenance and review of it. Just how I see it. |
Ah, understood. I misinterpreted your original statement to mean the creation of a new |
|
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. |
ghost
mentioned this pull request
ghost
mentioned this pull request
|
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
Specify library name and version: openblas/*