Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

config explorer webapp #1329

Merged
merged 42 commits into from
Jun 4, 2018
Merged

Conversation

joelgrus
Copy link
Contributor

@joelgrus joelgrus commented Jun 1, 2018

there's some ugly hacks in here, but so be it

@joelgrus joelgrus requested a review from schmmd June 1, 2018 20:53
@joelgrus
Copy link
Contributor Author

joelgrus commented Jun 1, 2018

here's what it looks like

top level

choices for a registrable

configuring a subclass

Copy link
Member

@schmmd schmmd left a comment

Choose a reason for hiding this comment

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

Looks great!


def full_name(cla55: type) -> str:
def full_name(cla55: Optional[type]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

OK, I feel obligated to point out cla55 after our in-person conversation. Clearly it should be clazz! Just teasing.

return f"""{_remove_prefix(str(origin))}[{", ".join(full_name(arg) for arg in args)}]"""
elif origin == Union:
# Special special case to handle optional types:
if len(args) == 2 and args[-1] == type(None):
Copy link
Member

Choose a reason for hiding this comment

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

Yikes! This is how options are implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically, options aren't "implemented", they don't exist other than as type annotations.

so while it looks ugly, it's precisely what an Optional[T] type hint means.

try:
config = configure(class_name)
except:
# TODO(joelgrus): better error handling
Copy link
Member

Choose a reason for hiding this comment

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

Just pointing out the TODO. I assume you meant to leave it but wanted to make sure.

@matt-gardner
Copy link
Contributor

This is super cool! One idea for a next iteration / even more useful interface: instead of having links to new pages, have a single page that contains the full JSON. When you click on something configurable, the list of options shows up in a modal dialog. When you pick something from that list, instead of going to a new page, the original link is replaced by the nested configuration, and you continue. Then, when you're done clicking on all of the links, you have a fully-formed configuration file that you can use. Does this make sense?

@matt-gardner
Copy link
Contributor

Also, I was clicking through things and found the initializers and regularizers aren't treated correctly yet.

@schmmd
Copy link
Member

schmmd commented Jun 1, 2018

@matt-gardner I have something similar in my mind. My hope is that we can re-use an off-the-shelf JSON autocompleter rather than extending a web application of our own.

@matt-gardner
Copy link
Contributor

@schmmd, I'd be surprised if there were a library that would just work for this. Maybe we can find one, but I'm skeptical. It doesn't seem that hard, though, as all of the pieces are already here in what @joelgrus has. You just capture the clicks and instead of following links, you put the response in a modal dialog, or replace the text in the clicked element. You just need a bit of instrumentation on the displayed JSON (doesn't seem likely that there's a library that already does this), and some handling for removing the // in certain places (there definitely won't be a library that already does this, but it's not hard).

@schmmd
Copy link
Member

schmmd commented Jun 1, 2018

We'd likely need to exact flow we envision to the software we find--if we found something.

@joelgrus
Copy link
Contributor Author

joelgrus commented Jun 1, 2018

ok, with some pretty ugly (but unavoidable) hacks, I got initializers and regularizers working.

I had to make one small tweak: our initializers are anonymous classes that wrap a curried function, so there was no easy to way to check if you had one. I added a class variable _initializer_wrapper = True to each one, it seemed like the easiest way to identify them.

@joelgrus joelgrus merged commit 0c2d5c7 into allenai:master Jun 4, 2018
gabrielStanovsky pushed a commit to gabrielStanovsky/allennlp that referenced this pull request Sep 7, 2018
* config explorer

* wip

* wip

* progress

* work

* config tool

* configuration

* configuration command line tool

* add test for configure command

* fix test collisions

* fix docs

* scope suppress FutureWarning to only h5py imports

* fix import ordering

* fix docs

* fix docs (again)

* add web based config explorer

* add test for include-packages

* rename new package to avoid collision with other tests

* fix config explorer on initializers and regularizers (via some nasty hacks)

* fix test for _init_function now being a class method

* fix some tests

* revert some initializer changes

* reorder

* fix pylint issues

* fix mypy
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants