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

Create allennlp-hub repo #3351

Closed
matt-gardner opened this issue Oct 11, 2019 · 5 comments
Closed

Create allennlp-hub repo #3351

matt-gardner opened this issue Oct 11, 2019 · 5 comments
Assignees
@brendan-ai2
Milestone
1.0.0

Comments

@matt-gardner
Copy link
Contributor

We're splitting out the models and dataset readers into task-specific repositories, keeping the core abstractions in a more lightweight main library. We still want somewhere to go to get all of the pretrained models, though. We should create a repository called something like AllenNLP Hub that pip installs all of the task-specific repos, then exposes something like our pretrained.py and our sniff tests.

The main repo would then pip install the hub during CI, to run the sniff tests periodically and make sure we're not breaking anything downstream.
@matt-gardner matt-gardner added this to the 1.0.0 milestone Oct 11, 2019
@brendan-ai2 brendan-ai2 self-assigned this Oct 11, 2019
@brendan-ai2 brendan-ai2 mentioned this issue Oct 11, 2019
Merged
@brendan-ai2 brendan-ai2 mentioned this issue Oct 24, 2019
Merged
brendan-ai2 added a commit to allenai/allennlp-semparse that referenced this issue Oct 24, 2019
@brendan-ai2
Add setup.py so we're pip installable. (#10)
937d594 
- IIUC, this is most of what's needed to make a formal release, but I haven't tested that. Local-only so far.
- Installing dependencies that aren't on pypi proved clunkier than expected.
  - For now I'm going to set the CI to manually run `pip install --editable allennlp` for on a local checkout.
  - The allennlp specified in `setup.py` will be excluded with an environment variable.
- Prereq for allenai/allennlp#3351.
@brendan-ai2
Copy link
Contributor

brendan-ai2 commented Oct 25, 2019 •
edited
Loading

I've created https://github.com/allenai/allennlp-hub and an associated build for the sniff tests on Team City.

The basic functionality works, but there's a bit of cleanup to be done. In particular:

  1. Get allennlp-hub properly reviewed. I was merging PRs without reviews to quickly debug TC issues.
  2. Find/create sniff tests for allennlp-semparse. Strangely it looks like the semparse models weren't included in pretrained.py even before the sub-repo split, but I may be missing something.
  3. Push some broken code in a few different combinations and verify that the new build catches the bug.

@brendan-ai2
Copy link
Contributor

@matt-gardner your advice on 2) would be appreciated! I'm out until Tuesday, but I'll wrap up the remaining pieces then.

@matt-gardner
Copy link
Contributor Author

matt-gardner commented Oct 28, 2019 •
edited
Loading

This is awesome, thanks @brendan-ai2! You're right that we never had sniff tests for the parsing models, and I'm not really sure why not.

As for adding things, it would be ideal if we didn't have to modify this repo in order to add a new semantic parser to the hub, just add something to a pretrained.py in allennlp-semparse, or something. That gets a little magical, though, and might not be easy to accomplish.

One option is to do a named approached, similar to "bert-base-cased", where you call hub.get_model(subrepo, model_name). You don't get the benefits of types and autocomplete when doing it that way, though, which is a nice feature of our current pretrained.py.

Probably the right thing to do is just put a few existing models (e.g., ones that were used for papers) into pretrained.py, without worrying about setting something similar up in each sub repo. This is what you've already put in here, it just means adding in a few models from the semparse repo. Probably the right ones are the models that are serving the semantic parsing demos that we have.

This was referenced Nov 2, 2019
Merged
Merged
brendan-ai2 added a commit to allenai/allennlp-hub that referenced this issue Nov 15, 2019
@brendan-ai2
Add pretrained models and sniff tests for allennlp_semparse. (#4)
2b28b80 
- For allenai/allennlp#3351.
- Conveniently allenai/allennlp#3361 broke `allennlp_semparse` a while back, so the (AllenNLP Hub Master Build)[http://build.allennlp.org/viewType.html?buildTypeId=AllenNLPHub_Master] should break when this PR merged.
  - We should then fix `allennlp-semparse` and verify that the build goes green.
@brendan-ai2 brendan-ai2 mentioned this issue Nov 15, 2019
Merged
brendan-ai2 added a commit to allenai/allennlp-hub that referenced this issue Nov 15, 2019
@brendan-ai2
Add Dockerfile.release (#5)
9b091fe 
- Ensures that the versions of `allennlp` and `allennlp-semparse` specified in `requirements.txt`/`setup.py` are compatible.
  - Corresponding build: http://build.allennlp.org/viewType.html?buildTypeId=AllenNLPHub_Release
- The existing dockerfile and TC build work only on the various `master`s. 
- For allenai/allennlp#3351
@matt-gardner
Copy link
Contributor Author

@brendan-ai2, can this be closed now?

@schmmd
Copy link
Member

schmmd commented Jan 6, 2020

See github.com/allenai/allennlp-hub

@schmmd schmmd closed this as completed Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@brendan-ai2 brendan-ai2

Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

No branches or pull requests
3 participants
@schmmd @matt-gardner @brendan-ai2