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

[RFC] oneMKL Interface renaming #564

Merged
merged 9 commits into from
Oct 10, 2024
205 changes: 205 additions & 0 deletions rfcs/20240903-onemkl-interface-rename/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
# OneMKL Interface renaming

### Revision


|Date |Revision | Comments |
|-----------|---------|--------------------------------------------------------------------------|
| 20240903 | 1.0 | Initial version |
| 20240916 | 1.1 | Rename occurrences of "mkl" and add an open question |

## Motivation

As oneMKL interface is moving to the UXL foundation we should make sure that the
name does not collide with the existing [Intel oneMKL
product](https://www.intel.com/content/www/us/en/developer/tools/oneapi/onemkl.html).

We have been discussing 2 solutions to avoid this issue:
1. Renaming the [oneMKL](https://github.com/oneapi-src/oneMKL) GitHub project to
a new name to be discussed and replacing or clarifying the occurrences of
"oneMKL" in that repository.
2. Using this opportunity to also split this GitHub project per domain. In this
case the names that are advertised would be based on each domain of oneMKL
like `one<Domain>`.

We think that the first solution should be enough to answer the main concern. It
is not clear whether the second solution could better answer the need of users
so we are not planning to split oneMKL per domain unless there is clear and
strong feedback from users.

**We aim to agree on a solution by October 4, 2024 and have the proposal
implemented by the end of January 2025.**
Comment on lines +31 to +32

Choose a reason for hiding this comment

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

seems to me that a decision about changing namespaces and not just the repo name should be considered for a longer period of time and advertised quite widely to get feedback. I was thinking several months for such a change to be discussed and decided on, so decision would be made more around Jan 2025 and then implemented. Or do we have a rush on this ? honestly it is a far reaching choice, so might suggest to give it time for advetisement ... :)

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 feel that this topic has been discussed for some time already. One of the goal is to move the oneMKL Interface repository to the UXL foundation organization by the end of the year and we thought it would be a good timing to also rename it.
I don't see why we would several months to agree on the renaming. Do you have a specific concern? As far as I can tell the main contributors agree on the general idea already. The impact on user is manageable, they should only need to rename a CMake namespace to get it to compile, see the section user impact.
We're planning to present this topic in the next UXL open-source WG on September 24. I think that will help us determine if people have concerns and need more time.

Choose a reason for hiding this comment

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

Reading the description below of discussion in Sept 24 UXL WG meeting, I accept the suggestion and Oct 4 is fine with me then. :)


