Deprecate unwrapped and implement unwrap and unwrapping #2741

Naios · 2017-07-05T13:01:35Z

This is the second PR of my GSoC 🌞 project

Some fixes to the pack traversal API
It replaces the internal implementation of the unwrapped internals to use the pack_traversal API.
The functional unwrap is now called unwrapping, the new immediate unwrap: unwrap. The API provides two alternatives for both functions: unwrap_n, unwrap_all, unwrapping_n, unwrapping_all which unwrap until a particular level of futures or all futures which are found recursively inside the arguments.
- Tuples which are passed as only argument to unwrap aren't unwrapped anymore. We could provide an explode or spread function for this behaviour.
For now all internal stuff of HPX is using a proxy function unwrapped which calls the new API unwrap.

Probably there are some remaing build failures and inconsisties which need to be fixed, however I want to receive feedback early.

For now:
✔️ hpx base library builds
✔️ unwrapped_test builds (using the unit tests of the old implementation for the new one)
✔️ unwrap_test builds
✔️ examples build
✔️ tests build

Outstanding points (need to be fixed before merging):

More unit tests for unwrap and unwrapping
Fix the remaining inconsistencies between the old implementation and the new one.
Replace using namespace
Add constexpr and noexcept
Replace unwrapped by unwrap and unwrapping
Add deprecation notice

Naios · 2017-07-07T10:26:11Z

@hkaiser did you take a look on this already?
It would be great if you could give me feedback whether I'm moving into the right direction here.

hkaiser

This looks very nice, overall.

hkaiser · 2017-07-07T21:04:17Z

hpx/util/detail/pack_traversal_impl.hpp

+            using element_of_t = typename std::conditional<
+                std::is_rvalue_reference<Container&&>::value,
+                decltype(std::move(*(std::declval<Container>().begin()))),
+                decltype(*(std::declval<Container>().begin()))>::type;


Can this be simplified (i.e. encapsulated in a separate facility)?

element_of_t also emulates the behaviour of container_accessor which means that it's quite difficult to refactor this into a seperate facility. However, I'm open for suggestions.

I just find this construct to be difficult to parse. Could you at least add an explaining comment, please?

Yes, I'll do.

I added more documentation to this alias.

hkaiser · 2017-07-07T21:05:04Z

hpx/util/detail/pack_traversal_impl.hpp

+                {
+                }
+
+                auto begin()


Can this be const?

I'll ckeck that

The begin and end method can't be a const references because we must allow the mapping from non const l-value references too (this was fixed today).

hkaiser · 2017-07-07T21:05:16Z

hpx/util/detail/pack_traversal_impl.hpp

+                    return std::make_move_iterator(container_.begin());
+                }
+
+                auto end()


Same here: const?

Same as above

hkaiser · 2017-07-07T21:07:15Z

hpx/util/detail/pack_traversal_impl.hpp

@@ -580,6 +644,13 @@ namespace util {
                    std::forward<T>(element));
            }

+            /// Boxes the given values into an according tuple
+            template <typename... T>
+            auto box(T&&... args) -> tuple<T...>


What's the rationale to use auto for return type deduction here?

Just did it for consistency, I'll replace it.

hkaiser · 2017-07-07T21:07:44Z

hpx/util/detail/pack_traversal_impl.hpp

