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

blasfeo: add package #15643

Closed
wants to merge 2 commits into from
Closed

blasfeo: add package #15643

wants to merge 2 commits into from

Conversation

lukaumi
Copy link

@lukaumi lukaumi commented Feb 1, 2023

Library name and version: blasfeo/0.1.3

https://github.com/giaf/blasfeo

Additional install documentation: https://blasfeo.syscop.de/docs/install/

Built with make as it is primary build system for the library

Python code formatted with black
C code formatted with clang-format


@CLAassistant
Copy link

CLAassistant commented Feb 1, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@lukaumi lukaumi closed this Feb 1, 2023
@lukaumi lukaumi reopened this Feb 1, 2023
@lukaumi lukaumi changed the title Add blasfeo package blasfeo: add package Feb 1, 2023
@lukaumi lukaumi closed this Feb 2, 2023
@lukaumi lukaumi reopened this Feb 2, 2023
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

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.

Thank you for your contribution, I'm curious why don't you use the cmake support from the project instead of parsing manually all thos options https://github.com/giaf/blasfeo/blob/0.1.3/CMakeLists.txt

recipes/blasfeo/all/conanfile.py Outdated Show resolved Hide resolved
options = {
"library_type": ["static_library", "shared_library"],
"fPIC": [True, False],
"target": [
Copy link
Member

Choose a reason for hiding this comment

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

It seems part of target architecture, so it's part of settings, not an option.

Comment on lines 10 to 82
printf("Testing processor\n");

char supportString[50];
blasfeo_processor_library_string(supportString);
printf("Library requires processor features:%s\n", supportString);

int features = 0;
int procCheckSucceed = blasfeo_processor_cpu_features(&features);
blasfeo_processor_feature_string(features, supportString);
printf("Processor supports features:%s\n", supportString);

if (!procCheckSucceed)
{
printf("Current processor does not support the current compiled BLASFEO library.\n");
printf("Please get a BLASFEO library compatible with this processor.\n");
exit(3);
}

int ii; // loop index

int n = 12; // matrix size

// A
struct blasfeo_dmat sA; // matrix structure
blasfeo_allocate_dmat(n, n, &sA); // allocate and assign memory needed by A

// B
struct blasfeo_dmat sB; // matrix structure
int B_size = blasfeo_memsize_dmat(n, n); // size of memory needed by B
void *B_mem_align;
v_zeros_align(&B_mem_align, B_size); // allocate memory needed by B
blasfeo_create_dmat(n, n, &sB, B_mem_align); // assign aligned memory to struct

// C
struct blasfeo_dmat sC; // matrix structure
int C_size = blasfeo_memsize_dmat(n, n); // size of memory needed by C
C_size += 64; // 64-bytes alignment
void *C_mem = malloc(C_size);
void *C_mem_align = (void *)((((unsigned long long)C_mem) + 63) / 64 * 64); // align memory pointer
blasfeo_create_dmat(n, n, &sC, C_mem_align); // assign aligned memory to struct

// A
double *A = malloc(n * n * sizeof(double));
for (ii = 0; ii < n * n; ii++)
{
A[ii] = ii;
}
int lda = n;
blasfeo_pack_dmat(n, n, A, lda, &sA, 0, 0); // convert from column-major to BLASFEO dmat
free(A);

// B
blasfeo_dgese(n, n, 0.0, &sB, 0, 0); // set B to zero
for (ii = 0; ii < n; ii++)
{
BLASFEO_DMATEL(&sB, ii, ii) = 1.0; // set B diagonal to 1.0 accessing dmat elements
}

// C
blasfeo_dgese(n, n, -1.0, &sC, 0, 0); // set C to -1.0

blasfeo_dgemm_nt(n, n, n, 1.0, &sA, 0, 0, &sB, 0, 0, 0.0, &sC, 0, 0, &sC, 0, 0);

printf("\nC = \n");
blasfeo_print_dmat(n, n, &sC, 0, 0);

blasfeo_free_dmat(&sA);
v_free_align(B_mem_align);
free(C_mem);

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printf("Testing processor\n");
char supportString[50];
blasfeo_processor_library_string(supportString);
printf("Library requires processor features:%s\n", supportString);
int features = 0;
int procCheckSucceed = blasfeo_processor_cpu_features(&features);
blasfeo_processor_feature_string(features, supportString);
printf("Processor supports features:%s\n", supportString);
if (!procCheckSucceed)
{
printf("Current processor does not support the current compiled BLASFEO library.\n");
printf("Please get a BLASFEO library compatible with this processor.\n");
exit(3);
}
int ii; // loop index
int n = 12; // matrix size
// A
struct blasfeo_dmat sA; // matrix structure
blasfeo_allocate_dmat(n, n, &sA); // allocate and assign memory needed by A
// B
struct blasfeo_dmat sB; // matrix structure
int B_size = blasfeo_memsize_dmat(n, n); // size of memory needed by B
void *B_mem_align;
v_zeros_align(&B_mem_align, B_size); // allocate memory needed by B
blasfeo_create_dmat(n, n, &sB, B_mem_align); // assign aligned memory to struct
// C
struct blasfeo_dmat sC; // matrix structure
int C_size = blasfeo_memsize_dmat(n, n); // size of memory needed by C
C_size += 64; // 64-bytes alignment
void *C_mem = malloc(C_size);
void *C_mem_align = (void *)((((unsigned long long)C_mem) + 63) / 64 * 64); // align memory pointer
blasfeo_create_dmat(n, n, &sC, C_mem_align); // assign aligned memory to struct
// A
double *A = malloc(n * n * sizeof(double));
for (ii = 0; ii < n * n; ii++)
{
A[ii] = ii;
}
int lda = n;
blasfeo_pack_dmat(n, n, A, lda, &sA, 0, 0); // convert from column-major to BLASFEO dmat
free(A);
// B
blasfeo_dgese(n, n, 0.0, &sB, 0, 0); // set B to zero
for (ii = 0; ii < n; ii++)
{
BLASFEO_DMATEL(&sB, ii, ii) = 1.0; // set B diagonal to 1.0 accessing dmat elements
}
// C
blasfeo_dgese(n, n, -1.0, &sC, 0, 0); // set C to -1.0
blasfeo_dgemm_nt(n, n, n, 1.0, &sA, 0, 0, &sB, 0, 0, 0.0, &sC, 0, 0, &sC, 0, 0);
printf("\nC = \n");
blasfeo_print_dmat(n, n, &sC, 0, 0);
blasfeo_free_dmat(&sA);
v_free_align(B_mem_align);
free(C_mem);
return 0;
char supportString[50];
blasfeo_processor_library_string(supportString);
printf("Library requires processor features:%s\n", supportString);
return 0;

Please, use something smaller: https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/test_packages.md#minimalist-source-code

Copy link
Author

Choose a reason for hiding this comment

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

I though of just instantiating some objects, but I am not sure if it would be 100% reliable.
E.g. when I used only code before the line int ii; // loop index, everything compiled and linked ok in test, but in fact there was linker errors after using other methods (which was solved by adding self.cpp_info.system_libs = ["m"]).
This example is the one from the package and actually runs matrix computation and outputs the result.

@conan-center-bot

This comment has been minimized.

@uilianries
Copy link
Member

@lukaumi Those configurations listed as options, are actually part of settings, because you are cross-building. So, you can not use options for that, otherwise, who consumes this package, would have a configuration mismatch, as Conan uses settings.arch to validate the architecture and match the package ID. It's not only a profile matter, but also how Conan works.

What you can do is using self.settings to translate the current self.settings.arch to the current target name that you expect. As the regular Conan settings does not cover all your cases, you still can expect from a custom setting, like self.settings.cpu.family`

More about custom settings: https://docs.conan.io/en/latest/extending/custom_settings.html

Of course, your case is not the first, so you can borrow some idea from OpenSSL recipe: https://github.com/conan-io/conan-center-index/blob/master/recipes/openssl/1.x.x/conanfile.py#L336

@lukaumi lukaumi marked this pull request as draft February 2, 2023 15:49
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@lukaumi lukaumi marked this pull request as ready for review February 5, 2023 16:46
@lukaumi
Copy link
Author

lukaumi commented Feb 5, 2023

While I agree that the target value usage with this library has to align with OS, architecture, compiler, etc. to make sense, I have some concerns/questions about introducing library-specific flags to external settings. Instead of settings, I've mapped target values to arch and added checks in validate() method. I feel that keeping library-available options inside the recipe gives more information to end user. Is this a valid option?

I've found the issue #13478 which aligns with what I am trying to do here.
@uilianries

@lukaumi
Copy link
Author

lukaumi commented Feb 6, 2023

Also, this PR is not picked-up by the bot for review list after all reopening/errors/changes? How can I re-trigger that @prince-chrismc?

- required as a dependency for hpipm 0.1.3 version to be added later
@uilianries
Copy link
Member

I have some concerns/questions about introducing library-specific flags to external settings

It can be added to configuration file (which is supported by profile too): https://docs.conan.io/en/latest/reference/config_files/global_conf.html

Which means, you can create a specific profile, aligned to your target, and customize any compiler flag that you need. For instance:

[settings]
os=Linux
os_build=Linux
arch=armv8
arch.cpu=a76
arch_build=x86_64
compiler=apple-clang
compiler.version=14
compiler.libcxx=libc++
build_type=Release
[options]
[conf]
tools.build:cflags=-march=armv8-a,-mtune=cortex-a76
tools.build:cxxflags=-march=armv8-a,-mtune=cortex-a76
[build_requires]
[env]
CC=/path/to/my/cross/compiler/gcc
CXX=/path/to/my/cross/compiler/g++

I've found the issue #13478 which aligns with what I am trying to do here.

Indeed, but need to consider new features maturated since there and we are even closer to Conan v2, which will enforce configuration file even more.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 17 (c0d956f0e3789f491b18a9dd5ee9dbf2ac8274d0):

  • blasfeo/0.1.3@:
    Didn't run or was cancelled before finishing

  • blasfeo/0.1.2@:
    CI failed to create some packages (All logs)

    Logs for packageID b9b8165bec6f88c9ccae6dda4fd0dcc267a80738:
    [settings]
    arch=x86_64
    build_type=Debug
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=10
    os=Linux
    [options]
    blasfeo:shared=False
    
    [...]
    -----------------
    -- The C compiler identification is GNU 10.3.0
    -- Detecting C compiler ABI info
    -- Detecting C compiler ABI info - done
    -- Check for working C compiler: /usr/bin/gcc - skipped
    -- Detecting C compile features
    -- Detecting C compile features - done
    -- Conan: called by CMake conan helper
    -- Conan: Adjusting output directories
    -- Conan: Using cmake targets configuration
    -- Library blasfeo found /home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/package/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/lib/libblasfeo.a
    -- Conan: Adjusting default RPATHs Conan policies
    -- Conan: Adjusting language standard
    -- This project seems to be plain C, using 'GNU' compiler
    -- Conan: Compiler GCC>=5, checking major version 10
    -- Conan: Checking correct version: 10
    -- Conan: C++ stdlib: libstdc++11
    -- Library blasfeo found /home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/package/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/lib/libblasfeo.a
    -- Found: /home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/package/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/lib/libblasfeo.a
    -- Configuring done
    -- Generating done
    -- Build files have been written to: /home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/blasfeo/all/test_v1_package/build/b5c4be2b65220be2e1a246cad75f09301645354d
    
    ----Running------
    > cmake --build '/home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/blasfeo/all/test_v1_package/build/b5c4be2b65220be2e1a246cad75f09301645354d' '--' '-j3'
    -----------------
    Scanning dependencies of target test_package
    [ 50%] Building C object CMakeFiles/test_package.dir/home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/blasfeo/all/test_package/test_package.c.o
    [100%] Linking C executable bin/test_package
    CMake Warning:
      Manually-specified variables were not used by the project:
    
        CMAKE_EXPORT_NO_PACKAGE_REGISTRY
        CMAKE_INSTALL_BINDIR
        CMAKE_INSTALL_DATAROOTDIR
        CMAKE_INSTALL_INCLUDEDIR
        CMAKE_INSTALL_LIBDIR
        CMAKE_INSTALL_LIBEXECDIR
        CMAKE_INSTALL_OLDINCLUDEDIR
        CMAKE_INSTALL_SBINDIR
    
    
    /usr/bin/ld: /home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/package/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/lib/libblasfeo.a(blasfeo_processor_features.o):/home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/build/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/auxiliary/../include/blasfeo_processor_features.h:55: multiple definition of `BLASFEO_PROCESSOR_FEATURES'; CMakeFiles/test_package.dir/home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/blasfeo/all/test_package/test_package.c.o:/home/conan/w/prod/BuildSingleReference/.conan/data/blasfeo/0.1.2/_/_/package/b9b8165bec6f88c9ccae6dda4fd0dcc267a80738/include/blasfeo_processor_features.h:55: first defined here
    collect2: error: ld returned 1 exit status
    make[2]: *** [CMakeFiles/test_package.dir/build.make:104: bin/test_package] Error 1
    make[1]: *** [CMakeFiles/Makefile2:95: CMakeFiles/test_package.dir/all] Error 2
    make: *** [Makefile:103: all] Error 2
    ERROR: blasfeo/0.1.2 (test package): Error in build() method, line 14
    	cmake.build()
    	ConanException: Error 2 while executing cmake --build '/home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/blasfeo/all/test_v1_package/build/b5c4be2b65220be2e1a246cad75f09301645354d' '--' '-j3'
    

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.


Conan v2 pipeline (informative, not required for merge) ❌

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future.

See details:

Failure in build 16 (c0d956f0e3789f491b18a9dd5ee9dbf2ac8274d0):

An unexpected error happened and has been reported. Help is on its way! 🏇

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

This is very odd but thats upstreams fault I think thisnis fine 🙂

"0.1.3":
url: "https://github.com/giaf/blasfeo/archive/refs/tags/0.1.3.tar.gz"
sha256: "c33eb6467a90d6075a8db34e5ab239ecdda38c5261ae096def122fe7a0f98780"
"0.1.2":
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you specifically need this version we generally don't add older versions (you can check the FAQ for more info)

Copy link
Author

Choose a reason for hiding this comment

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

This library blasfeo is a main dependency of another one hpipm which I plan to add later.
I tried building locally hpipm with blasfeo 0.1.3 version as a dependency, but apparently they are not compatible, which is not documented anywhere in either library.
Latest hpipm release works only with blasfeo 0.1.2 so I've added it here also.

Copy link
Author

Choose a reason for hiding this comment

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

Plus, apparently tests are not compatible with older version so I have to fix that also.

options = {
"fPIC": [True, False],
"shared": [True, False],
"target": [x for list in [i for i in _blasfeo_arch_targets.values()] for x in list],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very awkward way of packing softwarehttps://github.com/giaf/blasfeo#supported-computer-architectures

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is. I will try to add something according to @uilianries suggestion.

@lukaumi
Copy link
Author

lukaumi commented Feb 7, 2023

0.1.2 build and tests are failing for some compiler combinations because of

..... /auxiliary/../include/blasfeo_processor_features.h:55: multiple definition of `BLASFEO_PROCESSOR_FEATURES';
..... /include/blasfeo_processor_features.h:55: first defined here`

and I've tracked the issue in the source.

This library turns out to be too much of a mess to migrate, and I don't have time to continue playing with this.
I've been able to generate packages for my team's compilers and configurations, and I'll host on company servers which should be enough.

If anyone wants to continue with this, feel free to use my progress.

@lukaumi lukaumi closed this Feb 7, 2023
@lukaumi lukaumi deleted the add_blasfeo_0.1.3 branch February 7, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants