-
Notifications
You must be signed in to change notification settings - Fork 12
Add oneDPL Public CI state and requirements. #222
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. I think it should be everything
* Add oneMKL info to project-ci-documentation.md * Correct some typos * Update project-ci-documentation.md * Update project-ci-documentation.md
| gmake | clang++ compiler | | | ||
| ninja | g++ | | | ||
| Visual Studio 2019 | | | | ||
| Visual Studio 2022 | | | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also mention the supported GPU driver and their versions. It is a yet another tool oneDPL depends on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too much detail at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question of how much detail is needed in this document was discussed in the UXL Open Source Working Group meeting. The feedback received is that any details on configurations teams can provide is useful, so I will add the details suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* CMake | ||
* glibc | ||
* ... | ||
| Windows | Linux | MacOS (Arm CPU testing) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more readable to have "Software" or "Tool" as a first column. The cells may contain for example: minimal version number, "any" or "-" (not needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version information is too much detail in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different product on one line still bothers me (such as dpc++ compiler and clang++ compiler). I suggest making a bullet list.
You can ignore this comment though - it is a matter of taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the Software column.
| Dan Hoeflinger @danhoeflinger | | ||
| Dmitriy Sobolev @dmitriy-sobolev | | ||
| Timmie Smith @timmiesmith | | ||
|
||
*Existing public CI* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to mention how to run this CI, e.g. that it is automatically triggered in oneDPL repository when a PR is opened and a new commit is made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too much detail for what is being requested at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thank you for the comments, @dmitriy-sobolev . There are several suggestions that I think would be too detailed for this document. Version numbers for drivers and software tools are something that should be captured in oneDPL system requirements documentation. |
Testing on CPU platforms must exercise several execution policies with OpenMP and oneTBB to cover all of the oneDPL backends. | ||
| Processor Type | Execution Policy | | ||
| --- | --- | | ||
| CPU | seq, unseq, par (using oneTBB), par (using OpenMP), par\_unseq(using oneTBB), par\_unseq(using OpenMP) | | ||
| GPU | device\_policy | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify things, I would replace the 2x2 table with a bullet list:
Testing on CPU platforms must exercise several execution policies with OpenMP and oneTBB to cover all of the oneDPL backends. | |
| Processor Type | Execution Policy | | |
| --- | --- | | |
| CPU | seq, unseq, par (using oneTBB), par (using OpenMP), par\_unseq(using oneTBB), par\_unseq(using OpenMP) | | |
| GPU | device\_policy | | |
oneDPL testing must cover all execution policies. Different device types support different sets of policies: | |
* CPU: seq, unseq, par, par_unseq, device_policy | |
* GPU: device_policy | |
The par and par_unseq policies must be tested with both oneTBB and OpenMP backends. |
It breaks the flow with its GPU to CPU transition. Adapt the above if you find it useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* CMake | ||
* glibc | ||
* ... | ||
| Windows | Linux | MacOS (Arm CPU testing) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different product on one line still bothers me (such as dpc++ compiler and clang++ compiler). I suggest making a bullet list.
You can ignore this comment though - it is a matter of taste.
| Visual Studio 2019 | g++ | | | ||
| Visual Studio 2022 | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave only one Visual Studio major version (2022) - it should be enough for a minimal CI requirements. I would say that one version is enough in 99% cases.
Also, let's be careful for the name of the product. DPC++ system requirements, for example, list it as Microsoft Visual Studio*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…#223) * introduce initial version of CI descrition * Apply suggestions from code review Co-authored-by: ethanglaser <42726565+ethanglaser@users.noreply.github.com> Co-authored-by: Andreas Huber <9201869+ahuber21@users.noreply.github.com> * fixing links and typos * fixing links and typos * adding sklearnex ci * adding sklearnex ci * fixing typo * Apply suggestions from code review Co-authored-by: ethanglaser <42726565+ethanglaser@users.noreply.github.com> * Update project-infrastructure/project-ci-documentation.md * Update project-infrastructure/project-ci-documentation.md --------- Co-authored-by: ethanglaser <42726565+ethanglaser@users.noreply.github.com> Co-authored-by: Andreas Huber <9201869+ahuber21@users.noreply.github.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
Rebase has mangled the changeset. Closing this PR and will open a clean PR with just the oneDPL CI changes. |
Initial capture of the current state of oneDPL Public CI and what would be needed to fully test community contributions.
Fixes #208