Skip to content

Add feature to show names of layer dependencies in HLG JupyterLab repr#9081

Merged
jsignell merged 9 commits intodask:mainfrom
aomirolis:main
Jun 9, 2022
Merged

Add feature to show names of layer dependencies in HLG JupyterLab repr#9081
jsignell merged 9 commits intodask:mainfrom
aomirolis:main

Conversation

@aomirolis
Copy link
Copy Markdown
Contributor

This PR implements the enhancement proposed by @gjoseph92 on #8567.

I opened a new PR (closed previous #9061) with the corrections suggested by @jsignell and @gjoseph92, thank you a lot for your quick response and feedback!

@GPUtester
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@pavithraes
Copy link
Copy Markdown
Member

@aomirolis Thank you for opening this! For future reference, you can push update commits to the branch you use for the PR, it helps consolidate all discussion and feedback in the same place. :)

Some CI tests are failing with:

TypeError: Layer._repr_html_() missing 1 required positional argument: 'dependencies'

Since we added a new dependencies argument in this PR, could you please update all the tests where _repr_html_() is used?

aomirolis and others added 2 commits May 23, 2022 19:42
@aomirolis
Copy link
Copy Markdown
Contributor Author

Thank you! I'll keep that in mind.

@pavithraes
Copy link
Copy Markdown
Member

@aomirolis Thanks for the updates!

@gjoseph92 Could you please take a look at this when you get a chance?

Copy link
Copy Markdown
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This looks nice and clean but I don't actually see any dependency information in the output. Is it possible that that information isn't actually available until the graph is materialized @gjoseph92?

return obj

def _repr_html_(self, layer_index="", highlevelgraph_key=""):
def _repr_html_(self, dependencies, layer_index="", highlevelgraph_key=""):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def _repr_html_(self, dependencies, layer_index="", highlevelgraph_key=""):
def _repr_html_(self, layer_index="", highlevelgraph_key="", dependencies=()):

This should be an optional argument. Then you can revert your changes to all those tests as well.

I would have expected this to break rendering a Layer instance (as opposed to an HLG instance) in an actual notebook?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! May I ask why it should be this way? I will soon make the suggested changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By adding dependencies as an optional kwarg at the end of the method, this PR is fully backwards compatible. So any existing code will continue to work even if dependencies is not passed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thank you!

@gjoseph92
Copy link
Copy Markdown
Collaborator

@jsignell are you trying this out?

Is it possible that that information isn't actually available until the graph is materialized

That's definitely not the case, that's just part of the HLG structure. If it't not showing up, then I think something else is wrong.

@jsignell
Copy link
Copy Markdown
Member

jsignell commented Jun 6, 2022

Yeah I was trying it out with:

import dask.array as da

arr1 = da.zeros(shape=(100,100), chunks=(5,10))
arr2 = arr1.T + 100
arr3 = arr1.dot(arr2)
arr3.dask

@aomirolis
Copy link
Copy Markdown
Contributor Author

image
This code produces this output for me. You can see the dependencies of layer 4 at the last two lines. Am I missing something?

@jsignell
Copy link
Copy Markdown
Member

jsignell commented Jun 7, 2022

So sorry for the noise @aomirolis my git branch checkout failed and I didn't notice. I see the intended behavior now just as in your screenshot.

@aomirolis
Copy link
Copy Markdown
Contributor Author

No problem!!

Copy link
Copy Markdown
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @aomirolis for sticking with it!

@jsignell jsignell merged commit ddb65cf into dask:main Jun 9, 2022
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.

5 participants