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

Replacing util::function_nonser on std::function in hpx_init #5284

Merged
merged 4 commits into from
Apr 13, 2021

Conversation

toktarev
Copy link

@toktarev toktarev commented Apr 7, 2021

Proposed Changes

Use std::function in public APIs

Any background context you want to provide?

Use std::function in public APIs: #4987

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@hkaiser
Copy link
Member

hkaiser commented Apr 7, 2021

@toktarev thanks for looking into this! Much appreciated. What compatibility implications do you see for this patch? Should we consider introducing a compatibility layer?

@toktarev
Copy link
Author

toktarev commented Apr 7, 2021

@hkaiser
Thank you very much for review.

I will add back-compatibility tests

@hkaiser
Copy link
Member

hkaiser commented Apr 7, 2021

BTW, thanks for adding the move semantics for the intermediate APIs.

Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

As for compatibility, I wonder if we could get by without that... Since we're currently using hpx::util::function_nonser (not the unique version), even if someone has explicitly created the entry point upfront and written out the type:

hpx::util::function_nonser<void()> = myentrypoint;
hpx::init(myentrypoint, ...);

even that will work. hpx::util::function_nonser can't be constructed from a noncopyable callable just like std::function so that stays the same.

libs/full/init_runtime/include/hpx/hpx_init.hpp Outdated Show resolved Hide resolved
@toktarev toktarev force-pushed the issue/4987 branch 3 times, most recently from dc058d4 to 0acc4e1 Compare April 8, 2021 10:14
@toktarev
Copy link
Author

toktarev commented Apr 8, 2021

hpx::init: hpx::exception caught: Pools empty of resources are not allowed. Please re-run this application with allow-empty-pool-policy (not implemented yet): HPX(invalid_status)
../libs/full/resource_partitioner/tests/unit/named_pool_executor.cpp(164): test 'hpx::init(argc, argv, init_args) == 0' failed in function 'int main(int, char **)': '-1' != '0'
0 sanity checks and 1 test failed.
Base command is "/Users/runner/work/hpx/hpx/build/bin/named_pool_executor_test --hpx:threads=2"
Executing command: /Users/runner/work/hpx/hpx/build/bin/named_pool_executor_test --hpx:threads=2

        Start 299: tests.unit.modules.resource_partitioner.resource_partitioner_info
299/905 Test #299: tests.unit.modules.resource_partitioner.resource_partitioner_info ......................................................***Failed    0.31 sec
../libs/full/resource_partitioner/tests/unit/resource_partitioner_info.cpp(21): test 'std::size_t(4) == hpx::resource::get_num_threads()' failed in function 'int hpx_main()': '4' != '2'

I am not sure if it was caused by my changes

hkaiser
hkaiser previously approved these changes Apr 8, 2021
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@toktarev
Copy link
Author

toktarev commented Apr 8, 2021

macOS CI (Debug) / build (pull_request) failed

Was it caused by my changes or this is regular failure ?

@msimberg
Copy link
Contributor

msimberg commented Apr 8, 2021

macOS CI (Debug) / build (pull_request) failed

Was it caused by my changes or this is regular failure ?

Regular failure, unfortunately. The macOS build is not particularly well taken care of, but nothing for you to worry about.

@msimberg
Copy link
Contributor

msimberg commented Apr 9, 2021

retest

@toktarev
Copy link
Author

toktarev commented Apr 10, 2021

#!/bin/bash -eo pipefail
cd /hpx/source/libs && shopt -s globstar # to activate the ** globbing
clang-format-9 --version
clang-format-9 -i **/*.{cpp,hpp}
git diff --exit-code > /tmp/modified_clang_format_files.txt
clang-format version 9.0.1-12 

Exited with code exit status 1
CircleCI received exit code 1
3 tests failed
  out of 597
tests.unit.modules.runtime_components.distributed.tcp.migrate_component
tests.unit.modules.runtime_components.distributed.tcp.migrate_polymorphic_component
tests.unit.modules.timed_execution.timed_parallel_executor

Is this something I should fix ?

@hkaiser
Copy link
Member

hkaiser commented Apr 10, 2021

#!/bin/bash -eo pipefail
cd /hpx/source/libs && shopt -s globstar # to activate the ** globbing
clang-format-9 --version
clang-format-9 -i **/*.{cpp,hpp}
git diff --exit-code > /tmp/modified_clang_format_files.txt
clang-format version 9.0.1-12 

Exited with code exit status 1
CircleCI received exit code 1
3 tests failed
  out of 597
tests.unit.modules.runtime_components.distributed.tcp.migrate_component
tests.unit.modules.runtime_components.distributed.tcp.migrate_polymorphic_component
tests.unit.modules.timed_execution.timed_parallel_executor

Is this something I should fix ?

These tests fail occasionally. The failures are unrelated to your changes.

@toktarev
Copy link
Author

retest

@hkaiser
Copy link
Member

hkaiser commented Apr 11, 2021

This looks good as is. It can go in.

@msimberg
Copy link
Contributor

retest

@msimberg msimberg merged commit e6c572f into STEllAR-GROUP:master Apr 13, 2021
@toktarev toktarev deleted the issue/4987 branch April 13, 2021 10:34
@hkaiser hkaiser mentioned this pull request Apr 28, 2021
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants