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

ENH: update package version to 0.1.4 and enable microarch-enabled and arm builds for all platforms #14

Merged
merged 17 commits into from
Nov 19, 2024

Conversation

RSchwan
Copy link
Contributor

@RSchwan RSchwan commented Nov 19, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

This is a competing PR to #11 updating blasfeo and enabling microarch x86_64 and arm build on all platforms. I also took the liberty to add myself as maintainer.

Let's see if everything builds as expected...

Fix #4 .
Fix #2 .

@RSchwan RSchwan requested a review from traversaro as a code owner November 19, 2024 11:15
@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the GitHub actions workflow below for more details. You can also ping conda-forge/core for further assistance or you can try rerendering locally.

The following suggestions might help debug any issues:

  • Is the recipe/{meta.yaml,recipe.yaml} file valid?
  • If there is a recipe/conda-build-config.yaml file in the feedstock make sure that it is compatible with the current global pinnnings.
  • Is the fork used for this PR on an organization or user GitHub account? Automated rerendering via the webservices admin bot only works for user GitHub accounts.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11911901191.

recipe/meta.yaml Outdated
Comment on lines 31 to 33
# Modern arm processors (e.g. X Elite) have cache sizes more similar to the
# M1 from Apple, thus we set TARGET to ARMV8A_APPLE_M1
- BLASFEO_TARGET=ARMV8A_APPLE_M1 # [aarch64 or arm64]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am bit dubious on this, at least for Linux. Is this compatible with embedded arm boards, for example Raspberry PI or Jetson? We frequently use conda packages there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually a good point. As far as I can see in the blasfeo source code, the only difference is in the config of the L1 and L2 cache. Hence, it probably makes sense to use ARMV8A_ARM_CORTEX_A76 (Raspberry Pi) on Linux and ARMV8A_APPLE_M1 everywhere else. For Windows, the higher end chips (e.g. X Elite) are probably the more accurate targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into this. For what I understood, all the architectures anyhow have NEON and VPFv4 SIMD, so there is not risk of "illegal instructions" kind of error (that is the thing I fear the most). If it is mostly a matter of which architecture (i.e. cache size) the compilation is more optimized, I guess your suggestions make sense. If we ever need more targeted builts, in the future we could also consider making ultra-specific builds that depend on the exact __archspec , but I guess it is easier to start simple.

-DCMAKE_INSTALL_PREFIX=%LIBRARY_PREFIX% ^
-DCMAKE_BUILD_TYPE=Release ^
-DBUILD_TESTING:BOOL=ON ^
-DBLASFEO_TESTING:BOOL=ON ^
-DBLASFEO_TESTING:BOOL=OFF ^
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we disable this, did you tested that an executable on Windows can consume the produced libraries well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disabled this because of the potential arm build, but I can add it back and see if the CI has trouble there.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the problem is arm, we can add an if when we will add win-arm64 support.

@traversaro
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the GitHub actions workflow below for more details. You can also ping conda-forge/core for further assistance or you can try rerendering locally.

The following suggestions might help debug any issues:

* Is the `recipe/{meta.yaml,recipe.yaml}` file valid?

* If there is a `recipe/conda-build-config.yaml` file in the feedstock make sure that it is compatible with the current [global pinnnings](https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/master/recipe/conda_build_config.yaml).

* Is the fork used for this PR on an organization or user GitHub account? Automated rerendering via the webservices admin bot only works for user GitHub accounts.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11911901191.

Related error:

