Make jiant pip installable

[davidbenton]

This should be considered a work-in-progress, not a complete PR. It passes tests, but might break some untested things, and there are a few things I'll flag for review.

Because I'm still getting to know jiant, I chose running the tutorial experiment using a pip-installed package as my goal. This changeset allows that (without requiring conda or manual dependency installation). It doesn't directly address setting up an environment or downloading data using the pip package. To try it yourself, you can:

# create a new python virtual environment (conda not required)
# set up environment variables per tutorial, including pointing to an existing data directory, e.g.
export JIANT_PROJECT_PREFIX=~/jiant/tutorial
export JIANT_DATA_DIR=~/jiant/data

# copy default.conf from repo

# copy tutorial config file content from https://github.com/nyu-mll/jiant/blob/master/tutorials/setup_tutorial.md to tutorial.conf

# install package
pip install --upgrade --index-url https://test.pypi.org/simple/ jiant==1.1.0a0

# run experiment
python -m jiant --config_file tutorial.conf

Some items worth reviewing:

• A maintainer should check the metadata in setup.py - I took a shot at it...
• Config was moved to inside the package, per setuptools recommendation, but this could break things. I only tried the tutorial and the test suite.
• Assuming a distributed package should include only what is required to run typical tasks, I only added package dependencies necessary to run the tutorial. Are there other tasks/tests that should be considered?
• This doesn't expose any shell or python scripts as CLI commands, but we might want some for downloading data or generating a local copy of default.conf, for example.
• I moved main.main() into jiant.__main__ for convenience of running python -m jiant, and to keep necessary code inside the jiant package. I kept main.py for backward compatibility, and there's some duplicated code, but not too much.
• tests and other root-level packages were not included in the package. Does this satisfy the need for a pip package without the other packages? If no, it might make sense to reorganize some directories.

[pep8speaks]

Hello @davidbenton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 You can repair most issues by installing black and running: black -l 100 ./*. If you contribute often, have a look at the 'Contributing' section of the README for instructions on doing this automatically.

Comment last updated at 2019-08-24 00:19:26 UTC

[davidbenton]

see #877

[davidbenton]

I see I need to get demo.py running to pass CI, which should be straightforward. I have just been running nose2.

The PEP 8 issues aren't picked up by black locally, and I'm not sure why, but I'll clean that up too.

[davidbenton]

Okay, minor changes made. crossing my fingers on CI

Another item to note: if these changes are going in the right direction, documentation will need some updates, including change to config paths and pip installation.

[pruksmhc]

Thanks for taking this on! On your questions, most definitely run the superglue baselines and make sure the changes don't break anything from those runs (scripts/superglue-baselines.sh).
I'll defer to @iftenney on the last pip question.

[sleepinyourhat]

Thanks for putting this together!

Moving config/ and main() seems reasonable. I'm fine with keeping a legacy main.py and deleting it at a future release.

[sleepinyourhat]

GitHub is treating this as a substantial diff rather than a move. Are there any substantial changes here?

[davidbenton]

Only changes are making a long string multi-line, and using pkg_resources to locate defaults.conf if no config is passed.

[sleepinyourhat]

Update.

[davidbenton]

I get a double import warning for ipdb when I run it that way. ipdb docs say that formulation is python 2.7 only. Are we supporting python 2.7? 3.X? I've been using 3.6 and 3.7 for the two environments I've been working in.

[sleepinyourhat]

Hrm—I haven't been using ipdb lately. Does anyone else here know what the recommended practice is here?

We're only aiming to support 3.6/3.7—we should also clarify that somewhere...

[sleepinyourhat]

Also: We haven't been enforcing the PEP8 issues that don't also pop up in Black, though some of them can still be worth looking at. We should probably turn off the PEP8 bot...

Thanks!

[sleepinyourhat]

And to confirm, this generally all looks productive/reasonable. LMK when it's ready for a final look.

[davidbenton]

Thanks for review and comments! I'm glad it's going in the right direction.

@pruksmhc I ran the boolq task locally using superglue-baseline.sh. No surprise, but it's a bit slow to run with no GPU, ha! After about four days, it completed without error. That's a successful smoke test, but let me know if all (or other specific) tasks need to be run.

@sleepinyourhat Functionally, I think we have pip-installability complete, but depending what they're doing, users might run into some issues using jiant via pip. Specifically, they won't have easy access to the config files or shell scripts. You can see examples of this in my pip tutorial recipe above, where data and defaults.conf need to be manually fetched/located by the user. There are some options available to us for addressing this, but I don't have good sense of real-world use cases.

Do we need to make it easier for pip users to get config files or use some shell scripts? If yes, is that in scope for #877, or open new issue for it?

[sleepinyourhat]

That sounds reasonable. This PR seems to be net helpful as long as the standard workflow (clone, create/edit config files, modify code) isn't any harder. We don't have a great case study workflow to try that would involve pip yet, though. Once we start experimenting with flambé and similar tools, the pain points will become clearer.

[davidbenton]

After a thorough review, I see that defaults.conf is the only config file ever needed at runtime. Symlinking that into the jiant package would allow us to leave other config where it is, which would greatly simplify the documentation and script changes to this PR.

Is a symlink reasonable to make defaults.conf accessible from old and new paths? I've tested in OS X, and should also work the same in *nix. I believe Windows has full symlink support as of Windows 10, but I don't have an easy way to test.

[sleepinyourhat]

I suspect people will often want to name other config files explicitly, both at the command line, and as inheritances within their new config files, so we'll need to make sure that's relatively painless/documented.

I'm fine with symlinks if they're generally supported/safe for multi-platform git projects.

[davidbenton]

Symlink will work for most, but I am not confident it works for all Windows users, so I will move all config to the package (as it is in the PR now), update paths, and document. I think I have what I need, and I'll update the PR tomorrow.

[davidbenton]

Got hung up debugging the CircleCI changes, but I have a pip environment passing tests there now. Not finished with other work, but going to call it a day on this. Will pick this up by the end of the week.

[davidbenton]

Okay, I think this is ready for review @sleepinyourhat.

Note the significant changes to CircleCI, and let me know if you have questions about anything there. I saw in my merge just now that #897 specified a version of conda, which this wipes out. Not sure if that's a red flag or not.

I updated config paths everywhere, but did not attempt to run every script, etc. I did, as noted above, run one task from superglue-baselines.sh.

[davidbenton]

Oh, and I'll create the documentation PR if the README.md copy is approved.

[sleepinyourhat]

Great, thanks! I'll merge tomorrow unless anyone else raises concerns.

[sleepinyourhat]

This seems fine to me, as long as we have a fixed version and tests are passing.

[davidbenton]

Great! You'll need to create a PyPI account to upload package distributions, if you don't have one, and then the notes in setup.py should cover it.

[HaokunLiu]

Hey @davidbenton , I need to verify a new branch in jiant is pip installable, what do I need to do?

[sleepinyourhat]

Installing git repos with pip should be pretty standard. Roughly, IIRC: Make a clone/copy of the repo, exit any environment/virtualenv you're using that already has jiant set up, and run something like pip install jiant from the directory. Then try running demo.conf and/or any additional code you need to run to verify that the dependencies that you're adding loaded correctly.

[davidbenton]

@HaokunLiu Everything above seems right, but I'd recommend going a step further and creating a new virtual environment to make sure you're not pulling from existing global/user installed dependencies. The CircleCI config.yml steps create a virtual environment and pip install the package from local filesystem, so you can use that as a recipe, if it's helpful.

[HaokunLiu]

Thanks, I finally did it.
BTW, does what you said mean, any PR that passes the CircleCI check is automatically pip installable?

[davidbenton]

Yes, that's right. As pip use cases emerge, it might be necessary to add some more checks, but CircleCI currently does basic package installation via pip, including dependencies.

[HaokunLiu]

@davidbenton I've got another problem. Using RoBERTa tokenizer needs to run "python -m spacy download en". I can manually run this when running experiments. But when I'm using Robert tokenizer in a test script, there is no place for me to ask spacy to download en. Is there any way we can do this in setup, or somewhere else?