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

Package Graph #102

Merged
merged 14 commits into from
Nov 28, 2020
Merged

Package Graph #102

merged 14 commits into from
Nov 28, 2020

Conversation

hbcarlos
Copy link
Collaborator

@hbcarlos hbcarlos commented Nov 9, 2020

A graph to visualize packages #83
The dependencies are not correct we need to get the dependencies of the packages to show them.

I'm reusing the code from jupyterlab-plugin-graph

image

@hbcarlos hbcarlos marked this pull request as draft November 9, 2020 08:35
@jtpio
Copy link
Member

jtpio commented Nov 9, 2020

The dependencies are not correct we need to get the dependencies of the packages to show them.

Maybe there are some parts that can be ported from the previous backend: https://github.com/mamba-org/mamba-navigator/blob/master/server/list_and_query.py

});
if (ns.size >= 50) {
return {
name: 'circle',
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the previous implementation from: https://github.com/mamba-org/mamba-navigator/blob/84a3d5a700b30017112d68b98fdd7e63adc9f182/src/components/Network.vue#L70

It looks like it was using the vis.Network, which is likely to be the one from https://visjs.org:

image

Maybe it's a just a matter of using an equivalent as a cytoscape plugin to have a similar layout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this one looks nicer.
I'll look for a similar layout and modify the backend to get the dependencies.

Maybe something like this one: https://cytoscape.org/cytoscape.js-cose-bilkent/

Copy link
Member

Choose a reason for hiding this comment

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

it could also be good to have a lighter library than cytoscape.

Possible alternative:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks both for the info.
I changed the library and added an argument to the PackageHandler API, so now we can request package dependencies.

Screen-Recording-2020-11-16-at-13 45 22

Would be great to access this graph when clicking on a package in jupyter_conda. Maybe having a new column for dependencies or a link in the package description to open the graph in a dialog.

Let me know what do you think.

Copy link
Member

Choose a reason for hiding this comment

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

Looks great!

Maybe having a new column for dependencies or a link in the package description to open the graph in a dialog.

Yes maybe that's the simplest and most visible way for now?

@hbcarlos
Copy link
Collaborator Author

I improved the graph and add it to the package manager.
For now, we can open the dependencies graph for a package by clicking on his version (we should change that later)

Screen Recording 2020-11-17 at 12 22 00

@hbcarlos hbcarlos marked this pull request as ready for review November 17, 2020 12:08
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @hbcarlos It looks nice.

I left comments (quite a lot sorry).

I have also general question about the rendering. What happen in dark mode?
Could you add arrow to indicate the direction of the dependency?
Could you fade the node not directly dependent on the selected node?

I switch to react-window instead of react-virtualized; the main change is where a cell renderer must be defined. Could you rebase your code please? Let me know if you need help.

{"package": List[dependencies]}
"""
resp = {}
ans = await self._execute(self.manager, "repoquery", "depends", "--json", pkg)
Copy link
Member

Choose a reason for hiding this comment

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

As conda does not support this, the manager should be tested. And in this particular case a default value like {name: null} to signify it can't be done is a good candidate.

if type(dep) is dict :
deps = dep.get('depends', None)
if deps :
resp[dep['name']] = deps
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 returned an empty list rather than skipping the entry.

It seems strange to have:

pkg_depends("a") -> {"a": ["sub_a1", "sub_a2"]}

if a has two dependencies. But

pkg_depends("b") -> {}

If b has no dependency.

query (str): optional string query
"""
pkg = self.get_query_argument("package", "")
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 rather use a query arguments like dependencies equals to 0 or 1 to decide if we need to search or to get the dependencies (with default value to false).
See for example PackagesEnvironmentHandler.post

@@ -50,7 +50,11 @@
"@lumino/coreutils": "^1.3.0",
"@lumino/signaling": "^1.2.0",
"@lumino/widgets": "^1.6.0",
"@types/react-d3-graph": "^2.3.4",
Copy link
Member

Choose a reason for hiding this comment

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

This is a devDependencies not a dependencies

"jupyterlab_toastify": "^4.1.2",
"react": "^16.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"react": "^16.4.1",

Unneeded as set as peerDependencies


let idx: number;
if (cancellable) {
idx = this._cancellableStack.push(cancel) - 1;
Copy link
Member

Choose a reason for hiding this comment

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

Follow-up on cancellation changes:

Suggested change
idx = this._cancellableStack.push(cancel) - 1;
idx = this._cancellableStack.push({type: 'getDependencies', cancel}) - 1;

}
href="#"
onClick={(evt): void => {
this.props.onPkgGraph(rowData);
Copy link
Member

Choose a reason for hiding this comment

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

You need to stop the propagation of the event otherwise it will trigger an update or a remove action on the package.

Suggested change
this.props.onPkgGraph(rowData);
evt.stopPropagation();
this.props.onPkgGraph(rowData);

@@ -143,6 +147,23 @@ export class CondaPkgList extends React.Component<IPkgListProps> {
return <span>{rowData.name}</span>;
};

const versionRender = ({ rowData }: ICellRender): JSX.Element => (
Copy link
Member

Choose a reason for hiding this comment

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

What happen for not installed packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing. I think we should open this graph from another place, not from the version of the package but I'm not sure where to add it.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this mock-up?

The idea is to add an icon in the toolbar above the packages list (it will be absent if mamba is not available - or disabled with a title advertising mamba 😉 ).
On click on the button, this would open the dependency panel (not sure if on the right of the package list is a good place or if it needs to be a separated panel to allow positioning and resize by the user).

Then selecting a package will result in the display of its dependencies graph. If not package selected, a help message such Select a package to display its dependency graph.

This means also to change the current behavior as onClick on a row changes the status of the package. This should then be restricted to the status icon probably.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good for me!
For Mamba, I would add a title in the toolbar when Mamba is not installed saying something like: "Install mamba to have all the functionalities" or maybe and alert icon to have a smaller icon in the toolbar.

if (this._package) this._updatePackages();
}

public showGraph(pkg: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

Unused?


import { Conda } from '../tokens';

export class PkgGraph extends ReactWidget {
Copy link
Member

Choose a reason for hiding this comment

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

Could you refractor this into a thin ReactWidget and a React component?

The React component would have:

  • props
    • pkgManager
    • package
  • state
    • data: {nodes, links} | null

Call _updatePackages in componentDidMount and componentDidUpdate

_loading is unneeded and could be replaced with _data === null

The idea is to have a component that will be easily extendable to display the dependency graph of an environment.

@hbcarlos
Copy link
Collaborator Author

I applied all the changes except for:

Should we handle directly here the fact that mamba will be able to render the graph but not conda?
What happen for not installed packages?

Take a look and let me know if there is something wrong.

@hbcarlos
Copy link
Collaborator Author

I'll look at the test tonight or tomorrow.

@coveralls
Copy link

coveralls commented Nov 19, 2020

Pull Request Test Coverage Report for Build 388750202

  • 5 of 21 (23.81%) changed or added relevant lines in 2 files are covered.
  • 130 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.7%) to 67.983%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mamba_gator/handlers.py 4 5 80.0%
mamba_gator/envmanager.py 1 16 6.25%
Files with Coverage Reduction New Missed Lines %
src/services.ts 130 46.48%
Totals Coverage Status
Change from base Build 364212499: -1.7%
Covered Lines: 745
Relevant Lines: 1002

💛 - Coveralls

@fcollonval
Copy link
Member

fcollonval commented Nov 20, 2020

Binder 👈 Launch a binder notebook on the branch hbcarlos/gator/master

@fcollonval
Copy link
Member

Great work @hbcarlos

image

I have two requests regarding the style:

  • Is it possible to color the node of the selected package differently to highlight it?
  • Could you set the font size for the node to --jp-ui-font-size0? The current 8px is quite small.

@hbcarlos
Copy link
Collaborator Author

Sure, it's done. I'll do a new commit.

Regarding:

The idea is to add an icon in the toolbar above the packages list (it will be absent if mamba is not available - or disabled with a title advertising mamba 😉 ).
On click on the button, this would open the dependency panel (not sure if on the right of the package list is a good place or if it needs to be a separated panel to allow positioning and resize by the user).

Do you want to do it in this PR or open a new one?

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks for the reactivity, I added some final suggestions to finish simplifying the REST API and to add some documentation.

I also change a bit the default configuration because the font size of highlighted elements is not synchronized by default with the standard font size.

If you agree on those suggestions. Then we are good to go and pursuit as you suggested in another PR (I created #110 to keep track).

mamba_gator/handlers.py Outdated Show resolved Hide resolved
mamba_gator/handlers.py Outdated Show resolved Hide resolved
mamba_gator/handlers.py Outdated Show resolved Hide resolved
mamba_gator/rest_api.yml Outdated Show resolved Hide resolved
packages/common/src/services.ts Outdated Show resolved Hide resolved
packages/common/src/tokens.ts Outdated Show resolved Hide resolved
packages/common/src/tokens.ts Outdated Show resolved Hide resolved
packages/common/src/components/PkgGraph.tsx Show resolved Hide resolved
packages/common/src/components/PkgGraph.tsx Show resolved Hide resolved
@hbcarlos
Copy link
Collaborator Author

Thanks, @fcollonval.
I completely agree with those suggestions. You can commit them and merge the PR.

@fcollonval fcollonval self-requested a review November 28, 2020 15:35
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this through @hbcarlos

@fcollonval fcollonval merged commit 4b8378f into mamba-org:master Nov 28, 2020
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.

4 participants