fancy version of the configuration wizard #1344
Conversation
joelgrus
commented
Jun 5, 2018
joelgrus
commented
- sort of works
- is really ugly
- handles subconfigurations correctly
- does not handle List / Dict / Tuple types correctly
- "generate config" button doesn't do anything yet
|
What's the right command to try this? Last time I did it I had to resort to a bit of hackery; I assume you have a command that just works from this branch. |
|
just do and then take your browser to |
|
When I run that I get |
|
probably, I usually |
|
Very nice! This is going to be super cool when it's done. |
next steps:
|
|
joelgrus
changed the title
|
ok, this is probably as good as I'm going to get it for now. there are a few edge cases (union types + tuples) that it probably doesn't handle right, but they come up rarely. I tried to make it moderately attractive, but I don't know if I succeeded on that. I'll demo it at the team meeting today. |
|
btw this fixes #1125 |
|
This is awesome! Very nice work! It looks way better than what I would have done; thanks for doing this. My only suggestion would be to give a type hint for dicts and lists, so you don't have to guess what the "+" button does. |
| One option is to use the new `allennlp configure` command, which | ||
| produces stubs that can be used to produce configuration files. | ||
|
|
||
| If you run it without any options, you get an output corresponding to the top-level configuration: |
There was a problem hiding this comment.
You should probably note here that it's for the train command.
| assert '"type": "rmsprop"' in html | ||
| assert '// "weight_decay"' in html | ||
| assert config["type"] == "rmsprop" | ||
| assert any(item["name"] == "lr" for item in items) | ||
|
|
||
| def test_rnn_hack(self): |
There was a problem hiding this comment.
This could be a little more informative - are we testing that we get the torch class docstrings, instead of our wrapper?
There was a problem hiding this comment.
no, it's something different, I added a comment
| @@ -143,8 +219,8 @@ def _auto_config(cla55: Type[T]) -> Config[T]: | |||
| """ | |||
| typ3 = _get_config_type(cla55) | |||
|
|
|||
| # Don't include self | |||
| names_to_ignore = {"self"} | |||
| # Don't include self, or vocab | |||
There was a problem hiding this comment.
The vocab can still be configured at the top level, right? this is just to weed out all the other occurrences.
There was a problem hiding this comment.
exactly, this is inside auto_config, which uses reflection to figure out the config for some object. the top level config is arbitrary (it's just whatever commands/train expects), so it doesn't use this method.
|
|
||
| if self.typ3: | ||
| item_dict["type"] = self.typ3 | ||
| #items.insert(0, {"name": "type", "type": self.typ3}) |
There was a problem hiding this comment.
Delete this commented out line.
| <body> | ||
| $body | ||
| </body> | ||
| <head> |
There was a problem hiding this comment.
I think you can avoid this duplication by adding the html file to MANIFEST.in in the root of the repo - this dictates non python stuff which is included/excluded by pip.
There was a problem hiding this comment.
I'm going to leave this for a future PR, I looked into it a bit and it seems like it's tricky to get right
| <script src="https://unpkg.com/tippy.js@2.5.2/dist/tippy.all.min.js"></script> | ||
| <script type="text/babel"> | ||
| /* | ||
| ConfigItem = { |
There was a problem hiding this comment.
Looks like you've replaced these below, right?
There was a problem hiding this comment.
sorry, that's not javascript code, that's a comment about the types that come from the API, I'll add a comment to that effect
| } | ||
| */ | ||
|
|
||
| // A configItem is optional if it has no default value |
There was a problem hiding this comment.
If it does have a default value?
|
|
||
| componentDidMount() { | ||
| // Fetch the top level configuration | ||
| fetch('/api/') |
There was a problem hiding this comment.
Is it possible that this API is going to be extended to include partial json configurations/recommendations etc? It might be better to call this api /annotate or something, if that is the case.
There was a problem hiding this comment.
I'll make it /api/config/ to leave room for other /api/ routes
| return <span> | ||
| {configureButton} | ||
| <div class="choices-dropdown"> | ||
| {choicesDropdown}{deleteButton}{renderedValue} |
There was a problem hiding this comment.
Nit: render the deleteButton first so that it's always in the same place - currently, for List items, it is before the entry field, but here it's after.
There was a problem hiding this comment.
I don't like this way, but if I make that change then a Dict[str, Configurable] element becomes a huge mess
| const renderedValue = config ? <Config path={path.push('value')} config={config} setData={setData}/> : null | ||
|
|
||
| return <span> | ||
| {configureButton} |
There was a problem hiding this comment.
I think if you render the configureButton in it's own span the spacing between things will look nicer:
There was a problem hiding this comment.
That is bigger visually than I intended
| return ( | ||
| <div class="wizard"> | ||
| <Config path={Immutable.List(['value'])} setData={this.setData} config={config}/> | ||
| <JsonBox config={config}/> |
There was a problem hiding this comment.
Idea: it would be nice if the rendered JSON contained empty dictionaries for required fields - e.g you might want to fill in a partial config and then fill in the rest later, or something.
* initial version * progress * more features * progress * clean up * add docstring * provide type annotations for Dicts and Lists * rewrite * rewrite the whole thing * fix bugs * fix tests + final touches * add tutorial * fix broken link * fix typo