There has been a number issues of where we have had to clarify the differences
between oneMKL Interface and the Intel oneMKL product
[#501](https://github.com/oneapi-src/oneMKL/issues/501#issuecomment-2134681621);
[#377](https://github.com/oneapi-src/oneMKL/issues/377);
[#253](https://github.com/oneapi-src/oneMKL/issues/253);
[#222](https://github.com/oneapi-src/oneMKL/issues/222);
[#211](https://github.com/oneapi-src/oneMKL/issues/211);
[#207](https://github.com/oneapi-src/oneMKL/issues/207);
[#206](https://github.com/oneapi-src/oneMKL/issues/206). Many of these issues
are from 2022. I believe this is still an issue today since this has been an
issue on multiple occasion internally when Codeplay started to contribute to
oneMKL Interface. The oneMKL Interface
[README](https://github.com/oneapi-src/oneMKL?tab=readme-ov-file#onemkl) already
explains the differences between oneMKL Interface and Intel oneMKL product but
this is not enough for people who are not working directly on the projects.

## Outline

1. [Introduction](#introduction)
2. [Proposal](#proposal)
3. [User impact](#user-impact)
4. [Open questions](#open-questions)

## Introduction

oneMKL Interface has historically been implemented and supported by Intel. As
the project is moving to the UXL foundation we want to avoid using the name of
an Intel product. This RFC describes the solution of renaming oneMKL Interface
to a new name.

## Proposal

The main purpose of this RFC is to agree on a new name for the oneMKL specification and oneMKL Interface.
Some of the suggested names for the implementations are:
* **oneMath**
* The specification would be renamed oneMath Specification
* The C++ namespace would be `oneapi::math`
* The CMake namespace would be `ONEMATH`
* **oneSLA** (SYCL Linear Algebra)
Copy link
Contributor

Choose a reason for hiding this comment

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

'oneMath' looks good to me. I'm not sure 'oneSLA' will be accurate in this case, since FFT, RNG, and Vector math are not technically part of Linear Algebra

Copy link
Contributor

Choose a reason for hiding this comment

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

and SLA first association is service Level Agreement, not Linear Algebra

* The specification would be renamed oneSLA Specification
* New namespace would be `oneapi::sla`
* The CMake namespace would be `ONESLA`

Other suggestions are welcomed. The name **oneMath** will be chosen if there are
no objections by October 4, 2024.

The suggested solution is to proceed in the following steps:
1. The UXL foundation agrees on the new name.
2. Codeplay submits a oneAPI-spec PR to rename the occurrences of "oneMKL" and
"mkl" to the new name.
3. Codeplay submits a oneMKL Interface PR to:
* Update the root README to use the new name, with a mention that the project
was formerly called oneMKL Interface.
* Update the references to "oneMKL" and `onemkl_` in the documentation as
seen in the first few lines of
[docs/onemkl-datatypes.rst](https://github.com/oneapi-src/oneMKL/blob/develop/docs/onemkl-datatypes.rst?plain=1#L1)
for instance.
* Update occurrences of "onemkl" in internal functions such as
[onemkl_cublas_host_task](https://github.com/oneapi-src/oneMKL/blob/1ce98a699f93bd3a78350269b2e34d822fe43b91/src/blas/backends/cublas/cublas_task.hpp#L77).
* Update macros such as include guards and other internal macros like
`ONEMKL_EXPORT` to use the new name.
* Rename CMake targets `onemkl` and `onemkl_<domain>_<backend>` to use the
new name. The existing targets name can be added with a deprecation
messages for anyone using them. See the section on [CMake target
deprecation](#cmake-deprecated-target) for more details. The namespace of
the exported and installed targets are changed to use the new name.
* The namespace `oneapi::mkl` is deprecated and aliased to the new namespace
name.
* The main header and domain headers are deprecated and include the new headers as shown below:
Rbiessy marked this conversation as resolved.
Show resolved Hide resolved
* `include/oneapi/`
* `mkl.hpp` -> `onemath.hpp`
* `mkl/` -> `onemath/`

Choose a reason for hiding this comment

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

why onemath.hpp and not math.hpp ? and math/ instead of onemath/ ? since it is in the include/oneapi/ folder where the "one" presumably comes from ?

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 have a small preference for onemath as I find math is just too generic. I don't think it is an issue to repeat the one prefix. I have renamed it to 1182a95 since I seem outvoted for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other oneAPI parts don't repeat the prefix: <oneapi/dpl/execution>, "oneapi/dal.hpp", <oneapi/tbb/collaborative_call_once.h>.

* `blas.hpp`
* `dft.hpp`
* `lapack.hpp`
* `rng.hpp`
* `sparse_blas.hpp`
4. Once the PRs are approved, Codeplay transfers the
[oneMKL](https://github.com/oneapi-src/oneMKL) GitHub project to the
[uxlfoundation](https://github.com/uxlfoundation) organization under the new
name. We use the
[transferring](https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository)
feature from GitHub so the links from the previous repository are redirected
to the new one.
5. The PRs from the step 2 and 3 are merged.

We suggest to keep the deprecated features for 1 year before removing them.

### CMake target deprecation

CMake allows to set a
[`DEPRECATION`](https://cmake.org/cmake/help/latest/prop_tgt/DEPRECATION.html)
property on a target which will print a custom message whenever the target is
used. The property cannot be set on an [alias
target](https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#alias-targets)
as they are read-only. The property can be set on an imported target instead
like the example below:

```cmake
add_library(onemath lib.cpp) # New main target, for the example

add_library(onemkl INTERFACE IMPORTED) # onemkl works like an alias of onemath which can have different properties
target_link_libraries(onemkl INTERFACE onemath)
set_target_properties(onemkl PROPERTIES DEPRECATION "onemkl target is deprecated, please use onemath instead")

add_executable(main main.cpp)
target_link_libraries(main PUBLIC onemkl) # Prints a warning at CMake configuration time
```

The same solution can be used for the `onemkl_<domain>_<backend>` targets. This does
not add any extra targets to the generated `Makefile` or `build.ninja` files so
the library will not be built twice.

### Other Considered Approaches

Another considered approach is to split the existing oneMKL Interface per domain
like so: oneBLAS, oneLAPACK, oneDFT, oneRNG, oneSPARSE. This shifts the need of
renaming "oneMKL Interface" as the main visible names will be based on the
domain. It is not clear whether this better answers the users needs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have one idea to share about how to improve rebase simplicity when multiple people work on different domains in one repo at the same time, as an option we might consider switching merge policy from 'rebase and merge' to 'merge commits'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some developers, myself included, are not fond of using merge commits in the main branch. I find it makes the history harder to read and I don't see what difference it would make for developers working on a single domain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that, I personally tend to avoid merge commits at all costs exactly because the history is harder to figure out. Having a clean history also means the pretty output of git log --all --decorate --oneline --graph is easier to decipher.


With this approach the suggested solution is to have a common repository for
common types (`transpose`, `uplo`, `diag`, `side`, `offset`, `index_base`,
`layout`), exceptions and some CMake logic. Each domain would have its own
repository automatically pulling the common headers. Another repository could be
created to automatically pull multiple domains which would mimic the behavior of
the current oneMKL Interface.

We are not planning to proceed with this approach unless users express a strong
preference.

## User impact

The suggested solution can break user's code in 2 ways:
* Only the headers listed below should be included by the users. Any other
headers will be moved and will not have an equivalent header with a
deprecation warning.
* `include/oneapi/`
* `mkl.hpp`
* `mkl/`
* `blas.hpp`
* `dft.hpp`
* `lapack.hpp`
* `rng.hpp`
* `sparse_blas.hpp`
* The CMake logic using the `MKL::` or `ONEMKL::` namespaces will need to be
renamed to a new namespace based on the chosen name.

The following changes have no impact:
* The repository is transferred using the [GitHub
transfer](https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository)
feature so users accessing or pulling from
https://github.com/oneapi-src/oneMKL will be redirected to the new link.
* The changes in oneMKL do not affect the public API. The macros renamed or the
header files in the `detail` folders renamed should not be used outside of the
oneMKL Interface project.
* The CMake changes will still provide the same targets but will print a warning
message if users use targets with the `onemkl` name.

## Open questions

* Other suggestions for new names are welcomed.
* Is it needed to rename the occurrences of "mkl"?
* From the feedback gathered, users and maintainers are keen to rename these
occurrences.
* Should the specification and the existing implementation have different names?
* Currently both the specification and implementation are named based on
"oneMKL" which suggests that the oneMKL Interface is the main or only
implementation of the specification. If the goal of the specification is to
encourage for multiple implementations to co-exist then it should be named
differently than the implementation.
* Given the nature of the project that allows for multiple backends I don't
see any value in encouraging multiple implementations as of today.
* Using multiple names may create more confusion.