Assert a minimum thread count for TaskPoolOptions

[jmskov]

Objective

TaskPool policies assign one thread per group, so three threads are assigned at minimum. Adds an assertion for a minimum thread count for TaskPoolOptions. Passing 0, 1 or 2 for thread count has no real effect.

Testing

• Module tests pass

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

This needs a comment about why it's being set; ideally in the message to users.

[mockersf]

why an assert rather than

• an assert_dbg?
• returning an Option/Result?
• ignoring values below 3, and using 3 by default in those cases?

[jmskov]

why an assert rather than

• an assert_dbg?
• returning an Option/Result?
• ignoring values below 3, and using 3 by default in those cases?

I opened this after using the with_num_threads method at the plugin setup stage. I was confused why 1, 2 (and 0) all worked the same (effectively defaulted to 3) until I looked at the source. Since this method is exposed all the time, assert was the simplest way to avoid this ambiguity for others.

Not sure if debug_assert is more or less appropriate. 😄

Edit: Or a warning, just so the user knows what is happening.

[mockersf]

Edit: Or a warning, just so the user knows what is happening.

I would prefer the warning I think.

An assert usually means "if the assert fail, it wouldn't have been possible to continue executing the code in a safe way", it's not the case here

[alice-i-cecile]

Seconding the warning; I think that's a more appropriate failure level.

[jmskov]

Is it possible to raise a warning (or other message for that matter) from a constructor that is passed to add_plugins, i.e. from with_num_threads below?

.add_plugins(DefaultPlugins.set(TaskPoolPlugin {
            task_pool_options: TaskPoolOptions::with_num_threads(4),
        }))

I've tried both log::warn and tracing::warn, and neither work here. Both work in the create_default_pools system called later.

[jmskov]

I suppose it has to do with both TaskPoolPlugin and LogPlugin being in the DefaultPlugins set, because adding plugins after this can raise log messages using a constructor method. At this point, maybe it's best just to add a note to the documentation of with_num_threads and call it a day 😊

[alice-i-cecile]

I suppose it has to do with both TaskPoolPlugin and LogPlugin being in the DefaultPlugins set, because adding plugins after this can raise log messages using a constructor method. At this point, maybe it's best just to add a note to the documentation of with_num_threads and call it a day 😊

You can probably swap the ordering of the two plugins and make this work properly :)

[jmskov]

Updated to a warning.

You can probably swap the ordering of the two plugins and make this work properly :)

I can raise the warning if I add plugins separately, but the following does not raise a warning (or any trace, debug, etc.)

.add_plugins(DefaultPlugins.set(TaskPoolPlugin {    // This does not raise a warning!
             task_pool_options: TaskPoolOptions::with_num_threads(1),
}))