Merged
Conversation
Ended up writing this test, seemed like a good thing to test even though it's currently broken because of dask#8173.
TODO: tests
TODO: tests
crusaderky
requested changes
Dec 10, 2021
| # NOTE: Layer only used by `Item.from_delayed`, to handle Delayed objects created by other collections. | ||
| # e.g.: Item.from_delayed(da.ones(1).to_delayed()[0]) | ||
| # See Delayed.__init__ | ||
| self._layer = layer or key |
Collaborator
There was a problem hiding this comment.
Suggested change
| self._layer = layer or key | |
| if layer: | |
| self._layer = layer | |
| elif isinstance(key, tuple): | |
| self._layer = key[0] | |
| else: | |
| self._layer = key |
| dsk = self.__dask_optimize__(dsk, keys) | ||
| return [Delayed(k, dsk) for k in keys] | ||
| layer = None | ||
| return [Delayed(k, dsk, layer=layer) for k in keys] |
Collaborator
There was a problem hiding this comment.
Please make this coherent with dask.array above
Collaborator
Author
There was a problem hiding this comment.
I actually feel like setting the layer at all here is a bit unnecessary, since the graphs are always low-level anyway, but sure.
| self._length = length | ||
|
|
||
| # NOTE: Layer is used by `to_delayed` in other collections, but not in normal Delayed use | ||
| self._layer = layer or key |
Collaborator
There was a problem hiding this comment.
Suggested change
| self._layer = layer or key | |
| if layer: | |
| self._layer = layer | |
| elif isinstance(key, tuple): | |
| self._layer = key[0] | |
| else: | |
| self._layer = key |
Collaborator
Author
There was a problem hiding this comment.
I intentionally didn't do this because I'd rather be explicit about passing the layer name when it's needed, and not make any assumptions about key structure in Delayed. The self._layer not in dsk.layers check below should catch any misuse. Since __init__ should only be used internally, not directly by users, this seemed reasonable to me.
Co-authored-by: crusaderky <crusaderky@gmail.com>
crusaderky
approved these changes
Dec 13, 2021
jrbourbeau
reviewed
Dec 13, 2021
Member
jrbourbeau
left a comment
There was a problem hiding this comment.
The test_daily_stock_deprecated failure is unrelated to the changes here. xref #8477 which will remove that test altogether.
gjoseph92
added a commit
to gjoseph92/dask
that referenced
this pull request
Dec 13, 2021
In dask#8452 I realized that an incorrect pattern had emerged from dask#6510 of including ```python if not isinstance(dsk, HighLevelGraph): dsk = HighLevelGraph.from_collections(id(dsk), dsk, dependencies=()) ``` in optimization functions. Specifically, `id(dsk)` is incorrect as the layer name here. The layer name must match the `.name` of the resulting collection that gets created by `__dask_postpersist__()`, otherwise `__dask_layers__()` on the optimized collection will be wrong. Since `optimize` doesn't know about collections and isn't passed a layer name, the only reasonable thing to do here is to error when given a low-level graph. This is safe to do for Arrays and DataFrames, since their constructors convert any low-level graphs to HLGs. This PR doesn't really fix anything—the code path removed should be unused—but it eliminates a confusing pattern that has already wandered its way into other places dask#8316 (comment).
2 tasks
Collaborator
Author
aeisenbarth
pushed a commit
to aeisenbarth/dask
that referenced
this pull request
Jan 6, 2022
As discussed in dask#8173, it's necessary for `Delayed` and `bag.Item` objects to be able to keep track of the HLG layer name they're referencing. When constructed with `to_delayed(optimize_graph=False)`, this layer name will not be the same as the key (`array-abcde` vs `('array-abcde', 0)`). Co-authored-by: crusaderky <crusaderky@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
As discussed in #8173, it's necessary for
Delayedandbag.Itemobjects to be able to keep track of the HLG layer name they're referencing. When constructed withto_delayed(optimize_graph=False), this layer name will not be the same as the key (array-abcdevs('array-abcde', 0)).to_delayed(optimize_graph=False)objects often have incorrect__dask_layers__#8173pre-commit run --all-files