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

Conversation

Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented Sep 4, 2024

Link to rendered README: https://github.com/Rbiessy/oneMKL/tree/romain/rfcs/onemkl_rename/rfcs/20240903-onemkl-interface-rename

The RFC proposes a solution to rename oneMKL Interface while moving it to the UXL foundation organization.

@Rbiessy Rbiessy added the RFC A proposal to add new API label Sep 4, 2024
@Rbiessy Rbiessy requested a review from a team September 4, 2024 12:39
@al42and
Copy link
Contributor

al42and commented Sep 5, 2024

My 2 cents as a user:

  • Re: Is it needed to rename the occurrences of "mkl"?
    • I don't see how keeping mkl around reduces the confusion. "To use oneXYZ library (not to be confused with Intel oneMKL) library, we include oneapi/mkl.hpp file, use functions from oneapi::mkl namespace, and link to oneXYZ CMake target".
    • If the Intel and UXL APIs are in sync, then keeping the old name in the code could help users code duplication for handling different namespaces / macros prefixes / include paths. But if the APIs diverge, having them use the same namespace / macros would become error-prone.
    • If mkl is fully expunged from the library code, it's better to do it in one swoop (keeping the old symbols deprecated but available for a while). It would be unpleasant to deal with a mix of old and new naming that's changing from version to version.
  • As long as installing a single domain is supported, I see no benefits for the user from splitting the library. No downsides either.
    • Where would mkl::stats and mkl::vm go if the library is split by domain?
  • Deprecation / renaming are ok for us in terms of updating our code to use the new name. But that's mainly because we don't currently have open-source oneMKL in the typical builds for end-users.
  • We had very little actual issues with the old name, although I personally agree that the current situation is confusing; and "There are three oneMKLs" strongly indicates the need for better naming too.
  • Naming:
    • oneSLA: my only association with SLA is "service-level agreement"; two (FFT and RNG) out of five supported domains are not linear algebra; "SYCL" in the name is redundant since most oneAPI projects are SYCL-related.
    • I like oneMath is better, even though it departs from the one[A-Z]{3} scheme used by other projects.
    • Any one[A-Z][A-Za-z]+ name could be confused with Intel product, IMO.
  • Would it make sense to also introduce a clear distinction between the library (code) and the interface (spec)? If the spec is supposed to be open, it's not good/fair towards (hypothetical) other implementations to so strongly mark them as second-class.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 6, 2024

Thank you for your valuable feedback @al42and!

Regarding the occurrences of "mkl", as far as I know the plan is to keep the API of the Intel oneMKL product and the open-source interface as close as possible. Introducing a new namespace and header files for the interface would break the ability to transition between the product and the interface seamlessly (without a change in the product) so I have left it as an open question.

Where would mkl::stats and mkl::vm go if the library is split by domain?

They would also have their own repository like oneStats and oneVM. I didn't mention them since they have not been implemented yet.

Would it make sense to also introduce a clear distinction between the library (code) and the interface (spec)?

If I understand you correctly, you are suggesting the specification should not mention the name of the implementation. So we would provide the specification for "XYZ" and the implementation would be an implementation of "XYZ" and have a different name. It's a fair point. I am concerned this will only add more confusion. Given the nature of the project with the different backends I'm not sure another implementation would be that valuable to be honest.

@al42and
Copy link
Contributor

al42and commented Sep 6, 2024

Given the nature of the project with the different backends I'm not sure another implementation would be that valuable to be honest.

Agree with you on the practical aspect, but I think the messaging is important too. If oneXYZ is a library, then it is a specific piece of code with a documented interface (which someone might decide to copy). If oneXYZ is a standard, it strongly implies that there can be several implementations of it (and they all are equal). E.g., Python vs. C++.

oneAPI spec page implies there can be multiple implementations of the spec: "The oneAPI initiative encourages collaboration on the specification and compatible oneAPI implementations across the ecosystem." (emphasis mine). But that may be me reading too much into it.

as far as I know the plan is to keep the API of the Intel oneMKL product and the open-source interface as close as possible

Then, is Intel oneMKL an implementation of the oneXYZ API and also a backend of oneXYZ library? Since oneMKL implements oneXYZ, why is oneXYZ using mkl namespace?

Or, if it is oneXYZ following Intel oneMKL, then what's the point of the working groups, discussions and all that if it just copies whatever Intel oneMKL does?

Disclaimer: The flow of ideas is never as strictly uni-directional as I'm portraying above, especially early on in the project's life. But it's good to have a vision of how things are supposed/intended to work.

Given the nature of the project with the different backends I'm not sure another implementation would be that valuable to be honest.

If Intel oneMKL follows the oneXYZ spec, then there are two implementations already: Intel oneMKL and the open-source oneXYZ we have here :)

Introducing a new namespace and header files for the interface would break the ability to transition between the product and the interface seamlessly (without a change in the product) so I have left it as an open question.

Can't say it's totally seamless now. Pretty close, but not drop-in (not to mention the build system).

If it's just a name of the namespace, then using would solve this problem with about the same effort as the macro/enum problem is solved in GROMACS now.

UPDATE: I don't want to derail this PR into a discussion about project governance, but since the goal of the proposal is to disambiguate three entities, I feel like an informed decision requires a clear picture of how these entities are related.

@mkrainiuk mkrainiuk requested a review from a team September 7, 2024 01:25
Copy link
Contributor

@mkrainiuk mkrainiuk left a comment

Choose a reason for hiding this comment

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

The RFC looks good to me, thank you for summarizing all this work. I have several comments/questions.

The main purpose of this RFC is to agree on a new name for oneMKL Interface.
Some of the suggested names are:
* **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

rfcs/20240903-onemkl-interface-rename/README.md Outdated Show resolved Hide resolved

* Other suggestions for new names are welcomed.
* Is it needed to rename the occurrences of "mkl"?
* This will have a bigger impact and require more time to complete.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it will also affect specification

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.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 9, 2024

@al42and these are great questions. They are about the relationship between the oneMKL Interface and the Intel oneMKL product. I believe others can provide a better answer, maybe @mkrainiuk, @spencerpatty or @rscohn2?

@etienne-renault
Copy link

This RFC LGTM. I do have few questions / comments :

  • Regarding the "opportunity to split this GitHub project per domain". I'm also not sure about this, and I would say that it will add more confusion.
  • I'm puzzled with the usage of the prefix "one..." which is very Intel related. I would suggest to move to something drastically different to remove confusion. Otherwise, something <prefix>Math looks good to me (and I agree about the inadequation of the SLA name)
  • Regarding the renaming of the occurrences of oneapi::mkl namespace, the include/oneapi/mkl folder or the include/oneapi/mkl.hpp : i would recomand to perform this renaming to avoid any confusion / future troubles.

@colleeneb
Copy link

colleeneb commented Sep 9, 2024

I agree with comments above about oneMath is preferred to oneSLA since not everything is linear algebra.

Just to check my understanding is correct: There's the "oneMKL specification" which is the specification, "oneMath" (calling it by the new name for now) which is an implementation of the oneMKL specification with many backends. Then there is the Intel product oneMKL, which is an implementation of the oneMKL specification by Intel.

Since this is an implementation of the oneMKL specification, I think it makes sense to continue to use the oneapi::mkl:: namespaces since that's in the specification. However, the term "MKL" is so connected in people's minds with Intel that I think it will possibly always confuse people unless the namespaces are changed to some other name. But changing to another name would break a lot of people's code so I don't know if that's practical.

