oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Add distributed ranges as experimental feature.

Open BenBrock opened this issue 10 months ago • 9 comments

This draft PR adds distributed ranges as an experimental feature, inside the onedpl::experimental::dr namespace.

BenBrock avatar Apr 04 '24 23:04 BenBrock

Distributed Ranges branch is ready to be reviewed by oneDPL developers.

It passed all existing oneDPL CI tasks: https://github.com/oneapi-src/oneDPL/actions/runs/9128713306

It passed our tests executed by CI running in our repo that checkouts and run distributed ranges tests existing on the distributed-ranges branch of the oneDPL repo: https://github.com/oneapi-src/distributed-ranges/actions/workflows/onedpl.yml

I've just reviewed it again myself after finishing all code movement tasks and I've finished all cleanups I found myself in the code.

lslusarczyk avatar May 17 '24 17:05 lslusarczyk

@akukanov It seems we addressed all comments that you had so far. Are there more comments? If not, then could we please work toward merging this code to main branch?

Tests are passing. We have also added DR UTs to your CI and it is passing. I think it is better when this code will be sooner than later on main branch and will be tested during every PR. In case of new issues found later we will resolve them on main branch.

What do you think?

lslusarczyk avatar Jun 19 '24 12:06 lslusarczyk

This is a first pass with higher-level comments.

@julianmi , thank you for review. I've applied and answered all your comments. Please resolve discussions which you think I addressed correctly or answer discussions which you think need future work/discussion. I'm looking forward for the next pass of comments.

lslusarczyk avatar Jun 26 '24 11:06 lslusarczyk

@julianmi , I've applied unresolved discussions from yesterday. Please check if all is ok now and resolve what is ok.

lslusarczyk avatar Jun 27 '24 08:06 lslusarczyk

@akukanov , all, Here is documentation of Distributed-Ranges API which you may both use as a help during review and you may review it itself checking if it is clear enough.

lslusarczyk avatar Jul 01 '24 13:07 lslusarczyk

A couple of comments regarding the CI.

@julianmi , thank you for your valuable comments. In https://github.com/oneapi-src/oneDPL/pull/1663 PR I've refactored Distributed Ranges CI to be tested in the same way other oneDPL tests are run. Please take a look and resolve discussions if it looks good to you now.

lslusarczyk avatar Jul 09 '24 14:07 lslusarczyk

@danhoeflinger , thank you for the review and valuable comments. I've applied or answered all of them except https://github.com/oneapi-src/oneDPL/pull/1479#discussion_r1670894363 , which needs some more work and experiments from my side - I will apply this one tomorrow. In the meantime please check if other comments are well applied and resolve the ones which are OK and comment the ones which aren't OK.

lslusarczyk avatar Jul 10 '24 18:07 lslusarczyk

In this PR we have a lot of destructors, declared as = default. I understand why we need that in case when destructor also declared as virtual. But for that in other cases?

Exaxples:

~iterator_adaptor() = default;
~direct_iterator() noexcept = default;
~device_ptr() noexcept = default;
```;
and so far and so on...

SergeyKopienko avatar Jul 24 '24 08:07 SergeyKopienko

In this PR we have a lot of destructors, declared as = default. I understand why we need that in case when destructor also declared as virtual. But for that in other cases?

Exaxples:

~iterator_adaptor() = default;
~direct_iterator() noexcept = default;
~device_ptr() noexcept = default;
```;
and so far and so on...

applied in https://github.com/oneapi-src/oneDPL/pull/1745

lslusarczyk avatar Aug 01 '24 10:08 lslusarczyk