Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename nbytes widgets #4878

Merged
merged 8 commits into from
Jul 1, 2021
Merged

Rename nbytes widgets #4878

merged 8 commits into from
Jul 1, 2021

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Jun 3, 2021

Rename widgets:

  • NBytes -> WorkersMemory
  • NBytesHistogram -> WorkersMemoryHistogram
  • NBytesCluster -> ClusterMemory

Rename URIs:

  • /individual-nbytes -> /individual-workers-memory
  • /individual-nbytes-cluster -> /individual-cluster-memory

Closes #4871

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Big +1 here, this is a good change.

I expect this will break the Jupyter Lab Extension downstream. There is a default list of individual panels which is used if the panel discovery fails.

cc @ian-r-rose

@mrocklin
Copy link
Member

mrocklin commented Jun 3, 2021

In particular I think that changing the button in JLab is what I'm most after here. It always take me a while to find it because I'm always looking for "Worker Memory"

@jacobtomlinson
Copy link
Member

Yeah I totally agree @mrocklin. But I guess we need a related PR to the lab extension to change the default URLs.

@jrbourbeau
Copy link
Member

Gentle ping for @ian-r-rose

@crusaderky
Copy link
Collaborator Author

ping @ian-r-rose

@jrbourbeau
Copy link
Member

xref dask/dask-labextension#198

@ian-r-rose
Copy link
Collaborator

Sorry to miss the earlier ping. It's a simple fix to update the names of the default charts, and I'd be happy to do that. But we might consider removing the defaults all-together. They used to be more useful, but today they are mostly used to show the greyed out buttons when no dashboard is connected. When the dashboard is live, it should be able to pick up whatever is provided by the individual-plots.json endpoint.

@crusaderky
Copy link
Collaborator Author

I've reintroduced the old API endpoints; this should hopefully mean that that this PR can be merged without needing to wait for the matching change in dask-labextension?

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Awesome thanks @crusaderky, good idea.

I've made a comment on the dask-labextension issue to come back here and remove the backward compatibility again.

@crusaderky
Copy link
Collaborator Author

@mrocklin @jrbourbeau ready for merge

@mrocklin mrocklin merged commit b8b4ff8 into dask:main Jul 1, 2021
@crusaderky crusaderky deleted the nbytes branch July 1, 2021 15:42
@ian-r-rose
Copy link
Collaborator

I've reintroduced the old API endpoints; this should hopefully mean that that this PR can be merged without needing to wait for the matching change in dask-labextension?

Sorry, I suppose I was unclear in my comment above and the linked issue. What I meant was: the labextension will already pick up name the name changes that you have made. By reintroducing the old name, it now shows up twice in the button listing:
image

The only time that we'll see the old names is when there is not a cluster connected to the UI (which will be resolved with dask/dask-labextension#198).

I think we'll want to remove the duplicated individual plots from e7305ea

@ian-r-rose ian-r-rose mentioned this pull request Jul 1, 2021
3 tasks
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.

Rename "nbytes" chart to "worker memory"
5 participants