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

[DOCS]Update new backend guide to support the latest develop #593

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

vmalia
Copy link
Contributor

@vmalia vmalia commented Oct 15, 2024

Description

It was found that the create_new_backend.rst document is outdated and the instructions do not work with the current develop branch.

Solution/Changes

  1. Update usage of scripts to match latest header file changes (.hpp->.hxx) for actual API declarations to be picked up by the scripts.
  2. Add missing generate_ct_templates.py usage.

Test

All script (scripts/*.py) outputs now match the expected output shown in the docs.

@vmalia vmalia self-assigned this Oct 15, 2024
@vmalia vmalia requested a review from a team as a code owner October 15, 2024 05:32
@Rbiessy
Copy link
Contributor

Rbiessy commented Oct 15, 2024

I am curious, have you tried to add a dummy backend and compile the files generated by the python scripts?
We went through these instructions when we worked on adding the DFT domain a little while ago and found that using these scripts did not generate files that could be compiled. We ended up copying and adapting the files from an existing domain which was much easier.
I understand the intention of making it easier for anyone to add a backend and I think it could be a good idea if done well. I am concerned that the current instructions may instead waste the time of anyone trying to run them.
I think we should consider removing docs/create_new_backend.rst and the scripts folder, for the following reasons:

  1. Last time we tried the generated files were not compiling and not aligned with the existing domains in oneMKL. This should be verified if we want to keep support for it.
  2. It is common in software engineering to get inspiration from existing code and adapting it for a different purpose. We have found it was easier to copy existing domains or backends rather than starting from scratch.
  3. Even if we were to fix them now the python scripts are not often used or updated. They are more likely to fail or generate outdated file than work at first try.
  4. We have some relatively recent PRs that add new domains (DFT domain PR, SPARSE domain PR) or add new backends to existing domains (cuFFT backend PR, rocFFT PR or cuSPARSE PR). They are a more concrete example of what is needed to add new backends.

@@ -124,6 +138,7 @@ Below you can see structure of oneMKL top-level include directory:
<other backends>/
<other domains>/

Note: The actual structure may be different than what is shown here. To ensure your addition of a new backend is correct, verify that the above scripts correctly generate sample header files from the new files you have added.

Copy link
Contributor

Choose a reason for hiding this comment

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

MIght as well use the rST note role:

.. note::
   The actual structure…

…

@vmalia
Copy link
Contributor Author

vmalia commented Oct 18, 2024

@Rbiessy

I am curious, have you tried to add a dummy backend and compile the files generated by the python scripts?

I did try that with BLAS, as mentioned in the document and saw some issues during the CMake configure steps. I believe that part is dependent on the backend library being used/ported, so the existing documentation may not be very useful beyond the generation of the CMake config file. There is room for improvement here.

  1. Last time we tried the generated files were not compiling and not aligned with the existing domains in oneMKL. This should be verified if we want to keep support for it.
  2. It is common in software engineering to get inspiration from existing code and adapting it for a different purpose. We have found it was easier to copy existing domains or backends rather than starting from scratch.
  3. Even if we were to fix them now the python scripts are not often used or updated. They are more likely to fail or generate outdated file than work at first try.

The problem I see here is that each of the domains like BLAS, DFT etc have different ways of implementing the headers and the API declarations. This requires a different layout, which the scripts cannot manage at this time to the extent of being helpful for other domains than BLAS.

  1. We have some relatively recent PRs that add new domains (DFT domain PR, SPARSE domain PR) or add new backends to existing domains (cuFFT backend PR, rocFFT PR or cuSPARSE PR). They are a more concrete example of what is needed to add new backends.

I agree with your POV that the scripts/*.py are difficult to maintain at this time.

My recommendation is that we update the document as per this PR for now (it still works), and then have a community poll to decide if we want to keep these generation scripts or not. If the consensus is to keep them, we may try to tailor them to support the specific differences of each domain as described in the oneMKL Specification, else remove them in a separate PR. What do you think?

@Rbiessy
Copy link
Contributor

Rbiessy commented Oct 18, 2024

My recommendation is that we update the document as per this PR for now (it still works), and then have a community poll to decide if we want to keep these generation scripts or not. If the consensus is to keep them, we may try to tailor them to support the specific differences of each domain as described in the oneMKL Specification, else remove them in a separate PR. What do you think?

That's fine with me.

The problem I see here is that each of the domains like BLAS, DFT etc have different ways of implementing the headers and the API declarations. This requires a different layout, which the scripts cannot manage at this time to the extent of being helpful for other domains than BLAS.

I agree, this is part of the issue. The way I see it we have 3 different kinds of domains today:

  • BLAS and LAPACK which are mostly independent free functions
  • RNG and DFT which rely a lot on oriented-object programming
  • SPARSE_BLAS which also uses free functions but has many opaque objects to store information across functions

@vmalia
Copy link
Contributor Author

vmalia commented Oct 21, 2024

My recommendation is that we update the document as per this PR for now (it still works), and then have a community poll to decide if we want to keep these generation scripts or not. If the consensus is to keep them, we may try to tailor them to support the specific differences of each domain as described in the oneMKL Specification, else remove them in a separate PR. What do you think?

That's fine with me.

The poll is now active: #598

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