Client now retries .map() failures #2734
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When two items had the same timestamp, we would try to sort by the actual item value, which breaks for types that don't support comparison. Instead use a nonce when inserting an item, to ensure that we never have to compare the item value itself.
Though very unlikely outside of unit tests, it's possible to have an output returned before the corresponding retry context has been put into the `pending_outputs` dict.
Once the input queue filled up, we had no more room to put pending retries. And since we had no more room to put retries, we stopped fetching new outputs. And since we stopped fetching new outputs, the server stopped accepting new inputs. As a result, the input queue would never burn down. Instead, use a semaphore to ensure we never have more than 1000 items outstanding.
Instead of using a priority queue, just use the event loop to schedule retries in the future. This significantly simplifies the implementation and makes it much more like the original. Note that we still do have a semaphore that ensures that no more than 1K inputs are in flight (i.e., sent to the server but not completed).
rculbertson
force-pushed
the
ryan/retry-map
branch
from
d46cc29 to
9c5d968
Compare
rculbertson
changed the title
|
There were some unit tests on the priority queue that could be restored: |
rohansingh
reviewed
Jan 8, 2025
modal/parallel_map.py
Outdated
| if timestamp_seconds == self._MAX_PRIORITY: | ||
| return None | ||
| await self._queue.put((timestamp_seconds, idx)) | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
I think using an asyncio.Condition like the previous implementation is better than asyncio.sleep(1) since the latter becomes a busy wait.
modal-client/modal/_utils/async_utils.py
Lines 806 to 810 in afdc7f8
There was a problem hiding this comment.
ah good point. I'll bring back that original implementation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Closes SVC-180.
Adds client retries to .map(). We already did this to .remote() in this PR: #2403
For now, retries are enabled only if the
MODAL_CLIENT_RETRIESflag is set to true.Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog