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

runtime: Control over minimum block noutput item size is not respected #3930

Open
jacobagilbert opened this issue Nov 11, 2020 · 3 comments
Open

Comments

@jacobagilbert
Copy link
Contributor

I have been looking into this issue on and off over the past few years. This is an update to #1483.

Expected Behavior

The set_min_noutput_items() and set_max_noutput_items() controls seem straight forward. "call my work function with no fewer (or no more) than N items, however within block executor this is not handled in a straightforward way. One would reasonably expect to see your work function called with at least min_noutput_items() and not more than max_noutput_items().

Observed Behavior

The maximum noutput items is generally applied as expected, but the minimum noutput items is usually (always?) effectively ignored. This is due to several undocumented reasons. The first, is that you cannot set this value larger than the output buffer size. While this may seem obvious, it is not documented and has tripped up many people including myself.

More importantly, block_executor treats this value with little regard. In fact, i have been completely unable to get this to affect the behavior of a running block whatsoever (#1483 was not that far off...). This is because of several reasons:

  1. When checking the minimum available space, the resulting value can be arbitrarily reduced below min_noutput_items if there is less space_available() in the output buffer. While this is necessary, its unclear to a user that this would be the case as opposed to not running the block until more space is available.

  2. This value is used with other values as arguments to min() function potentially reducing it, and it is rounded down to the nearest output multiple if set.

  3. If a list of checks do not pass, it is arbitrarily reduced by a factor of two and forecast is called again. It seems this is the biggest part of the problem.

Suggestions

For 3.9 I think this should simply be documented as making changes here seems fairly risky and clearly this has existed for a very long time without causing widespread problems. Most users who have this issue use the set output multiple to circumvent it. Moving forward in to 4.0 (looking at @mormj here), I think it is important that users have the ability to set a strict limit on the minimum and maximum output sizes.

@willcode
Copy link
Member

Agree that for both algorithm and performance reasons, min should be min. Especially when expensive transfers could happen.

Might have to be some checking to keep the user from specifying a deadlock. The assumption could be that a user who specified a bad set of constraints is savvy enough to understand the error.

@marcusmueller
Copy link
Member

need to have a sensible test suite for this. I think we fixed the python problem for these functions, but whether that addresses the issue here: determining that would require a split C++/Python test

@marcusmueller
Copy link
Member

Aang23 reproduced this issue on current git; further investigation ongoing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants