[Datasets] [Out-of-Band Serialization: 2/3] Refactor ExecutionPlan to maintain complete lineage and eagerly unlink block references.
#23931
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
…k block references.
clarkzinzow
force-pushed
the
datasets/feat/refactor-execution-plan
branch
from
a4d2f71
to
b4a2cbd
Compare
|
Does uncommenting this line in test work after the changes here? |
ericl
reviewed
Apr 15, 2022
| @@ -87,11 +87,13 @@ def test_spread_hint_inherit(ray_start_regular_shared): | |||
| ds = ray.data.range(10)._experimental_lazy() | |||
| ds = ds.map(lambda x: x + 1) | |||
| ds = ds.random_shuffle() | |||
| for s in ds._plan._stages: | |||
| for s in ds._plan._stages_before_snapshot: | |||
There was a problem hiding this comment.
It would be great to have more tests for this unlinking behavior as it's fairly intricate.
ericl
added
the
@author-action-required
jianoaix
reviewed
Apr 18, 2022
…lear everything if pipelining.
clarkzinzow
force-pushed
the
datasets/feat/refactor-execution-plan
branch
from
bb55429
to
e27926d
Compare
… lazy, clear everything if pipelining." This reverts commit d75386c.
… remove conditional copying
clarkzinzow
force-pushed
the
datasets/feat/refactor-execution-plan
branch
from
fa1c84b
to
f13a2da
Compare
jianoaix
reviewed
Apr 22, 2022
| dataset_uuid = self._dataset_uuid | ||
| in_blocks = self._in_blocks | ||
| if isinstance(in_blocks, BlockList): | ||
| in_blocks = in_blocks.copy() |
There was a problem hiding this comment.
Is it actually deep copy? The blocklist copy API used here is a shallow copy: https://sourcegraph.com/github.com/ray-project/ray@78d6ed7/-/blob/python/ray/data/impl/block_list.py?L37
There was a problem hiding this comment.
The execution plan only consumes those BlockList-internal lists (blocks and metadata), it doesn't mutate them, so it's a "deep" copy as we define it in the docstring: "This copy can be executed AND cleared without mutating the original."
jianoaix
approved these changes
Apr 22, 2022
jjyao
approved these changes
Apr 22, 2022
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.
This PR refactors
ExecutionPlanto maintain complete stage lineage, even for eagerly computed datasets, while ensuring that block references are unlinked as early as possible in order to more eagerly release block memory. This PR is the final precursor to adding the actual out-of-band serialization APIs (PR 3/3).The fully lineage has to be maintained, even for eagerly computed datasets, since the lineage is needed for out-of-band serialization of datasets. See the mono-PR for more context.
Checks
scripts/format.shto lint the changes in this PR.