-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Conversation
src/nasui/src/Chart.tsx
Outdated
if (graph === undefined || activation === undefined) | ||
return; | ||
const weights = graph.weightFromMutables(activation); | ||
console.log(weights.size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought could delete this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will delete all console.log
before merge.
src/nasui/src/Chart.tsx
Outdated
if (graphChanged) | ||
this.expandSet = lodash.cloneDeep(graph.defaultExpandSet); | ||
const graphEl = this.graphElements(); | ||
console.log(graphEl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could add no-console: true in your lint
src/nasui/package.json
Outdated
"@bardit/cytoscape-expand-collapse": "^2.0.3", | ||
"@material-ui/core": "^4.9.3", | ||
"@material-ui/icons": "^4.9.1", | ||
"@testing-library/jest-dom": "^4.2.4", | ||
"@testing-library/react": "^9.3.2", | ||
"@testing-library/user-event": "^7.1.2", | ||
"@types/cytoscape": "^3.14.0", | ||
"@types/d3-graphviz": "^2.6.3", | ||
"@types/dagre-d3": "^0.4.39", | ||
"@types/graphlib-dot": "^0.6.1", | ||
"@types/jest": "^24.0.0", | ||
"@types/lodash": "^4.14.149", | ||
"@types/node": "^12.0.0", | ||
"@types/react": "^16.9.0", | ||
"@types/react-dom": "^16.9.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of this dependencies maybe could put into devDependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in future item: Clean up unused dependencies.
src/nasui/src/App.tsx
Outdated
<ListItem className={classes.listSubtitle}>Inputs ({info.inputs.length})</ListItem> | ||
{ | ||
info.inputs.map((item, i) => <ListItem className={classes.listItem} key={`input${i}`}>{item}</ListItem>) | ||
}</React.Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</React.Fragment>换个行吧
const handleSliderChange = (event: ChangeEvent<{}>, value: number | number[]) => { | ||
this.setState({ sliderValue: value as number }); | ||
}; | ||
const handleSettingsDialogToggle = (value: boolean) => () => { | ||
this.setState({ settingsOpen: value }); | ||
}; | ||
const handleSettingsChange = (name: string) => (event: React.ChangeEvent<HTMLInputElement>) => { | ||
this.setState({ | ||
...this.state, | ||
[name]: event.target.checked | ||
}, () => { | ||
this.setState({ | ||
graph: new Graph(this.state.graphData, this.state.hideSidechainNodes), | ||
}) | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
下个版本考虑把这些方法挪出render() 这个周期吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. Will do.
'text-wrap': 'wrap', | ||
'text-valign': 'center', | ||
'text-halign': 'center', | ||
'font-family': '"Roboto", "Helvetica", "Arial", sans-serif', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果跟WebUI风格保持一致的话,也应该选Segoe UI作为第一字体?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aligning with tensorboard. So I used Material-UI + Roboto.
'padding-left': '8px', | ||
'padding-right': '8px', | ||
'padding-top': '8px', | ||
'padding-bottom': '8px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
padding: 8px;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
'padding-top': '30px', | ||
'padding-bottom': '10px', | ||
'padding-right': '10px', | ||
'padding-left': '10px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
也可以合成一条
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Cytoscape use its own style format. I tried to merge them and it didn't work.
const { classes } = this.props; | ||
const { selectedNode, graph } = this.state; | ||
if (graph === undefined) | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference between null
and undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React suggests null
when returning a None element. Fixed. Thanks.
src/nasui/src/graphUtils.ts
Outdated
} | ||
this.defaultExpandSet = this.getDefaultExpandSet(graphData.mutable); | ||
this.mutableEdges = this.inferMutableEdges(graphData.mutable); | ||
console.log(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console.log()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1 @@ | |||
/// <reference types="react-scripts" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ultmaster better to explain why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explained in facebook/create-react-app#6560. I guess we should respect the practice of create-react-app.
}; | ||
}) | ||
.catch(error => { | ||
console.error('Error during service worker registration:', error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Alert(error)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is auto-generated. I don't want to modify it.
src/nasui/src/setupTests.ts
Outdated
@@ -0,0 +1,5 @@ | |||
// jest-dom adds custom jest matchers for asserting on DOM nodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this useful? It seems that there is no main logic code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's auto-generated. Removing it for now.
tools/nni_cmd/nnictl.py
Outdated
@@ -158,6 +158,10 @@ def parse_args(): | |||
parser_webui_url = parser_webui_subparsers.add_parser('url', help='show the url of web ui') | |||
parser_webui_url.add_argument('id', nargs='?', help='the id of experiment') | |||
parser_webui_url.set_defaults(func=webui_url) | |||
parser_webui_nas = parser_webui_subparsers.add_parser('nas', help='show nas ui') | |||
parser_webui_nas.add_argument('--port', default=6060, type=int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add help=
to explain the argement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
for mutable in self.mutables.traverse(deduplicate=False): | ||
modules = mutable.name.split(".") | ||
path = [ | ||
{"type": self.model.__class__.__name__, "name": ""} | ||
] | ||
m = self.model | ||
for module in modules: | ||
m = getattr(m, module) | ||
path.append({ | ||
"type": m.__class__.__name__, | ||
"name": module | ||
}) | ||
result["mutable"][mutable.key].append(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to add comments to explain this part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
tools/nni_cmd/nnictl.py
Outdated
@@ -203,7 +207,7 @@ def parse_args(): | |||
parser_tensorboard_start = parser_tensorboard_subparsers.add_parser('start', help='start tensorboard') | |||
parser_tensorboard_start.add_argument('id', nargs='?', help='the id of experiment') | |||
parser_tensorboard_start.add_argument('--trial_id', '-T', dest='trial_id', help='the id of trial') | |||
parser_tensorboard_start.add_argument('--port', dest='port', default=6006, help='the port to start tensorboard') | |||
parser_tensorboard_start.add_argument('--port', dest='port', default=6060, help='the port to start tensorboard') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an accident. Sorry.
@ultmaster could you write doc for it, including how to change user code, how to start the nas ui. From the code, I can get how you get the super graph, but I did not get how the log files come from, how to generate those log files for nas ui. |
I introduced briefly how to use it in the description of this PR, but it's for test only. The docs will be in next release. This is because currently, the only way to launch it is to build from source and launch it with nnictl under nni directly, which is because, release integration is not done. BTW, as discussed in the last meeting, I need help to integrate it into release (including changing all the Makefiles and make it work on pypi version). Ideally, graph logging should be registered as hook (callback) into trainer. But since |
This PR is open for code comparison against master. Not ready for review yet.Working items:
Might do if I have time:
I will post some preview here when I think it's ready.
Ready for review
Deferred items:
These items might be done in separate PRs.
How to test:
Change DARTS search into something like this:
This will write the graph to
graph.json
before starts, and write each step as a line into a file calledlog
.Launch
nnictl webui nas --logdir /path/to/the/directory/containing/two/files
. Find your UI at port66676060.