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

Add microarch-level packages #24306

Merged
merged 25 commits into from
Nov 7, 2023
Merged

Add microarch-level packages #24306

merged 25 commits into from
Nov 7, 2023

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Oct 18, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

cc @conda-forge/core

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/x86_64_level) and found some lint.

Here's what I've got...

For recipes/x86_64_level:

  • The recipe must have some tests.

@isuruf
Copy link
Member Author

isuruf commented Oct 18, 2023

This recipe intentionally does not have tests so that we could build these packages on one microarchitecture.

@kkraus14
Copy link
Contributor

Should there be a matching conda virtual package for this package in order for built packages against a specific microarchitecture to be able to be solved for in environments?


requirements:
run:
- __archspec 1={{ microarchitecture }}
Copy link
Member Author

Choose a reason for hiding this comment

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

@kkraus14, we need __archspec package.

@isuruf
Copy link
Member Author

isuruf commented Oct 18, 2023

conda/mamba supports __archspec virtual package. Micromamba PR: mamba-org/mamba#2921

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Do we want run exports so people can include this in say build and then get the proper constraint in run?

^ ignore this

@beckermr
Copy link
Member

Ack no. We want packages that adjust the compiler flags that export the constraints on these packages.

@beckermr
Copy link
Member

beckermr commented Oct 18, 2023

Yeah I think for the run exports here to work the build machine would have to have same micro arch as the run env. Thus we cannot use these exports to cross compile. That's not necessarily a bug just a limitation.

@isuruf
Copy link
Member Author

isuruf commented Oct 18, 2023

Added a new x86_64_level_build package that doesn't have a run requirement of __archspec so that we can cross-compile easily.

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Code looks good. Are we happy with the names? They are not obvious to me for sure.

@kkraus14
Copy link
Contributor

kkraus14 commented Oct 18, 2023

conda/mamba supports __archspec virtual package. Micromamba PR: mamba-org/mamba#2921

Does that report the levels correctly? I.E. the conda info / mamba info report the __archspec package as just x86_64 as far as I can tell?

EDIT: looks like your PR will update mamba to correctly report the level

EDIT2: Shown by Isuru below, need archspec in the base environment, otherwise it just yields x86_64

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I made some suggestions to try and make the names clearer to me. Also I think we need a strong run export since the flags package should go in build.

recipes/x86_64_level/meta.yaml Outdated Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Outdated Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Outdated Show resolved Hide resolved
isuruf and others added 2 commits October 18, 2023 16:30
Co-authored-by: Matthew R. Becker <[email protected]>
@beckermr
Copy link
Member

Still needs the strong run export I think.

@beckermr
Copy link
Member

Thanks a bunch for the work here @isuruf! I think we're good to go.

Copy link
Contributor

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

Thanks @isuruf, LGTM

@jezdez
Copy link
Member

jezdez commented Oct 19, 2023

I'm looking at conda/conda#13213 and conda-forge/conda-feedstock#222, does anyone have a primer what the plan is for the levels?

@hmaarrfk
Copy link
Contributor

i can't believe I am so excited about a "package"......

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Isuru! 🙏

We could template the build/number to make it easier to work with

recipes/x86_64_level/meta.yaml Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Outdated Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <[email protected]>
@isuruf isuruf requested a review from mbargull November 7, 2023 15:58
Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM, apart from minor nits

recipes/x86_64_level/gen.py Outdated Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Show resolved Hide resolved
recipes/x86_64_level/meta.yaml Show resolved Hide resolved
isuruf and others added 2 commits November 7, 2023 13:45
Co-authored-by: Marcel Bargull <[email protected]>
@mbargull mbargull changed the title Add x86_64_level package Add microarch-level packages Nov 7, 2023
@mbargull
Copy link
Member

mbargull commented Nov 7, 2023

If there aren't any further concerns, feel free to merge!

@beckermr
Copy link
Member

beckermr commented Nov 7, 2023

merge merge merge!

@isuruf isuruf merged commit 13d817e into conda-forge:main Nov 7, 2023
4 of 5 checks passed
@isuruf isuruf deleted the level branch November 7, 2023 20:34
@mbargull
Copy link
Member

mbargull commented Nov 7, 2023

I'm looking at conda/conda#13213 and conda-forge/conda-feedstock#222, does anyone have a primer what the plan is for the levels?

@jezdez, AFAICT you question hasn't been addressed yet:
If one would want to provide microarch-optimized package builds (when the upstream project does not support some dynamic dispatch mechanism), they can build packages with these little helper packages like this:

package:
  name: pkg
  version: 1.0
build:
  script: |
    # export CFLAGS="${CFLAGS} -march=x86-64-v3"  # done via x86_64-microarch-level=3 activation script
    ./build-package.sh
requirements:
  build:
    - x86_64-microarch-level 3  # need at least this level for AVX2
    - {{ compiler("c") }}

with the run_exports: strong: ... we'd then get a package with

depends:
  - _x86_64-microarch-level >=3

For my PC I have

conda info|grep archspec
       virtual packages : __archspec=1=skylake

and the package _x86_64-microarch-level=3=*_skylake has

depends:
  - __archspec 1=skylake

which is satisfied by the virtual package.

As such conda install pkg=1.0 would install

- pkg=1.0
- _x86_64-microarch-level=3=0_skylake

for me.

If we'd instead built a package targeting some AVX512 and thus required x86_64-microarch-level 4, I wouldn't be able to install it because _x86_64-microarch-level=3=0_skylake wouldn't satisfy _x86_64-microarch-level >=4 -- which is what we want since my processor can't run that build's binaries.

(Setting the CFLAGS and adding the *-microarch-level ? dependency manually can of course be made more convenient by further compiler activation(-like) packages or similar.)

@beckermr
Copy link
Member

beckermr commented Nov 7, 2023

@mbargull The code here already injects the proper flags in the build env. That is what the activate scripts do.

@mbargull
Copy link
Member

mbargull commented Nov 7, 2023

@mbargull The code here already injects the proper flags in the build env. That is what the activate scripts do.

🙀 Can't be -- I just reviewed this PR today even including a code change suggestion regarding the flags...
Yeah, I might be a bit tired 🙈. Thanks for the correction :).

@jaimergp
Copy link
Member

jaimergp commented Nov 7, 2023

Thanks for that comment! It painted a very clear picture. This would be awesome to have in the docs, if it's not there yet!

@mbargull
Copy link
Member

mbargull commented Nov 8, 2023

Yeah, thanks @beckermr and @isuruf for the corrections :).

This would be awesome to have in the docs, if it's not there yet!

I didn't check if we have docs for that yet.
When we add/update those, we might want to put a

# conda_build_config.yaml
microarch_level:  # [unix and x86_64]
  - 1  # [unix and x86_64]
  - 3  # [unix and x86_64]
  - 4  # [unix and x86_64]
---
# meta.yaml
[...]
requirements:
  build:
    - x86_64-microarch-level {{ microarch_level }}  # [unix and x86_64]

example in there and also a comment telling people

  • not to overuse it (build matrix bloating),
  • only use it when appropriate (i.e., if it provides relevant speed up)
  • look out for upstream runtime arch-specific dispatch support,
  • possibly applying this only on a sub-package level if the main package is already large due to non-arch-specific content.

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

Successfully merging this pull request may close these issues.

9 participants