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.

Sign up for GitHub

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

Jump to bottom

feat(samples): add CMakeLists.txt for mpi/ccl samples #2688

Merged
merged 1 commit into from
Apr 26, 2024
Merged

feat(samples): add CMakeLists.txt for mpi/ccl samples #2688

merged 1 commit into from
Apr 26, 2024

Conversation

homksei
Copy link
Collaborator

@homksei homksei commented Mar 11, 2024 •
edited
Loading

No description provided.

@homksei homksei changed the title feat(chore): add CMakefile.txt for mpi/ccl samples feat(chore): add CMakeLists.txt for mpi/ccl samples Mar 11, 2024
@homksei homksei force-pushed the feat-cmake-samples branch 2 times, most recently from 3d9ae0f to ce16ccb Compare March 11, 2024 20:03
@homksei homksei changed the title feat(chore): add CMakeLists.txt for mpi/ccl samples feat(samples): add CMakeLists.txt for mpi/ccl samples Mar 11, 2024
@homksei homksei force-pushed the feat-cmake-samples branch 10 times, most recently from a67985f to a93588a Compare March 14, 2024 10:51
@homksei homksei marked this pull request as ready for review March 14, 2024 14:16
ethanglaser
ethanglaser reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@ethanglaser ethanglaser left a comment

Choose a reason for hiding this comment

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

Nice work on this, but have a few questions that may be resolved by addition of a description:
Is just public CI LinuxMake_GNUMKL to be impacted by these changes? Are the same changes meant for Linux DPCPP public CI job as well or is that still to be make? Are oneapi/dpc ccl and mpi CMakeLists.txts used anywhere currently (ie any internal CI)?

samples/cmake/setup_samples.cmake Outdated Show resolved Hide resolved
samples/oneapi/dpc/ccl/CMakeLists.txt Outdated Show resolved Hide resolved
@homksei
Copy link
Collaborator Author

homksei commented Apr 1, 2024

Nice work on this, but have a few questions that may be resolved by addition of a description:

Is just public CI LinuxMake_GNUMKL to be impacted by these changes? Are the same changes meant for Linux DPCPP public CI job as well or is that still to be make? Are oneapi/dpc ccl and mpi CMakeLists.txts used anywhere currently (ie any internal CI)?

mpi samples are launched only for LinuxMake_GNUMKL in public CI. If we decide to add it to the Linux DPCPP public CI job, they can also be launched using CMake. I would also suggest deprecating the ability to run mpi/ccl samples through make (and .bat in the case of Windows) and leave only CMake.

@napetrov
Copy link
Contributor

napetrov commented Apr 1, 2024

Yes, moving to cmake we can drop other methods

@ethanglaser
Copy link
Contributor

ethanglaser commented Apr 1, 2024 •
edited
Loading

mpi samples are launched only for LinuxMake_GNUMKL in public CI. If we decide to add it to the Linux DPCPP public CI job, they can also be launched using CMake.

Looks like we are launching mpi samples in Linux DPCPP public CI step - https://github.com/oneapi-src/oneDAL/blob/main/.ci/pipeline/ci.yml#L204-L207

@homksei
Copy link
Collaborator Author

homksei commented Apr 2, 2024

mpi samples are launched only for LinuxMake_GNUMKL in public CI. If we decide to add it to the Linux DPCPP public CI job, they can also be launched using CMake.

Looks like we are launching mpi samples in Linux DPCPP public CI step - https://github.com/oneapi-src/oneDAL/blob/main/.ci/pipeline/ci.yml#L204-L207

Thank you for bringing this to my attention. I completely forgot that it's enabled there. Switched to CMake there too.

Alexandr-Solovev
Alexandr-Solovev approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@Alexandr-Solovev Alexandr-Solovev left a comment

Choose a reason for hiding this comment

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

Nice work!

@napetrov
Copy link
Contributor

@homksei @ethanglaser @Alexandr-Solovev - should we go ahead and integrate?

@ethanglaser ethanglaser merged commit 71b5f29 into oneapi-src:main Apr 26, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alexandr-Solovev Alexandr-Solovev Alexandr-Solovev approved these changes

@ethanglaser ethanglaser ethanglaser approved these changes

@napetrov napetrov napetrov approved these changes

@Alexsandruss Alexsandruss Awaiting requested review from Alexsandruss Alexsandruss is a code owner

@samir-nasibli samir-nasibli Awaiting requested review from samir-nasibli samir-nasibli is a code owner

@ahuber21 ahuber21 Awaiting requested review from ahuber21 ahuber21 is a code owner

@maria-Petrova maria-Petrova Awaiting requested review from maria-Petrova maria-Petrova is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@homksei @napetrov @ethanglaser @Alexandr-Solovev