Overall I think the RFC looks good (good to try to prevent confusion and distinguish from Intel's implementation!), just that I think there will still be confusion since the term "MKL" is still in the specification and it's so connected with Intel.

(If the oneapi::mkl namespaces change in the future, one option is to match the C++26 APIs, at least for linear algebra: https://en.cppreference.com/w/cpp/numeric/linalg . Just as an option, but not something I'd necessarily suggest.)

@al42and
Copy link
Contributor

al42and commented Sep 9, 2024

They are about the relationship between the oneMKL Interface and the Intel oneMKL product.

To clarify, I don't want to derail this PR into a discussion about project governance, but since the goal of the proposal is to disambiguate three entities, I feel like an informed decision requires a clear picture of how all these entities are related.

Given the nature of the project with the different backends I'm not sure another implementation would be that valuable to be honest.

Another use case to consider here could be 3rd party domain-specific libraries implementing the oneAPI interface on their own. E.g., what if https://github.com/intel/double-batched-fft-library decides to adopt oneAPI interface for DFT? If oneAPI gains traction, that would not be unreasonable (just like Intel MKL has FFTW-like interface).

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 10, 2024

Thank you for the good feedback so far. I note the preference for oneMath for now although the prefix one is debatable. The oneAPI specification has been moved as in to the UXL foundation repository and I am not aware of a plan to rename this so I kept this prefix for now.

I agree that not every domain can be classified as "linear algebra". This is one of the reason for the initial proposition to split per domain, there isn't much in common between all of the domains so it is hard to come up with a new meaningful name.

Regarding the Intel oneMKL product, I try to see it as 2 backends (for MKLCPU and MKLGPU) like any others. Technically it has its own C specification and C++ specification. I also think it would be best to remove the occurrences of "mkl". I could write more details about what this would look like. It would be useful to get some feedback from the Intel oneMKL product owners on this and the suggestion of using different names for the specification and implementation.

@gajanan-choudhary
Copy link
Contributor

Regarding the Intel oneMKL product, I try to see it as 2 backends (for MKLCPU and MKLGPU) like any others. Technically it has its own C specification and C++ specification.

Calling them specifications (or in Wikipedia terms, standards) is technically incorrect. Those are just documentation pages of the C and SYCL APIs in the Intel oneMKL library.

@spencerpatty
Copy link

spencerpatty commented Sep 10, 2024

So I am part of Intel oneMKL team and I have discussed this concern about the name oneMKL being in 3 places and the potential for separating the Specification and the Interfaces project in name from the Intel oneMKL Product with several people from oneMKL team and I think we all see the concerns and feel similarly to those asking about if names could change.

To be clear, I am talking about the following potential changes:

  • oneMKL Specification -> oneMath Specification
    • (and using oneapi::math:: namespace instead of oneapi::mkl:: namespace)
  • oneMKL Interfaces Project -> oneMath Interfaces Project
    • (again using oneapi::math:: namespace instead of oneapi::mkl:: namespace)
  • Intel oneMKL Library (or Product) -> stays Intel oneMKL Library (or Product)
    • (using oneapi::mkl:: namespaces)

It makes a lot of sense to me (and at least to those I have talked with) to make this kind of change. We at Intel introduced (now using new proposed names) the oneMath Specification to try to encode a common set of math related APIs that could be supported by few or many different library implementations that allow applications to move beyond a single software/hardware configuration and be more productive on multiple hardware/software systems. Intel oneMKL Library already had a wide range of supported math domains and so we modelled much of the original specification around existing and future planned solutions, and now that it is part of the UXL foundation, we hope it will continue to evolve according to many different users needs into a common C++ language solution.

The oneMath Interfaces project was always meant to be a direct implementation of the oneMath specification and move with it. It was also designed to have many potential backends which would facilitate the end goal of making the hardware/software switch simple. Intel oneMKL Library provides two such backend implementations for the oneMath Interfaces: namely MKLCPU and MKLGPU. There were many tricky things that came from having the Intel oneMKL library as backends even though both use the same namespaces, but as I understand it, those have been worked through already, so it is not as tricky anymore ...

On the other hand, the Intel oneMKL Library relationship with the oneMath Specification has always been a little bit more complicated. Our goal has been to be compliant with the specification, but we also often have many more things available than are in the specification, and in fact often do some of the design work that leads to a new function being introduced to the specification through actually implementing things and getting customer feedback on our implementations before adding it. So from our perspective, we are both behind and in front of the specification. Additionally, there are many things which we offer using particular hardware features which may not belong in the specification as they are not general enough for common implementations. The question for us is this: "Can we continue considering ourselves as an implementation of the oneMath specification if we do not have exactly the same interfaces (say that differs in a namespace) ? The answer is a resounding: maybe :) - haha! We definitely want to try to keep our interfaces consistent with the specification as much as we overlap, and we might consider to add additional oneapi::math:: based interfaces to our product to be exactly compliant, or we might be able to come up with some sort of aliasing or namespace manipulation to convert what is available in the library into what is defined in the spec and written in the application… On the other hand, there are several existing applications which are written with oneapi::mkl:: and using the oneMath Interfaces, so either they would have to change their code or a similar conversion might need to be constructed for them in their applications.

All that said I think it makes sense (if that is the direction this UXL group discussion is heading) to move away from the mkl namespace in the oneMath Specification and in the oneMath Interfaces project.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 16, 2024

Thank you all for the feedback so far. I have added some clarifications on the impact of renaming the occurrences of "mkl". I have bumped the estimation to implement this change by January 2025 as a consequence.
I have also added an open question on whether to use a different name for the specification.

Comment on lines 103 to 104
* `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>.

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

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. :)

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Sep 25, 2024

FYI we discussed this RFC in the UXL open-source WG on September 24. There were no major concern on the RFC, I have applied some feedback in 3847831. The outcomes are:

  • Update the RFC to mention that the onemkl_<domain> CMake targets will also be renamed, like the other CMake targets using the onemkl name.
  • @al42and we clarified that the goal of the specification is not to encourage for more implementations to exist and we plan to keep using the same name in the specification and implementation. The preference is for users to contribute to the existing implementation.
  • @spencerpatty we discussed how long should an RFC be up for review before being accepted. The members present suggested that October 4 should be enough time which coincide with the deadline we already planned. I'm not seeing any concerns that haven't been answered here. Unless there are more discussions here I think this deadline is reasonable.

@Rbiessy Rbiessy requested a review from a team September 25, 2024 15:21
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 4, 2024

Today is the deadline for this RFC. All the questions and comments have been addressed as far as I can tell. @mkrainiuk is the next step to merge this PR?
We are planning to start working on the suggested changes for the specification and the implementation ASAP.

@mkrainiuk
Copy link
Contributor

Today is the deadline for this RFC. All the questions and comments have been addressed as far as I can tell. @mkrainiuk is the next step to merge this PR? We are planning to start working on the suggested changes for the specification and the implementation ASAP.

Hi @Rbiessy, yes, before merging RFC could you please highlight what is the name we selected as the final one? RFC contains information about at least two options, but not what was selected at the end.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 4, 2024

Do you mean it should be clarified in the RFC itself? Currently the document says:

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

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 9, 2024

Who can give a second approval on this RFC so that we merge it @oneapi-src/onemkl-admin, @spencerpatty, @rscohn2?

@Rbiessy Rbiessy merged commit 5f620ce into oneapi-src:rfcs Oct 10, 2024
Copy link

@spencerpatty spencerpatty left a comment

Choose a reason for hiding this comment

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

I also approve of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC A proposal to add new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants