Skip to content

[REVIEW] HighLevelGraph validation and dependencies fixes#6588

Merged
mrocklin merged 20 commits intodask:masterfrom
madsbk:hlg-validate
Sep 8, 2020
Merged

[REVIEW] HighLevelGraph validation and dependencies fixes#6588
mrocklin merged 20 commits intodask:masterfrom
madsbk:hlg-validate

Conversation

@madsbk
Copy link
Contributor

@madsbk madsbk commented Sep 2, 2020

Expanding on the work of #6509, this PR implement a complete HighLevelGraph validation by re-calculating the layer dependencies and fixes all incorrect dependencies.

  • Tests added / passed
  • Passes black dask / flake8 dask

@madsbk madsbk changed the title Hlg validate HighLevelGraph validation and dependencies fixes Sep 2, 2020
@madsbk madsbk changed the title HighLevelGraph validation and dependencies fixes [REVIEW] HighLevelGraph validation and dependencies fixes Sep 3, 2020
if hasattr(self, "_dask_layers"):
return self._dask_layers
else:
return (self.key,)
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to always be self.key. I'm curious in which cases this assumption is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This boils down to the discrepancy between the definition of __dask_layers__ that should return output layers and Delayed that might not have any layers.
Delayed.key is always a low level graph key and so this is kind of a hack that make Delayed return a layer if possible.

@mrocklin
Copy link
Member

mrocklin commented Sep 4, 2020

In general this seems like a very useful cleanup. Thank you @madsbk for doing this. I am curious about Delayed.__dask_layers__ but otherwise everything here seems straightforward.

@madsbk madsbk mentioned this pull request Sep 7, 2020
@mrocklin
Copy link
Member

mrocklin commented Sep 8, 2020

I apologize for the delay in reviewing this. This looks good to me. Thank you for this cleanup @madsbk . Merging.

@mrocklin mrocklin merged commit 8bcb7fe into dask:master Sep 8, 2020
kumarprabhu1988 pushed a commit to kumarprabhu1988/dask that referenced this pull request Oct 29, 2020
Follows on from dask#6508

* Added core.keys_in_tasks()
* HLG: added more thorough validation check
* lingalg: layer dependencies are now sets
* linalg.lstsq(): fixed dependencies
* linalg.sfqr(): fixed dependencies
* linalg.tsqr(): removed dependency when all slices are known
* linalg.tsqr(): name_q2bs are name_q2cs are not layers
* added help function compute_layer_dependencies()
* elemwise(): fixed dependencies
* Delayed: fixed __dask_layers__() of HLGs
* _loc_list(): empty dependencies on empty index
* reformat: black
* keys_in_tasks(): fixed doc example
* optimize_blockwise(): fixed dependencies
* tril() and triu(): fixed dependencies when empty result
* added a doctest: +SKIP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants