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
23 changes: 23 additions & 0 deletions jupyter_conda/envmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,29 @@ async def env_packages(self, env: str) -> Dict[str, List[str]]:

return {"packages": [normalize_pkg_info(package) for package in data]}

async def pkg_depends(self, pkg: str) -> Dict[str, List[str]]:
"""List environment packages dependencies.

Args:
pkg (str): Package name

Returns:
{"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.

_, output = ans
query = self._clean_conda_json(output)

if "error" not in query:
for dep in query['result']['pkgs'] :
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.


return resp

async def list_available(self) -> Dict[str, List[Dict[str, str]]]:
"""List all available packages

Expand Down
9 changes: 7 additions & 2 deletions jupyter_conda/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,12 +349,17 @@ async def get(self):
"""`GET /packages` Search for packages.

Query arguments:
package (str): optional package name to seach dependencies
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

query = self.get_query_argument("query", "")

idx = None
if query: # Specific search
if pkg :
idx = self._stack.put(self.env_manager.pkg_depends, pkg)

elif query: # Specific search
idx = self._stack.put(self.env_manager.package_search, query)

else: # List all available
Expand Down
5 changes: 5 additions & 0 deletions jupyter_conda/rest_api.yml
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,11 @@ paths:
produces:
- "application/json"
parameters:
- name: "package"
in: "query"
description: "Package name to search for dependencies"
type: "string"
default: ""
- name: "query"
in: "query"
description: "Query string to pass to conda search"
Expand Down
4 changes: 4 additions & 0 deletions packages/common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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

"d3": "^5.5.0",
"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

"react-d3-graph": "^2.5.0",
"react-virtualized": "^9.21.1",
"semver": "^6.0.0||^7.0.0",
"typestyle": "^2.0.0"
Expand Down
33 changes: 22 additions & 11 deletions packages/common/src/components/CondaPkgList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ export interface IPkgListProps {
* Package item version selection handler
*/
onPkgChange: (pkg: Conda.IPackage, version: string) => void;
/**
* Package item graph dependencies handler
*/
onPkgGraph: (pkg: Conda.IPackage) => void;
}

/**
Expand Down Expand Up @@ -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.

<a
Copy link
Member

Choose a reason for hiding this comment

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

Should we handle directly here the fact that mamba will be able to render the graph but not conda?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you want to do it? I'm not following you, sorry.

className={
rowData.updatable
? classes(Style.Updatable, Style.Link)
: Style.Link
}
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);

}}
rel="noopener noreferrer"
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
rel="noopener noreferrer"
rel="noopener noreferrer"
title="Show dependency graph"

It would be nice to add a title to inform the user of the action resulting of the click.

>
{rowData.version_installed}
</a>
);

const changeRender = ({ rowData }: ICellRender): JSX.Element => (
<div className={'lm-Widget'}>
<HTMLSelect
Expand Down Expand Up @@ -235,17 +256,7 @@ export class CondaPkgList extends React.Component<IPkgListProps> {
/>
)}
<Column
cellRenderer={({ rowData }: ICellRender): JSX.Element => (
<span
className={
rowData.updatable
? classes(Style.Updatable, Style.Cell)
: Style.Cell
}
>
{rowData.version_installed}
</span>
)}
cellRenderer={versionRender}
dataKey="version_installed"
disableSort
label="Version"
Expand Down
12 changes: 11 additions & 1 deletion packages/common/src/components/CondaPkgPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { showDialog } from '@jupyterlab/apputils';
import { showDialog, Dialog } from '@jupyterlab/apputils';
import { INotification } from 'jupyterlab_toastify';
import * as React from 'react';
import semver from 'semver';
Expand All @@ -10,6 +10,7 @@ import {
PACKAGE_TOOLBAR_HEIGHT,
PkgFilters
} from './CondaPkgToolBar';
import { PkgGraph } from './PkgGraph';

// Minimal panel width to show package description
const PANEL_SMALL_WIDTH = 500;
Expand Down Expand Up @@ -239,6 +240,14 @@ export class CondaPkgPanel extends React.Component<
});
}

handleDependenciesGraph = (pkg: Conda.IPackage): void => {
showDialog({
title: pkg.name,
body: new PkgGraph(this._model, pkg.name),
buttons: [Dialog.okButton()],
})
}

handleSearch(event: any): void {
if (this.state.isApplyingChanges) {
return;
Expand Down Expand Up @@ -505,6 +514,7 @@ export class CondaPkgPanel extends React.Component<
packages={searchPkgs}
onPkgClick={this.handleClick}
onPkgChange={this.handleVersionSelection}
onPkgGraph={this.handleDependenciesGraph}
/>
</div>
);
Expand Down
87 changes: 87 additions & 0 deletions packages/common/src/components/PkgGraph.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import {
ReactWidget
} from '@jupyterlab/apputils';

import { Graph, GraphData, GraphNode, GraphLink } from "react-d3-graph";

import * as React from 'react';

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.

constructor(pkgManager: Conda.IPackageManager, pkg: string) {
super();
this._package = pkg;
this._pkgManager = pkgManager;
this._loading = true;

// create a network
this._data = { nodes: [], links: [] };
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?

this._package = pkg;
this._updatePackages();
}

private async _updatePackages(): Promise<void> {
this._loading = true;
this.update();

this._pkgManager.getDependencies(this._package, true)
Copy link
Member

Choose a reason for hiding this comment

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

Add catch to at least display error in the web console. But better is also to provide a human readable error message.

.then( available => {
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
this._pkgManager.getDependencies(this._package, true)
.then( available => {
const available = await this._pkgManager.getDependencies(this._package, true);

The code uses as much as possible async/await syntax rather than then. Please try to use that syntax too.

this._data.nodes = [];
this._data.links = [];

Object.keys(available).forEach( key => {
this._data.nodes.push({ id: key });
available[key].forEach( dep => {
const dependencie = dep.split(' ')[0];
if (!this._data.nodes.find(value => value.id === dependencie)) {
this._data.nodes.push({ id: dependencie });
}
this._data.links.push({ source: key, target: dependencie });
});
});

this._loading = false;
this.update();
})
}

render(): JSX.Element {
const config = {
nodeHighlightBehavior: true,
node: {
color: "lightblue",
highlightStrokeColor: "blue",
},
link: { highlightColor: "blue" },
};
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 extract this at the class level or even better as default value but tunable through an constructor argument?

Do we need JLab theme here?

return (
<div>
{
this._loading
?
<span>Loading dependencies</span>
:
<div>
{
this._data.nodes.length !== 0
?
<Graph id="graph-id" data={this._data} config={config}/>
:
<span>This is a pip package</span>
}
</div>
}
</div>
);
}

private _pkgManager: Conda.IPackageManager;
private _data: GraphData<GraphNode, GraphLink>;
private _package: string;
private _loading: boolean;
}
28 changes: 28 additions & 0 deletions packages/common/src/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,34 @@ export class CondaPackage implements Conda.IPackageManager {
}
}

async getDependencies(
pkg: string,
cancellable = true
): Promise<Conda.IPackageDeps> {
this._cancelTasks();
Copy link
Member

Choose a reason for hiding this comment

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

You are introducing a second cancellable action. So it would be good to change a bit the API to store in the cancellable stack objects {type, cancel}. So _cancelTasks can be called on a particular type of tasks.

Meaning here:

Suggested change
this._cancelTasks();
this._cancelTasks('getDependencies');

const request: RequestInit = {
method: 'GET'
};

const { promise, cancel } = Private.requestServer(
URLExt.join('conda', 'packages') +
URLExt.objectToQueryString({ package: pkg }),
request
);

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;

}

const response = await promise;
if (idx) {
this._cancellableStack.splice(idx, 1);
}
const data = (await response.json()) as Conda.IPackageDeps;
return Promise.resolve(data);
}

async refreshAvailablePackages(cancellable = true): Promise<void> {
await this._getAvailablePackages(true, cancellable);
}
Expand Down
13 changes: 13 additions & 0 deletions packages/common/src/tokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,16 @@ export namespace Conda {
* @param environment Environment name
*/
remove(packages: Array<string>, environment?: string): Promise<void>;
/**
* Get packages dependencies list.
*
* @param environment Environment name
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
* @param package Package name
* @param cancellable Can this asynchronous action be cancelled?
*
* @returns The package list
*/
getDependencies(pkg: string, cancellable: boolean): Promise<{[key: string]: [string]}>
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
getDependencies(pkg: string, cancellable: boolean): Promise<{[key: string]: [string]}>
getDependencies(pkg: string, cancellable: boolean): Promise<IPackageDeps>

/**
* Signal emitted when some package actions are executed.
*/
Expand Down Expand Up @@ -233,6 +243,9 @@ export namespace Conda {
version_selected?: string;
updatable?: boolean;
}
export interface IPackageDeps {
[key: string]: [string]
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
[key: string]: [string]
[key: string]: string[]

}

export interface IPackageChange {
/**
Expand Down
Loading