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

return architecture levels for micromamba #2921

Merged
merged 6 commits into from
Oct 23, 2023
Merged

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Oct 18, 2023

@jonashaag
Copy link
Contributor

Is there a way to unit test this?

@JohanMabille
Copy link
Member

JohanMabille commented Oct 19, 2023

Is there a way to unit test this?

We could check preprocessor tokens (like we do in xsimd) to detect the architecture, and check that result of get_archspec_x86_64 is consistent, but that would test only the architecture available on the CI. If we want to be exhaustive, we could explictly pass the arch we want to use in the CI (see xsimd again ;) for an example). However, that would require a lot of new jobs in the CI and I'm not sure we want that for testing a single function given the time the CI is already taking.

@jonashaag
Copy link
Contributor

Maybe we can at least compare the output of the Mamba logic to the output of the xsimd logic? That's still limited to the runner architecture but it's better than nothing

@mbargull
Copy link
Contributor

On Linux you could use QEMU. E.g., if you were to compile a test program that prints this function's output:

[ "$( qemu-x86_64 -cpu Penryn test-get_archspec_x86_64` )" = "x86_64" ]
[ "$( qemu-x86_64 -cpu Nehalem test-get_archspec_x86_64` )" = "x86_64-v2" ]
[ "$( qemu-x86_64 -cpu Haswell test-get_archspec_x86_64` )" = "x86_64-v3" ]
[ "$( qemu-x86_64 -cpu Skylake-Server test-get_archspec_x86_64` )" = "x86_64-v4" ]

This would only work if the host CPU supports the features too. I.e., depending on the runner, you may want to guard the test for Skylake-Server at least.
And this would only test on Linux. No idea what the emulators on macOS offers.

@isuruf
Copy link
Contributor Author

isuruf commented Oct 20, 2023

I think adding a better test would not be worth the trouble. If we use __builtin_cpu_supports in the test, we'll be duplicating the actual code in test code which doesn't really test anything. If we use preprocessor macros like __AVX2__, we need to set -march=native, but we need quite a bit of logic to make that work. (Check that it is UNIX and (clang or gcc) and x86_64 which is not easy in CMake as there are options like CMAKE_OSX_ARCHITECTURES)
Finally using an emulator like QEMU or Intel SDE would work, but not sure it's worth it to complicate the CI infrastructure.

@JohanMabille
Copy link
Member

Indeed, I missed that we don't pass --march=native, and other options would make the CI much more complicated for a simple function. Let's merge as is.

@JohanMabille JohanMabille merged commit 99278ff into mamba-org:main Oct 23, 2023
23 checks passed
@isuruf isuruf deleted the level branch October 23, 2023 19:41
isuruf added a commit to isuruf/mamba that referenced this pull request Oct 25, 2023
* return architecture levels for micromamba

* fix formatting

* use function multiversioning only on x86

* fix formatting

* update test

* use __builtin_cpu_supports and check for more features

Co-authored-by: Marcel Bargull <[email protected]>

---------

Co-authored-by: Marcel Bargull <[email protected]>
JohanMabille pushed a commit that referenced this pull request Oct 26, 2023
* Allow defaults::* spec (#2927)

* return architecture levels for micromamba (#2921)

* return architecture levels for micromamba

* fix formatting

* use function multiversioning only on x86

* fix formatting

* update test

* use __builtin_cpu_supports and check for more features

Co-authored-by: Marcel Bargull <[email protected]>

---------

Co-authored-by: Marcel Bargull <[email protected]>

* Fix channels with slashes regression (#2926)

* Fix channels with slashes regression

* fix formatting

* make the test isolated

Co-authored-by: Antoine Prouvost <[email protected]>

* Add more tests

* Use pytest utilities for negative testing

* Add extra test doc

---------

Co-authored-by: Antoine Prouvost <[email protected]>

* Fix for name change

---------

Co-authored-by: Marcel Bargull <[email protected]>
Co-authored-by: Antoine Prouvost <[email protected]>
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.

5 participants