+            template <typename... T>
+            auto box(T&&... args) -> tuple<T...>
+            {
+                return tuple<T...>{std::forward<T>(args)...};


Should this be make_tuple instead?

Actually no, The use of tuple<T...>{ ... } is the result of a earlier compilation issue with preserving l-value references across mapping, for instance this example would fail:

// Multiple args: Make it possible to pass move only objects in // as reference, while returning those as reference. { std::unique_ptr<int> ptr1(new int(6)); std::unique_ptr<int> ptr2(new int(7)); hpx::util::tuple<std::unique_ptr<int> const&, std::unique_ptr<int> const&> ref = map_pack( [](std::unique_ptr<int> const& ref) -> std::unique_ptr<int> const& { // ... return ref; }, ptr1, ptr2); HPX_TEST_EQ((*get<0>(ref)), 6); HPX_TEST_EQ((*get<1>(ref)), 7); *ptr1 = 1; *ptr2 = 2; HPX_TEST_EQ((*get<0>(ref)), 1); HPX_TEST_EQ((*get<1>(ref)), 2); }

Because make_tuple comverts l-value references to actual copies of the objects, while forward_as_tuple doesn't take ownership of r-values.

Ok, understood.

hkaiser · 2017-07-07T21:09:32Z

hpx/util/detail/unwrap_impl.hpp

+        ///       configuration.
+        ///
+        template <bool AllowsVoidFutures, bool UnwrapTopLevelTuples>
+        struct unwrap_config


Is this supposed to be visible to the user?

No, this is an implementation detail which can be removed as soon as the old unwrapped function was completely removed from the codebase.

You might want to introduce a preprocessor compatibility constant right away and use it to wrap this code.

Our policy for introducing breaking changes is to do it in 3 stages (releases):

Wrap the old API in a preprocessor macro, but leave it ON by default. Also mark the old API as deprecated (at least in the docs)

Change the macro to be OFF by default, allowing for the user to still revert to the old API

Remove the old API altogether

Thanks for the hint. How is the naming convention for such s deprecating macro?

@hkaiser Ah the actual issue here is that the code is used from both versions: the new and the old one. Thus I decided to use compile-time value based configuration here intead of preprocessor macros.
However, this way makes it difficult to remove the real old unwrapped behaviour since it will be still available in the dead paths of the configuration then.

Maybe it would be better here, to just duplicate the file into two pieces, where one can be removed when the old unwrapped is removed (this is also better for the compile time).

That's fine by me.

Well, I'll change this then as suggested as soon as I know which capabilties end up into the final version of the new unwrapped.

hkaiser · 2017-07-07T21:10:48Z

hpx/util/unwrap.hpp

+    /// - `hpx::future<int>` -> `int`
+    /// - `hpx::future<std::vector<float>>` -> `std::vector<float>`
+    /// - `std::vector<future<float>>` -> `std::vector<float>`
+    ///


Would this unwrap std::vector<std::vector<hpx::future<T>>> ?

Yes, it would. Also futures nested inside arbitrary combinations of tuples, pairs and homogeneous containers like std::list or std::vector are being remapped. For instance:

std::list< std::pair< int, std::tuple< int, std::vector< hpx::lcos::future<int> >, double > > >

is also unwrapped.

That's great. We just ran into the need for when_all() to be able to handle a vector<vector<future<T>>> the other day...

Sadly the underlying API is only capable of doing a synchronous unwrap which isn't suitable for the current implementation of when_all.
In order to improve when_all we would have to use a stackful coroutine with this API or
implement another version of the traversal API which is completely stateless (to emulate a stackless coroutine).

Yes, forgot about that. Thanks.

hkaiser · 2017-07-22T13:55:45Z

@Naios This is impressive work! The only thing I'd ask is for you to use meaningful commit messages. Otherwise it will be impossible to reconstruct the changes you applied, if necessary. You might even want to go back and to merge commits or to rewrite some of the history on this branch to make things more transparent.

Naios · 2017-07-22T15:03:27Z

@hkaiser Thank you. Yes, I'll interactively rebase the commits back into the 7 first commits, so everything looks like atomic changes, as soon as I'll have a proper state. Usually I keep the commits rebased with the current master.
Just wanted to keep the history as long as possible so I have a chance for reverts and also that you can take a look at my current progress (note that my actual working branch is Naios/hpx/unwrap so the CI isn't stressed that much).

Naios · 2017-07-23T16:20:36Z

@hkaiser For me it seems like the functional part of this PR is done now, since all the unit tests pass with the new unwrap implementation and the changes I applied recently.
I'll work on the few outstanding ToDo items now, in order to get the PR ready for merge in the next 2 days.

Additionally, I have one outstanding question regarding the originally planned compatibility layer between unwrap and unwrapped.

Since the 1:n mapping improvements, the original unwrapped and the new unwrap unwrap implementation just distingiush theirself in one point: unwrapped did unwrap the first occurring tuple regardless the callable was called with more than one argument or not. Thus tuples which were passed to unwrapped were unpacked by default as shown in the following unit test:

int add(int c1, int c2)
{
    return c1 + c2;
}

// ...

hpx::util::tuple<future<int>, future<int> > tuple =
                hpx::util::forward_as_tuple(
                    hpx::make_ready_future(42), hpx::make_ready_future(42));

HPX_TEST_EQ(unwrapped(&add)(tuple), 42 + 42);

Because uwrapped will be deprecated anyway, I would like to drop this behaviour immediatly (and delete the unit test above), so we don't need to add an additional compatibility layer (and duplicate the implementation headers). Since this particular functionality of unwrapped isn't documented nor used in the overall framework, I think we can apply this change without risking to break the API.

Do you think this is arguable?

hkaiser · 2017-07-23T18:37:31Z

That is fine. We need to add a note about the breaking change in the docs, though (here).

Also, we may be able not to rename the existing unwrapped, or do should we do that in any case?

hkaiser · 2017-07-23T18:41:44Z

@Naios: could you please also clarify what relation this PR has to #1404, #1400, and #1126?

Naios · 2017-07-23T18:45:33Z

Internally I'll rename the function calls to unwrap or unwrapping accordingly so I can flag unwrapped with HPX_DEPRECATED.
Because HPX itself and all the unit tests and examples build with the new behaviour, there is nothing against the rename in my opinion.

Because of the issues: #1404, #2456 and #1400 will be resolved, #1126 is only solved half because when_all and dataflow are still missing the changes (which require the async traversal API).
I'll add closing comments to the last commit in this PR so the issues get resolved automatically through merging.

Naios · 2017-07-24T13:10:27Z

@hkaiser From my side this PR is finished and ready for merging 🎉

hkaiser · 2017-07-24T15:04:56Z

@Naios: the only real question I have is whether we should retain the old name (at least for the functional part of unwrapped).

Naios · 2017-07-24T16:43:05Z

@hkaiser My personal opinion on this is to also take a new name for the functional part since the overall capability of the function was changed and thus library users should be forced to review this change.

But if it's decided to keep the old name for the functional implementation, I could change it back easily.
However, this would prevent users from using the current deprecated unwrapped function which stays functionable until removal (in this case we would remove the old unwrapped completely without deprecating it officially).

aserio · 2017-07-24T21:23:21Z

@Naios, Congratulations on your first pull request! After this is merged we can coordinate to get you an HPX T-shirt ;)

hkaiser · 2017-07-25T11:59:03Z

@Naios: well, we have to think about code out there using unwrapped today. We have two possible ways forward:

either we leave the name unwrapped (instead of unwrapping), or
deprecate unwrapped the usual way (in three stages, as described earlier)

I personally think we should deprecate today's unwrapped as a simple renaming doesn't cover all possible use cases.

For this reason, may I ask you to add a preprocessor constant (for instance HPX_WITH_UNWRAPPED_COMPATIBILITY/HPX_HAVE_UNWRAPPED_COMPATIBILITY) which wraps the old code (no need to try to 'emulate' the old behavior in the new code) and to mark the definition of the old unwrapped using HPX_DEPRECATED`. Also, please add a sentence or two to the docs describing the necessary changes to existing code, including the name of the configuration constant you used.

Naios · 2017-07-25T19:56:30Z

@hkaiser I applied the requested changes as discussed.

hkaiser · 2017-07-25T22:26:15Z

hpx/util/unwrapped.hpp

-    //                    "hpx::util::unwrap and hpx::util::unwrapping "
-    //                    "and might be removed in a later version of HPX!")
-    // #endif    // HPX_NO_DEPRECATE_UNWRAPPED
+#if !defined(HPX_NO_DEPRECATE_UNWRAPPED)


Where does this preprocessor constant come from? Do we really need this? I think the deprecation message should be shown unconditionally.

@hkaiser This preprocessor constant is required for the unwrapped.cpp unit test, which still uses the old implementation for testing. Since I didn't want to yield deprecation messages from the said unit test, I introduced the conditional deprecation message.
As alternative I could remove the old unit test.

...or simply let the test generate the deprecate warning...

Wouldn't it be better not to generate any warnings so we are still able to build the tests using -Werror?

Ok, let's then rather remove the unit test for a feature which will be removed anyways.

@hkaiser I removed the unit test so this point should be resolved

* Partially re-uses the previous tests of tests/unit/util/unwrapped.cpp

* Correct the naming of some functions

* Prevents wrong function selection caused through ADL * Also namespace a call to make_tuple

* Ref STEllAR-GROUP#2782

* Use hpx::util::unwrap and hpx::util::unwrapping instead, that clearify which underlying implementation should be used (the immediate or the deferred one). The automatic implementation selection was broken since unwrapped allowed to pass non future arguments through. * Closes STEllAR-GROUP#1400 * Closes STEllAR-GROUP#1404 * Closes STEllAR-GROUP#2456 * Ref STEllAR-GROUP#1126 * Ref STEllAR-GROUP#1132

hkaiser

LGTM, thanks a lot!

taeguk · 2017-07-29T08:33:56Z

@Naios What compilers this PR was tested with?
In Visual Studio 2015, I found compile error messages related to this PR.

c:\users\xornr\onedrive\documents\github\hpx\hpx\util\detail\unwrap_impl.hpp(176): error C2297: '&&': 오른쪽 피연산자 형식으로 'Unique-Type-value'을(를) 사용할 수 없습니다.

There is the error message above. (Sorry to korean error message. Maybe you need to translate the message to english.)

Naios · 2017-07-29T11:57:09Z

@taeguk I'm sorry because it seems the PR was only tested with with Visual Studio 2017.
I blindly trusted the AppVeyor CI which uses Visual Studio 2017 too, I totally missed that Visual Studio 2015 is still supported.

What you could try is to patch your locale Visual Studio version to Visual Studio 2015 Update 3, which eventually could fix your issue.
Beside of this I'll take a look on this.

@hkaiser Is the current minimum compiler version for HPX still Visual Studio 2015 as described in the docs or could it be raised to Visual Studio 2017?
It would be great if the documentation could also state the patch version of Visual Studio, which preferably should be Visual Studio Update 3, since the previous updates were not stable.
Also maybe we could align the Appveyor Visual Studio version with the minimum required version to avoid such issues in the future.

hkaiser · 2017-07-29T14:40:18Z

Is the current minimum compiler version for HPX still Visual Studio 2015 as described in the docs or could it be raised to Visual Studio 2017?

So far we've had the policy to support the latest two versions of Visual Studio. We just dropped support for VS 2013 the other day,

It would be great if the documentation could also state the patch version of Visual Studio, which preferably should be Visual Studio Update 3, since the previous updates were not stable.

Ok, patches welcome. Anybody?

Also maybe we could align the Appveyor Visual Studio version with the minimum required version to avoid such issues in the future.

That should be doable as well.

hkaiser · 2017-07-29T14:45:38Z

@Naios: another option would be to leave the old unwrapped code in place when compiling with VS2015. I don't particularly care about this version as VS2017 is ABI compatible and everybody can simply download the newer one. This however would require some discussion, I'd like to hear the opinion of others.

* Ref STEllAR-GROUP#2741

* Thanks to K-ballo for pointing this out * We use hpx::util::get now for retrieving a specific element at index i instead of the internal tuple_element API. * Ref STEllAR-GROUP#2741

Naios · 2017-07-29T20:55:38Z

@hkaiser @taeguk I provided the fixes for both known issues of this PR in #2787.
The MSVC 2015 issue should be fixed.

* Thanks to K-ballo for pointing this out * We use hpx::util::get now for retrieving a specific element at index i instead of the internal tuple_element API. * Ref STEllAR-GROUP#2741

taeguk · 2017-07-30T07:23:46Z

@Naios I found that issue with Visual Studio 2015 Enterprise Update 3.
Now, is my issue fixed??

* Ref STEllAR-GROUP#2741

* Thanks to K-ballo for pointing this out * We use hpx::util::get now for retrieving a specific element at index i instead of the internal tuple_element API. * Ref STEllAR-GROUP#2741

Naios · 2017-07-30T11:37:20Z

@taeguk I'm really sorry for the inconveniences.
The build fix for MSVC 2015 is provided in #2787.

As temporary workaround you may cherry-pick the fix locally:

git remote add msvcfix https://github.com/Naios/hpx.git
git fetch msvcfix
git cherry-pick 320f515335b43f0342

Or as alternative you could exclude this PR from your locale history through interactive rebasing git rebase -i 2bd90f07b3a5.

taeguk · 2017-07-30T17:03:51Z

@Naios Thanks for your fast fix!
Don't worry for me. I'm already doing my works in the point which this PR is not applied.

* Thanks to K-ballo for pointing this out * We use hpx::util::get now for retrieving a specific element at index i instead of the internal tuple_element API. * Ref STEllAR-GROUP#2741

Naios changed the title ~~Replace the unwrapped internals for using the pack_traversal API~~ [WIP] Replace the unwrapped internals for using the pack_traversal API Jul 5, 2017

hkaiser added category: core type: enhancement labels Jul 5, 2017

hkaiser added this to the 1.1.0 milestone Jul 5, 2017

Naios force-pushed the unwrap_pr branch from e3c0cec to 1362039 Compare July 7, 2017 14:27

hkaiser reviewed Jul 7, 2017

View reviewed changes

Naios force-pushed the unwrap_pr branch from 1362039 to 422b968 Compare July 11, 2017 12:40

Naios force-pushed the unwrap_pr branch from 422b968 to 4cab436 Compare July 19, 2017 14:36

Naios force-pushed the unwrap_pr branch 3 times, most recently from a2e97b1 to f75bb35 Compare July 23, 2017 10:52

Naios force-pushed the unwrap_pr branch from f75bb35 to b039e3b Compare July 23, 2017 21:12

Naios changed the title ~~[WIP] Replace the unwrapped internals for using the pack_traversal API~~ Deprecate unwrapped and implement unwrap and unwrapping Jul 24, 2017

Naios force-pushed the unwrap_pr branch from 28609cd to 00a906f Compare July 25, 2017 11:08

Naios force-pushed the unwrap_pr branch from 00a906f to 3c0bdf2 Compare July 25, 2017 19:55

Naios force-pushed the unwrap_pr branch from 3c0bdf2 to 790392c Compare July 25, 2017 20:17

hkaiser reviewed Jul 25, 2017

View reviewed changes

Naios added 9 commits July 27, 2017 11:25

Implement unit tests for unwrap and unwrapping

c093f47

* Partially re-uses the previous tests of tests/unit/util/unwrapped.cpp

Remove using namespace in the pack_traversal unit test

35bd75b

* Correct the naming of some functions

Rename the internal mapper invoke to invoke_mapper

c708fea

* Prevents wrong function selection caused through ADL * Also namespace a call to make_tuple

Add constexpr and noexcept to the pack_traversal API

1ba82ee

Use unwrap and unwrapping in the core library

b6545ba

Use unwrap and unwrapping in the examples

e4465fc

Use unwrap and unwrapping in the tests

347be2f

Replace the usage of boost::atomic in the unwrap test

6a32d0c

* Ref STEllAR-GROUP#2782

Naios force-pushed the unwrap_pr branch from 790392c to 5982cf2 Compare July 27, 2017 09:36

hkaiser approved these changes Jul 27, 2017

View reviewed changes

hkaiser merged commit 8ba2a4e into STEllAR-GROUP:master Jul 28, 2017

Naios added a commit to Naios/hpx that referenced this pull request Jul 29, 2017

Fix a parsing error with Visual Studio 2015

058542c

* Ref STEllAR-GROUP#2741

Naios added a commit to Naios/hpx that referenced this pull request Jul 29, 2017

Fix a parsing error with Visual Studio 2015

aff3122

* Ref STEllAR-GROUP#2741

Naios mentioned this pull request Jul 29, 2017

Unwrap hotfixes #2787

Merged

Naios added a commit to Naios/hpx that referenced this pull request Jul 30, 2017

Fix a parsing error with Visual Studio 2015 which occurred in unwrap

320f515

* Ref STEllAR-GROUP#2741

msimberg mentioned this pull request Nov 21, 2017

vector<future<T> > does not work gracefully with dataflow, when_all and unwrapped #1126

Closed

Deprecate unwrapped and implement unwrap and unwrapping #2741

Deprecate unwrapped and implement unwrap and unwrapping #2741

Conversation

Naios commented Jul 5, 2017 • edited

Naios commented Jul 7, 2017 • edited

hkaiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Jul 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naios Jul 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser commented Jul 22, 2017

Naios commented Jul 22, 2017 • edited

Naios commented Jul 23, 2017 • edited

hkaiser commented Jul 23, 2017

hkaiser commented Jul 23, 2017

Naios commented Jul 23, 2017 • edited

Naios commented Jul 24, 2017

hkaiser commented Jul 24, 2017

Naios commented Jul 24, 2017

aserio commented Jul 24, 2017

hkaiser commented Jul 25, 2017

Naios commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaiser left a comment

Choose a reason for hiding this comment

taeguk commented Jul 29, 2017

Naios commented Jul 29, 2017 • edited

hkaiser commented Jul 29, 2017

hkaiser commented Jul 29, 2017

Naios commented Jul 29, 2017 • edited

taeguk commented Jul 30, 2017

Naios commented Jul 30, 2017 • edited

taeguk commented Jul 30, 2017

Naios commented Jul 5, 2017 •

edited

Naios commented Jul 7, 2017 •

edited

Naios Jul 9, 2017 •

edited

Naios Jul 10, 2017 •

edited

Naios Jul 11, 2017 •

edited

Naios Jul 9, 2017 •

edited

Naios commented Jul 22, 2017 •

edited

Naios commented Jul 23, 2017 •

edited

Naios commented Jul 23, 2017 •

edited

Naios commented Jul 29, 2017 •

edited

Naios commented Jul 29, 2017 •

edited

Naios commented Jul 30, 2017 •

edited