/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/environ.py:544: UserWarning: The environment variable 'BLASFEO_TARGET' is being passed through with value 'X64_INTEL_CORE'.  If you are splitting build and test phases with --no-test, please ensure that this value is also set similarly at test time.
  warnings.warn(
/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/environ.py:544: UserWarning: The environment variable 'BLASFEO_TARGET' is being passed through with value 'X64_INTEL_HASWELL'.  If you are splitting build and test phases with --no-test, please ensure that this value is also set similarly at test time.
  warnings.warn(
/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/environ.py:544: UserWarning: The environment variable 'BLASFEO_TARGET' is being passed through with value 'ARMV8A_APPLE_M1'.  If you are splitting build and test phases with --no-test, please ensure that this value is also set similarly at test time.
  warnings.warn(
/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/environ.py:544: UserWarning: The environment variable 'BLASFEO_TARGET' is being passed through with value 'GENERIC'.  If you are splitting build and test phases with --no-test, please ensure that this value is also set similarly at test time.
  warnings.warn(
Traceback (most recent call last):
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/metadata.py", line 2003, in _get_contents
    rendered = template.render(environment=env)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/jinja2/environment.py", line 1304, in render
    self.environment.handle_exception()
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/jinja2/environment.py", line 939, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/tmp/tmph6awd_rn/blasfeo-feedstock/recipe/meta.yaml", line 32, in top-level template code
    # M1 from Apple, thus we set TARGET to ARMV8A_APPLE_M1
^^^^^^^^^^^^^^^^^^^^^^^^^
jinja2.exceptions.UndefinedError: 'microarch_level' is undefined

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/conda/envs/cf-feedstock-ops/bin/conda-smithy", line 10, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/cli.py", line 758, in main
    args.subcommand_func(args)
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/cli.py", line 601, in __call__
    self._call(args, args.temporary_directory)
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/cli.py", line 604, in _call
    configure_feedstock.main(
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/configure_feedstock.py", line 2880, in main
    render_azure(env, config, forge_dir, return_metadata=True)
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/configure_feedstock.py", line 1900, in render_azure
    return _render_ci_provider(
           ^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/configure_feedstock.py", line 1138, in _render_ci_provider
    metas = _conda_build_api_render_for_smithy(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_smithy/configure_feedstock.py", line 938, in _conda_build_api_render_for_smithy
    metadata_tuples = render_recipe(
                      ^^^^^^^^^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/render.py", line 1005, in render_recipe
    return distribute_variants(
           ^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/render.py", line 894, in distribute_variants
    mv.parse_until_resolved(
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/metadata.py", line 1357, in parse_until_resolved
    self.parse_again(
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/metadata.py", line 1262, in parse_again
    self._get_contents(
  File "/opt/conda/envs/cf-feedstock-ops/lib/python3.11/site-packages/conda_build/metadata.py", line 2013, in _get_contents
    raise CondaBuildUserError(
conda_build.exceptions.CondaBuildUserError: Failed to render jinja template in /tmp/tmph6awd_rn/blasfeo-feedstock/recipe/meta.yaml:
'microarch_level' is undefined
2024-11-19 11:18:16,871 ERROR    conda_forge_webservices.github_actions_integration.rerendering || Rerendering failed: Error running 'conda-forge-feedstock-ops-container rerender --log-level debug' in container - error RuntimeError raised:
'Failed to rerender.
output: Adding in variants from internal_defaults
Adding in variants from /tmp/tmp83brb7p8/conda-smithy/conda_build_config.yaml
Adding in variants from /tmp/tmph6awd_rn/blasfeo-feedstock/recipe/conda_build_config.yaml
Adding in variants from argument_variants

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

It seems like microarch_level might not be supported on Windows... I'll have to dig a bit deeper, though. But would be nice to have micro arch support on Windows.

@traversaro
Copy link
Contributor

It seems like microarch_level might not be supported on Windows... I'll have to dig a bit deeper, though. But would be nice to have micro arch support on Windows.

Apparently some work was done in archspec and conda to support this (see conda-forge/conda-forge.github.io#1261 (comment)), but some work is still needed for full support, see conda-forge/microarch-level-feedstock#3 and https://github.com/conda-forge/microarch-level-feedstock/blob/main/recipe/meta.yaml#L45-L48 . Anyhow, as we do not need the x86_64-microarch-level package but only _x86_64-microarch-level, I do not think there is anything blocking us here. Perhaps we simply need to modify

- 1 # [linux and x86_64]
?

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

It seems like, one can also do it manually, similar to https://github.com/AnacondaRecipes/llama.cpp-feedstock/blob/main/recipe/meta.yaml. I have to play a bit around with it, but this looks like it should work.

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

@traversaro
Copy link
Contributor

If the test are not passing on Windows no problem for me, as long as we tested it once manually offline.

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

It also seems like something fishy is also going on with AVX512 on linux:

+ nm -s /home/conda/feedstock_root/build_artifacts/libblasfeo_1732024316379/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/lib/libblasfeo.so
                 U exit@GLIBC_2.2.5
                 U fmax
                 U fmaxf
                 U fmin
                 U fminf
                 U fprintf@GLIBC_2.2.5
                 U fputc@GLIBC_2.2.5
                 U free@GLIBC_2.2.5
                 U fwrite@GLIBC_2.2.5
                 U gettimeofday@GLIBC_2.2.5
                 U kernel_daxpy_11_lib
                 U kernel_ddot_11_lib
                 U kernel_dgemv_n_4_libc
                 U kernel_dgemv_n_4_vs_libc
                 U kernel_dgemv_t_4_libc
                 U kernel_dgemv_t_4_vs_libc
                 U kernel_dger_4_libc
                 U kernel_dger_4_vs_libc
                 U kernel_dgetrf_pivot_4_lib
                 U kernel_dgetrf_pivot_4_lib4
                 U kernel_dgetrf_pivot_4_vs_lib
                 U kernel_dgetrf_pivot_4_vs_lib4

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits November 19, 2024 15:19
@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11916329122.

@traversaro
Copy link
Contributor

It also seems like something fishy is also going on with AVX512 on linux:

+ nm -s /home/conda/feedstock_root/build_artifacts/libblasfeo_1732024316379/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/lib/libblasfeo.so
                 U exit@GLIBC_2.2.5
                 U fmax
                 U fmaxf
                 U fmin
                 U fminf
                 U fprintf@GLIBC_2.2.5
                 U fputc@GLIBC_2.2.5
                 U free@GLIBC_2.2.5
                 U fwrite@GLIBC_2.2.5
                 U gettimeofday@GLIBC_2.2.5
                 U kernel_daxpy_11_lib
                 U kernel_ddot_11_lib
                 U kernel_dgemv_n_4_libc
                 U kernel_dgemv_n_4_vs_libc
                 U kernel_dgemv_t_4_libc
                 U kernel_dgemv_t_4_vs_libc
                 U kernel_dger_4_libc
                 U kernel_dger_4_vs_libc
                 U kernel_dgetrf_pivot_4_lib
                 U kernel_dgetrf_pivot_4_lib4
                 U kernel_dgetrf_pivot_4_vs_lib
                 U kernel_dgetrf_pivot_4_vs_lib4

Yes, that sounds like a regression similar to #8 . If we can replicate the problem outside of conda, probably we can report the problem upstream. In the meanwhile perhaps we can just skip AVX512 builds for now? Done that, I guess we are good to go?

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

I think so, except we should probably test everything locally first before. Is there a way to get the build binaries so they can be tested locally?

@traversaro
Copy link
Contributor

I think so, except we should probably test everything locally first before. Is there a way to get the build binaries so they can be tested locally?

You can just set store_build_artifacts parameter in azure group in conda-forge.yaml, see https://conda-forge.org/docs/maintainer/conda_forge_yml/#azure and https://github.com/conda-forge/python-feedstock/blob/db88998e1e63b44dc1afbbaac2e2c6ebb4f3703e/conda-forge.yml#L12 for an example. Then you can get the built packages from the Azure CI job.

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits November 19, 2024 16:13
@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11917792051.

@RSchwan
Copy link
Contributor Author

RSchwan commented Nov 19, 2024

MacOS ARM build is working, and I tested the windows package on my local vm. MSVC is able to compile and link blasfeo successfully!

I also looked into AVX512 again, and it looks like there is just a lot that isn't implemented for this architecture yet. Hence, if you don't use the implemented library calls, you can run into issues. Thus, it's probably best to not build for this target. Especially, if people expect the same behavior independent of CPU architecture.

From my side, this can be merged.

@traversaro
Copy link
Contributor

I also looked into AVX512 again, and it looks like there is just a lot that isn't implemented for this architecture yet. Hence, if you don't use the implemented library calls, you can run into issues. Thus, it's probably best to not build for this target. Especially, if people expect the same behavior independent of CPU architecture.

Indeed, see also giaf/blasfeo#182 (comment) .

@traversaro
Copy link
Contributor

From my side, this can be merged.

Great, let's merge so we can enable multi-platform support also on fatrop. Thanks a lot for the help!

@RSchwan RSchwan merged commit cef7f36 into conda-forge:main Nov 19, 2024
13 checks passed
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

Successfully merging this pull request may close these issues.

Enable Windows and macOS builds Understand the proper TARGET for arm architecture
3 participants