Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a Dockerfile and ignore example artifacts #15

Merged
merged 1 commit into from Feb 20, 2019
Merged

Add a Dockerfile and ignore example artifacts #15

merged 1 commit into from Feb 20, 2019

Conversation

madisonmay
Copy link
Contributor

@madisonmay madisonmay commented Feb 14, 2019 •
edited

If you'd like, here's a Dockerfile to toss up as an alternate installation method.

Also quickly gitignored the samples file and core file generated by running the example in the README.

@madisonmay madisonmay force-pushed the madison/dockerfile branch 2 times, most recently from 137065a to 19327c1 Compare February 14, 2019 23:31
@madisonmay madisonmay changed the title Add a Dockerfile and document usage in README Add a Dockerfile and ignore example artifacts Feb 14, 2019
@minimaxir
Copy link
Contributor

You may want to add a CPU dockerfile as well.

@madisonmay
Copy link
Contributor Author

Good call. Note that dependency installation in Dockerfile.gpu is currently a bit awkward because of the tensorflow>=1.12 dependency declared in requirements.txt. Might be easier to move the tensorflow dependency out of requirements.txt and into the README.md to prevent folks accidentally overriding their tensorflow-gpu installs.

valentinvieriu
valentinvieriu reviewed Feb 15, 2019
View reviewed changes
Dockerfile.gpu Outdated Show resolved Hide resolved
@valentinvieriu
Copy link

a docker-compose.yml would be usefull here

WuTheFWasThat
WuTheFWasThat reviewed Feb 20, 2019
View reviewed changes
Dockerfile.gpu Outdated

RUN apt-get update && \
mkdir /gpt-2 && \
pip3 install fire==0.1.3 regex==2017.4.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, I should remove tensorflow from the requirements.txt perhaps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ did so, so you can install from requirements.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks for the change, it's kind of unfortunate that tensorflow installs don't play nice with requirements.txt files.

@WuTheFWasThat
Copy link
Collaborator

@madisonmay I just added a LICENSE (#10) - do you agree to license your contribution under the MIT license? Thanks!

WuTheFWasThat
WuTheFWasThat reviewed Feb 20, 2019
View reviewed changes
.gitignore Outdated
@@ -1,2 +1,4 @@
__pycache__
models/
samples
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think this needs to be in .gitignore, i could update the README to tee to /tmp/ or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the .gitignore changes then.

WuTheFWasThat
WuTheFWasThat reviewed Feb 20, 2019
View reviewed changes
.gitignore Outdated
@@ -1,2 +1,4 @@
__pycache__
models/
samples
core
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tensorflow core dump file... probably not strictly necessary here either. I'll remove.

WuTheFWasThat
WuTheFWasThat reviewed Feb 20, 2019
View reviewed changes
Dockerfile.gpu Outdated
WORKDIR /gpt-2
ADD . /gpt-2

CMD ["sleep", "infinity"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these really necessary? i would remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable, better to assume nothing about how someone might intend to use the Docker container.

@WuTheFWasThat
Copy link
Collaborator

thanks @madisonmay for the contribution! do you mind also adding instructions on docker install to the README? (feel free to rearrange stuff a bit)

@madisonmay
Copy link
Contributor Author

Happy to, held off on that because I didn't want to add any unnecessary cruft to your tidy readme. Will push that up shortly w/ the other edits.

@madisonmay madisonmay force-pushed the madison/dockerfile branch 2 times, most recently from 506c278 to b9ef977 Compare February 20, 2019 02:44
@madisonmay
Copy link
Contributor Author

Added a README section, removed the .gitignore changes, and squashed the commits back down. Also moved the model download into the Dockerfile now that the master branch uses curl instead of gsutil to download the model files and opening up a browser to auth with google isn't required.

@WuTheFWasThat
Copy link
Collaborator

@madisonmay I just added a LICENSE (#10) - do you agree to license your contribution under the MIT license? Thanks!

@madisonmay
Copy link
Contributor Author

madisonmay commented Feb 20, 2019 •
edited

@madisonmay I just added a LICENSE (#10) - do you agree to license your contribution under the MIT license? Thanks!

And yes, absolutely! Thanks for the re-ping on that.

WuTheFWasThat
WuTheFWasThat reviewed Feb 20, 2019
View reviewed changes
README.md Outdated
@@ -34,6 +34,21 @@ Install other python packages:
pip3 install -r requirements.txt
```

## Docker Installation
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe ###?

And add another sub-heading "## Plain installation" (or something similar) before "Download the model data"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, pushing up that change now.

@WuTheFWasThat WuTheFWasThat merged commit 99af6d7 into openai:master Feb 20, 2019
@madisonmay madisonmay deleted the madison/dockerfile branch February 20, 2019 02:59
jchwenger pushed a commit to jchwenger/gpt-2 that referenced this pull request Jan 2, 2020
@minimaxir
Merge pull request openai#15 from minimaxir/cli
cd257bb 
Cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valentinvieriu valentinvieriu valentinvieriu left review comments

@WuTheFWasThat WuTheFWasThat WuTheFWasThat left review comments

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@madisonmay @minimaxir @valentinvieriu @WuTheFWasThat