allenai / deep_qa

A deep NLP library, based on Keras / tf, focused on question answering (but useful for other NLP too)
Apache License 2.0
404 stars 132 forks source link

Add a "Setting up a development environment" section to the README. #385

Closed schmmd closed 7 years ago

schmmd commented 7 years ago

Here are some modest changes to the README.

  1. Add a "Setting up a development environment" section to the root README.
  2. Remove most of the deep_qa/ README as it was redundant.
  3. A few small tweaks.

Feel free to give me all the feedback you've got--I think these are improvements but that can be pretty subjective.

I experimented for a bit to see if I could remove PYTHONHASHCODE from the Getting Started instructions--but it looks like that's not possible. I also learned that if you run tests without PYTHONHASHCODE set (or set to another value) the tests fail for cryptic reasons as the checks are not run.

The Getting Started section largely copies the Dockerfile. I think it would be great to have a Dockerfile that creates a development environment--the current one doesn't work because it installs tensorflow-gpu rather than tensorflow so when you run it on your Mac you get a CUDA exception.

matt-gardner commented 7 years ago

And I agree that if we can get PYTHONHASHSEED set in pytest, that'd be lovely. Is there a way to set environment variables in pytest.ini? (and do they load before the python interpreter?)

I think it'd be fine to have a parallel dockerfile that's for CPU usage instead of GPU usage, though I don't know enough about docker to know if it's even allowed to have two dockerfiles in the same directory, or how that works if it's possible.

schmmd commented 7 years ago

@matt-gardner I'll follow up with Jesse to learn about Dockerfile best practices here. I'm also not sure what's the best approach.

DeNeutoy commented 7 years ago

Could we use a --local ARG in the Dockerfile which would then install the correct tensorflow package, like we have for the json parameter files atm?

matt-gardner commented 7 years ago

I think the argument is probably the easiest idea, though I'd call the option --no-gpu instead of --local.