8000 release config explorer by joelgrus · Pull Request #2118 · allenai/allennlp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

release config explorer #2118

Merged
merged 11 commits into from
Dec 3, 2018
Merged

release config explorer #2118

merged 11 commits into from
Dec 3, 2018

Conversation

joelgrus
Copy link
Contributor
  • make it so allennlp configure launches the config explorer
  • a couple of minor cosmetic changes
  • lots of bug fixes for edge cases:
    ** Vocabulary has a weird from_params and needs bespoke treatment
    ** not all FromParams classes are subclasses of Registrable classes (e.g. FeedForward), so I had to change a bunch of logic to deal with that
    ** some Registrable base classes are themselves instantiable (Vocabulary, Trainer), so I had to change a bunch of logic to deal with that too
    ** make the API changeActivation annotations to str annotations, since when we take them from_params that's how we take them anyway
    ** probably some others

the main remaining non-supported case is union types, which I'm still not sure how to handle

schmmd
schmmd reviewed Dec 1, 2018
View reviewed changes
allennlp/service/config_explorer.html
<div class="known-issues">
<h3>Known Issues</h3>
<ul>
<li>For the most part this doesn't handle union types correctly.
Copy link
Member

Choose a reason for hiding this comment

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

maybe put the

  • on one line? GitHub hates this ;-)

    •

    @schmmd
    Copy link
    Member
    schmmd commented Dec 1, 2018

    OK--so there's a lot going on in here (and much of it's in javascript) so it's hard to review such a large PR--but the code looks good on the whole and the wizard looks great.

    I'm bothered by the fact that the HTML is duplicated across python and a .html file. @joelgrus had trouble distributing the html file with pip. @nelson-liu did you figure this out when you moved the test cases to allennlp itself?

    schmmd
    schmmd approved these changes Dec 1, 2018
    View reviewed changes
    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.

    I'm approving the PR--since it doesn't introduce the duplicated HTML (even though it's pretty bad).

    @nelson-liu
    Copy link
    Contributor

    @schmmd seems like the .html file is being included.

    I ran:

    conda create -n test_config_explorer python=3.6
source activate test_config_explorer
pip install git+https://github.com/joelgrus/allennlp.git@release-config-explorer
python -c "import allennlp; print(allennlp.__file__)"

    Then, navigate to the install directory, in my case /Users/nfliu/miniconda3/envs/test_config_explorer/lib/python3.6/site-packages/allennlp/...

    Lastly:

    $ ls service/
__init__.py           config_explorer.html  predictors/
__pycache__/          config_explorer.py    server_simple.py

    @schmmd
    Copy link
    Member
    schmmd commented Dec 1, 2018

    @nelson-liu you make everything look so easy ;-P

    @joelgrus
    Copy link
    Contributor Author
    joelgrus commented Dec 1, 2018

    this xcode issue makes it so I can't even pip install allennlp on my macbook 😢

    @joelgrus
    Copy link
    Contributor Author
    joelgrus commented Dec 3, 2018

    ok, I got rid of the duplicated HTML and it seems to still work

    @joelgrus
    Copy link
    Contributor Author
    joelgrus commented Dec 3, 2018

    I'd feel better if someone else tests that they can run the config explorer from this version before I merge it

    @schmmd
    Copy link
    Member
    schmmd commented Dec 3, 2018

    Will do.

    @schmmd
    Copy link
    Member
    schmmd commented Dec 3, 2018

    Works for me.

    conda create -n allennlp-joelgrus python=3.6 
conda activate allennlp-joelgrus
pip install git+https://github.com/joelgrus/allennlp.git@release-config-explorer
allennlp configure

    @joelgrus joelgrus merged commit ca7e3cf into allenai:master Dec 3, 2018
    @schmmd schmmd mentioned this pull request Mar 14, 2019
    Closed
    Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
    Reviewers

    @schmmd schmmd schmmd approved these changes

    Assignees
    No one assigned
    Labels
    None yet
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @joelgrus @schmmd @nelson-liu
    0