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.

Sign up for GitHub

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

Jump to bottom

In submit_iteration, don't send None values to the future #236

Closed
wants to merge 1 commit into from
Closed

In submit_iteration, don't send None values to the future #236

wants to merge 1 commit into from

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Nov 17, 2020 •
edited

The submit_iteration function provides a way to run a generator in the background, with yield statements in that generator marking potential cancellation points.

Currently, each yield statement also sends a message to the foreground future, making those yields somewhat heavyweight.

This PR modifies the code so that:

  • a plain yield (or equivalently, a yield None) marks a cancellation point, but does not send a value to the future
  • a statement of the form yield value with value non-None both marks a cancellation point and sends a value to the future

This is a significant change in user-visible behaviour. Technically it's backwards incompatible, but the likelihood of breakage seems small (and the fix for any such breakage should be very easy). Nevertheless, we need to advertise the change properly, so I've added a changelog entry to make sure that this doesn't get neglected at release time.

Closes #229.

mdickinson
mdickinson commented Nov 17, 2020
View reviewed changes
traits_futures/background_iteration.py
@@ -54,7 +54,13 @@ def __call__(self, send, cancelled):
# exception carries that value. Return it.
return e.value

send(GENERATED, result)
# A plain yield without an argument should not send a message; it's
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this comment needs to be rewritten, or possibly simply removed. The reference to yield doesn't make sense in the case that what's been submitted is a simple iterator rather than a generator created by a generator function.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could potentially simply point to the submit_iteration function instead of repeating a portion/all of the docstring from that function here.

rahulporuri
rahulporuri approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM.

@rahulporuri
Copy link
Contributor

rahulporuri commented Nov 19, 2020 •
edited

Just to confirm, the final value of a future can be None. But, when the background task is still executing, the future will never be in a None state . There is no way for the user to explicitly set it to None.

I can't think of a good reason why anyone would want the future to be None when the background task is still being executed. I'm trying to think of a future which is keeping track of I/O operations to disk or a database instead of a computation (which I think is possible with traits-futures) but even there, I can't think of a good reason for the future to be in a None state before completion.

@mdickinson
Copy link
Member Author

Just to confirm, the final value of a future can be None.

Yes; this change doesn't affect the return value of the future (the IterationFuture.result attribute) in any way.

But, when the background task is still executing, the future will never be in a None state . There is no way for the user to explicitly set it to None.

So it's true that the user can't ever send a None to the result_event trait on the IterationFuture. (I don't really think of the yielded messages as representing the future's state - they could represent any sort of communication from the background process.)

This is a bit of an uneasy compromise. There are two distinct use-cases:

  1. The user genuinely wants to submit an iterable and iterate over it in the background; in that case, dropping None values coming from the iterable would be quite surprising.
  2. The user is executing a function and wants to add yield statements to it for both cancellation purposes and for sending progress or partial result information to the foreground process.

In the first case, it's the contents of the iterable that are of interest to the user. In the second case, it's the result that the user is after, and the yielded partial results are just icing on the cake.

Possibly we actually want two separate job types. Let me rethink this one.

@mdickinson
Copy link
Member Author

mdickinson commented Nov 23, 2020 •
edited

Closing. This doesn't seem like the right way to go, partly because of the backwards compatibility break, partly because wanting to receive all values (None or not) from a background iteration seems like a genuine use-case that we shouldn't break, and partly because using submit_iteration when what you want is an interruptible background call already seems like a strained non-obvious API, and doesn't allow users to easily express the intent.

Instead, I think we want a new submission function submit_interruptible that allows submission of a generator factory (almost always a generator function in practice) representing an interruptible computation, plus whatever other machinery is needed to support that.

@mdickinson mdickinson closed this Nov 23, 2020
@mdickinson mdickinson deleted the chg/dont-send-none-values branch November 23, 2020 11:30
@mdickinson mdickinson mentioned this pull request Mar 31, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rahulporuri rahulporuri rahulporuri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

submit_iteration should not send a message on a plain yield
2 participants
@mdickinson @rahulporuri