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

[cmake] Allow users to link against onemath_<domain>_<backend> targets #627

Merged
merged 5 commits into from
Jan 30, 2025

Conversation

Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented Jan 22, 2025

Description

The goal of this PR is to allow users to integrate oneMath in their application with compile-time dispatching, only by linking against onemath__ targets. With the current CMake this would fail to compile due to missing include directories and linking libraries unless users manually add the dependencies or set up environment variables. This makes it easier for users.
To show this I have also updated the example's CMake to simplify them and make sure they only rely on target_link_libraries to get the oneMath library dependencies.

For some more context about my motivation for this change:

  1. I hope this would help avoid issues discussed in this reddit thread.
  2. We found we needed these changes to easily integrated oneMath in lamma.cpp without relying on environment variables.

Other changes:

  • Remove mention of onemath_ targets in the documentation. I believe these targets only provide partial support for runtime-dispatching and have not benefit over using onemath.
  • Remove occurrences of install(TARGETS ${LIB_OBJ} ...). These target pollute the outputted oneMathTargets.cmake file and end up requiring more CMake targets such as ONEMATH::cublas targets which are not useful to users. With these changes ${LIB_NAME} targets link ${LIB_OBJ} targets privately so it is not needed to install them anymore.

These are only CMake changes so I think the review from onemath-arch-write is enough but more feedback is welcomed.

Checklist

All Submissions

@Rbiessy Rbiessy requested review from a team as code owners January 22, 2025 17:26
Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements, I like this.

Would it be possible (probably in a different PR) to have shared code between the many nearly identical CMakeLists.txt files? Then a PR like this would only have to change a couple files instead of 38.

docs/using_onemath_with_cmake.rst Show resolved Hide resolved
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jan 24, 2025

Thanks for the improvements, I like this.

Would it be possible (probably in a different PR) to have shared code between the many nearly identical CMakeLists.txt files? Then a PR like this would only have to change a couple files instead of 38.

I'm not sure, it may not be that easy to share common code in the src CMake files without adding more complexity but maybe with some functions. The CMakefiles in the examples should be kept as they are to be self-contained in my opinion.

Also I am planning to focus more on issues we need to fix for other projects or issues reported by users when possible.

Copy link
Contributor

@dnhsieh-intel dnhsieh-intel left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for the improvements! Ping @andreyfe1 to double-check if I missed something.

By the way, do you know why the compile-time dispatching examples require both mklcpu and cuda backends? I was wondering if this is a good idea...

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jan 27, 2025

By the way, do you know why the compile-time dispatching examples require both mklcpu and cuda backends? I was wondering if this is a good idea...

No real reasons other than the examples have been written this way. I suppose the idea is that we expect this may be more useful for most users.
It should be possible to update the examples so that they run only on one of the backend if it is enabled with a bit of refactoring.

@Rbiessy Rbiessy force-pushed the romain/improve_cmake branch from 2ed4d5e to e751460 Compare January 28, 2025 14:08
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jan 28, 2025

@andreyfe1 I have added back the install(${LIB_OBJ} ... targets. They seem to be needed when -DBUILD_SHARED_LIBS=False is used.
@uxlfoundation/onemath-rng-write also note I have enabled the device example only when BUILD_SHARED_LIBS is enabled as the example is using runtime dispatching.

@andreyfe1
Copy link
Contributor

Thanks @Rbiessy. CMake generation looks good now.
Could you please enable the device example for both values of BUILD_SHARED_LIBS? Device API is header based. So, let's check that the example works anyway

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jan 28, 2025

Thanks @Rbiessy. CMake generation looks good now. Could you please enable the device example for both values of BUILD_SHARED_LIBS? Device API is header based. So, let's check that the example works anyway

Oh I see the issue. I linked that example against the onemath CMake target as this seemed to me as the proper CMake way. This is only enabled for BUILD_SHARED_LIBS=ON.
I revert this since it's not needed for the device API. I think it can be confusing for new users to properly integrate oneMath in their project to use the rng device API (i.e. the example uses ${PROJECT_SOURCE_DIR}/include but that's not something an external project can do). It isn't the goal of the PR to address this so I have reverted some changes to not use onemath anymore and always enable the example.

On a separate note when I try to run this example locally I get undefined reference to 'mkl_serv_iface_exit'. I am also getting this with the develop branch so I assume this is an issue with my setup. I am using icpx 2025.0 and setting up my environment with source /opt/intel/oneapi/setvars.sh. I'm happy if you confirm that the example works for you, thanks!

@andreyfe1
Copy link
Contributor

Thanks @Rbiessy. I'll do experiments locally

adegomme added a commit to SiPearl/oneMath that referenced this pull request Jan 29, 2025
Copy link
Contributor

@andreyfe1 andreyfe1 left a comment

Choose a reason for hiding this comment

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

Local testing is good enough. Thanks for changes!

@Rbiessy Rbiessy merged commit 1401716 into uxlfoundation:develop Jan 30, 2025
9 checks passed
@Rbiessy Rbiessy deleted the romain/improve_cmake branch January 30, 2025 09:53
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.

4 participants