[MISC] Config parallel with member thread_count by joergi-w · Pull Request #1917 · seqan/seqan3

joergi-w · 2020-06-17T11:22:31Z

This PR adds a member variable thread_count to the parallel config (it replaces the old value member). It influences both seqan3::align_cfg::parallel and seqan3::search_cfg::parallel.

Documentation and tests are included in the second commit.

Closes seqan/product_backlog#68 and contributes to seqan/product_backlog#131.

codecov · 2020-06-17T12:49:13Z

Codecov Report

Merging #1917 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #1917    +/-   ##
========================================
  Coverage   97.76%   97.76%            
========================================
  Files         253      258     +5     
  Lines        9560     9684   +124     
========================================
+ Hits         9346     9468   +122     
- Misses        214      216     +2

Impacted Files	Coverage Δ
...clude/seqan3/alignment/pairwise/align_pairwise.hpp	`100.00% <100.00%> (ø)`
.../algorithm/configuration_element_parallel_mode.hpp	`100.00% <100.00%> (ø)`
.../seqan3/alignment/pairwise/alignment_algorithm.hpp	`98.50% <0.00%> (-1.50%)`	⬇️
include/seqan3/range/views/drop.hpp	`100.00% <0.00%> (ø)`
include/seqan3/io/stream/iterator.hpp	`98.55% <0.00%> (ø)`
include/seqan3/range/views/repeat.hpp	`100.00% <0.00%> (ø)`
include/seqan3/alphabet/nucleotide/dna4.hpp	`100.00% <0.00%> (ø)`
include/seqan3/core/detail/debug_stream_tuple.hpp	`100.00% <0.00%> (ø)`
...ude/seqan3/alignment/pairwise/alignment_result.hpp	`100.00% <0.00%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f799c0...068f50a. Read the comment docs.

joergi-w · 2020-06-17T12:59:09Z

+    //!\}
+
+    //!\brief The maximum number of threads the algorithm can use.
+    uint32_t thread_count{1u};


Question: Do we want std::thread::hardware_concurrency as a default here?

hmm good question. I would vote for yes. I'll ask @seqan/core

simonsasse

Thanks for the explanation ! Just found two lines which are slightly too long. :)

joergi-w · 2020-06-23T15:37:56Z

Rebased on master.

simonsasse

Thanks :)

smehringer

Some small stuff :)

smehringer · 2020-06-24T08:14:30Z

+     * \param[in] thread_c The maximum number of threads to be used by the algorithm.
+     */
+    explicit parallel_mode(uint32_t thread_c) noexcept : thread_count{thread_c}


Suggested change

* \param[in] thread_c The maximum number of threads to be used by the algorithm.

*/

explicit parallel_mode(uint32_t thread_c) noexcept : thread_count{thread_c}

* \param[in] thread_count_ The maximum number of threads to be used by the algorithm.

*/

explicit parallel_mode(uint32_t thread_count_) noexcept : thread_count{thread_count_}

:nail_care:

smehringer · 2020-06-24T08:15:46Z

+    parallel_mode(parallel_mode &&) noexcept = default; //!< Defaulted.
+    parallel_mode & operator=(parallel_mode const &) = default; //!< Defaulted.
+    parallel_mode & operator=(parallel_mode &&) noexcept = default; //!< Defaulted.


Suggested change

parallel_mode(parallel_mode &&) noexcept = default; //!< Defaulted.

parallel_mode & operator=(parallel_mode const &) = default; //!< Defaulted.

parallel_mode & operator=(parallel_mode &&) noexcept = default; //!< Defaulted.

parallel_mode(parallel_mode &&) = default; //!< Defaulted.

parallel_mode & operator=(parallel_mode const &) = default; //!< Defaulted.

parallel_mode & operator=(parallel_mode &&) = default; //!< Defaulted.

or did you define this on purpose? because usually for = default constructors this is added automatically.

I added noexcept on purpose, because when I left it away, CLion told me:

Clang-Tidy: Move constructors should be marked noexcept. Clang-Tidy: Move assignment operators should be marked noexcept.

If it's our policy I can remove them now.

Yes :)
See here: https://en.cppreference.com/w/cpp/language/noexcept_spec that they are noexcept if they can.

smehringer · 2020-06-24T08:17:49Z

+    //!\}
+
+    //!\brief The maximum number of threads the algorithm can use.
+    uint32_t thread_count{1u};


hmm good question. I would vote for yes. I'll ask @seqan/core

rrahn · 2020-06-24T11:48:12Z

What should be the default thread count if the user does not specifies anything?

Resolution 24.06.2020

The option should default to std::nullopt if default constructed and throws in the configuration if not set by the user explicitly.

joergi-w · 2020-06-24T12:09:56Z

I cannot assign std::nullopt to a uint32_t type. A solution is to make the thread_count variable of type std::optional<uint32_t> and then we have to access the variable as config.thread_count.value or config->thread_count. Is this intended or do you have another solution in mind?

eseiler · 2020-06-24T12:21:48Z

I cannot assign std::nullopt to a uint32_t type. A solution is to make the thread_count variable of type std::optional<uint32_t> and then we have to access the variable as config.thread_count.value or config->thread_count. Is this intended or do you have another solution in mind?

Yes, std::optional was the intention

smehringer

two small things. thanks for the change to std::optional

smehringer · 2020-06-25T11:17:30Z

+    else if constexpr (std::same_as<type_param_t, seqan3::align_cfg::parallel>)
    {
-        auto && config = cfg | type_param_t{};
+        auto && config = cfg | seqan3::align_cfg::parallel{4};


I don't understand this change?

type_param_t is either void or seqan3::align_cfg::parallel, because we want to test the serial and the parallel version. And if it is parallel, we do not want to use the default initialisation, because half of the tests would fail with the std::bad_optional_access exception. So I initialise the parallel tests with 4.

thank you for the explanation :)

smehringer · 2020-06-25T11:25:01Z

+            return execution_handler_t{get<align_cfg::parallel>(complete_config).thread_count.value()};
        else
            return execution_handler_t{};


Suggested change

return execution_handler_t{get<align_cfg::parallel>(complete_config).thread_count.value()};

else

return execution_handler_t{};

{

if (!get<align_cfg::parallel>(complete_config).thread_count.has_value())

throw std::runtime_error{"You must configure the number of threads in seqan3::align_cfg::parallel."};

return execution_handler_t{get<align_cfg::parallel>(complete_config).thread_count.value()};

}

else

{

return execution_handler_t{};

}

I think this will be more easy to understand for the user than a std::bad_optional_acces ?
Maybe rephrase the sentence, it's just a proposal :)

Yes, I like this suggestion :)

…fg::parallel

joergi-w requested review from a team and simonsasse and removed request for a team June 17, 2020 11:22

joergi-w commented Jun 17, 2020

View reviewed changes

simonsasse suggested changes Jun 22, 2020

View reviewed changes

Comment thread test/unit/alignment/configuration/align_config_parallel_test.cpp Outdated

Comment thread test/unit/alignment/configuration/align_config_parallel_test.cpp Outdated

joergi-w requested a review from simonsasse June 22, 2020 11:18

joergi-w added 3 commits June 23, 2020 17:37

[MISC] parallel config has a member thread_count

d1b6745

[DOC,TEST] documentation and tests for parallel config

9ea939a

[TEST] fix line length

ea28521

joergi-w force-pushed the config_parallel branch from 8484ba1 to ea28521 Compare June 23, 2020 15:37

simonsasse approved these changes Jun 24, 2020

View reviewed changes

joergi-w requested review from a team and smehringer and removed request for a team June 24, 2020 07:08

smehringer requested changes Jun 24, 2020

View reviewed changes

[DOC] add a snippet which uses thread_count

64288f7

[API] thread_count becomes std::optional

3b7487c

joergi-w requested a review from smehringer June 25, 2020 07:31

smehringer requested changes Jun 25, 2020

View reviewed changes

[MISC] throw runtime_error instead of bad_optional_access for align_c…

068f50a

…fg::parallel

joergi-w requested a review from smehringer June 25, 2020 14:52

smehringer approved these changes Jun 26, 2020

View reviewed changes

smehringer merged commit 39ee4a2 into seqan:master Jun 26, 2020

joergi-w deleted the config_parallel branch June 30, 2020 08:39

eseiler mentioned this pull request Jul 6, 2020

[FIX] Broken parallel search #1969

Merged

-     * \param[in] thread_c The maximum number of threads to be used by the algorithm.
-     */
-    explicit parallel_mode(uint32_t thread_c) noexcept : thread_count{thread_c}
+     * \param[in] thread_count_ The maximum number of threads to be used by the algorithm.
+     */
+    explicit parallel_mode(uint32_t thread_count_) noexcept : thread_count{thread_count_}
+:nail_care:

Uh oh!

Conversation

joergi-w commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonsasse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joergi-w commented Jun 23, 2020

Uh oh!

simonsasse left a comment

Choose a reason for hiding this comment

Uh oh!

smehringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rrahn commented Jun 24, 2020

Resolution 24.06.2020

Uh oh!

joergi-w commented Jun 24, 2020

Uh oh!

eseiler commented Jun 24, 2020

Uh oh!

smehringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joergi-w commented Jun 17, 2020 •

edited

Loading

codecov Bot commented Jun 17, 2020 •

edited

Loading

smehringer left a comment •

edited

Loading