8000 Upgrade Dockerfile to stretch. by schmmd · Pull Request #2647 · 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.

Upgrade Dockerfile to stretch. #2647

Merged
merged 4 commits into from
Mar 27, 2019
Merged

Upgrade Dockerfile to stretch. #2647

merged 4 commits into from
Mar 27, 2019

Conversation

schmmd
Copy link
Member
@schmmd schmmd commented Mar 26, 2019 •
edited
Loading

@kyleclo found that jessie gave up support for their apt package so, when using our Docker image, they needed to add the following as a work-around. I didn't realize there was a newer stable release of Debian (stretch) but now that I do I'm upgrading to it.

RUN echo "deb http://deb.debian.org/debian/ jessie main" > /etc/apt/sources.list \
  && echo "deb-src http://deb.debian.org/debian/ jessie main" >> /etc/apt/sources.list \
  && echo "deb http://security.debian.org/ jessie/updates main" >> /etc/apt/sources.list \
  && echo "deb-src http://security.debian.org/ jessie/updates main" >> /etc/apt/sources.list \
  && echo "deb [check-valid-until=no] http://archive.debian.org/debian jessie-backports main" >> /etc/apt/sources.list \
  && echo "deb-src [check-valid-until=no] http://archive.debian.org/debian jessie-backports main" >> /etc/apt/sources.list

See https://unix.stackexchange.com/questions/508724/failed-to-fetch-jessie-backports-repository for more details about the deprecation of apt repositories for jessie.

@schmmd schmmd requested a review from brendan-ai2 March 26, 2019 21:29
@schmmd schmmd changed the title Upgrade Dockerfile to stretch. WIP: Upgrade Dockerfile to stretch. Mar 26, 2019
schmmd
schmmd commented Mar 26, 2019
View reviewed changes
Dockerfile

# Install Java.
RUN echo "deb http://http.debian.net/debian jessie-backports main" >>/etc/apt/sources.list
RUN apt-get update
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this update was previously necessary because of the rm -rf /var/lib/apt/lists/* immediately proceeding it--so this change should make the build a bit faster too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure updating is just necessary after a new source is added.

Copy link
Member Author

Choose a reason for hiding this comment

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

@brendan-ai2 look earlier in the Dockerfile. We no longer need to add a new source (since we're on stretch) and we no longer need to update here because we do earlier. An update would have been required for all installs after rm -rf /var/lib/apt/lists/*, so the previous iteration of this Dockerfile was inefficient (it would create the apt lists, nuke them, and then recreate them).

Copy link
Member Author
@schmmd schmmd Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

My last comment wasn't particularly clear. My point was just that the previous Dockerfile should have been reorganized so the jdk install happened before the caches were cleared.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, reorganizing would have been ideal. At any rate, thanks again for fixing this!

@schmmd schmmd changed the title WIP: Upgrade Dockerfile to stretch. Upgrade Dockerfile to stretch. Mar 26, 2019
@schmmd
Copy link
Member Author
schmmd commented Mar 26, 2019

I tested Dockerfile.pip manually by building it locally since it only runs on master commit as I recall.

docker build -f Dockerfile.pip -t allennlp-pip .
docker run allennlp-pip test-install

@schmmd schmmd mentioned this pull request Mar 26, 2019
Merged
brendan-ai2
brendan-ai2 reviewed Mar 26, 2019
View reviewed changes
Dockerfile Outdated
@@ -30,13 +30,8 @@ RUN apt-get update --fix-missing && apt-get install -y \
libxrender1 \
wget \
libevent-dev \
build-essential && \
rm -rf /var/lib/apt/lists/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This was probably here to keep down the size of the image, right? See https://askubuntu.com/questions/179955/var-lib-apt-lists-is-huge for instance. Should we retain this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we should. In fact, I didn't mean to remove it.

brendan-ai2
brendan-ai2 approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor
@brendan-ai2 brendan-ai2 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @schmmd. Left a minor comment. LGTM after that!

@kyleclo
Copy link
Contributor
kyleclo commented Mar 27, 2019 •
edited
Loading

thx to danielkingai2 for finding it.
q: confirming that i'll need to keep using the workaround until the docker image for allennlp v0.8.3 is uploaded w/ this fix?

@brendan-ai2
Copy link
Contributor

That's my understanding. Though if you need to urgently get something working, you could pull down this PR and build the Dockerfile directly. (I'd anticipate this being merged and image uploaded tomorrow.)

@schmmd
Copy link
Member Author
schmmd commented Mar 27, 2019

@kyleclo we push image per commit, so you wouldn't need to build your own image. See https://hub.docker.com/r/allennlp/commit/tags. But it's preferable to be on a release since we know more about releases than commits.

@matt-gardner
Copy link
Contributor

I tested Dockerfile.pip manually by building it locally since it only runs on master commit as I recall.

docker build -f Dockerfile.pip -t allennlp-pip .
docker run allennlp-pip test-install

Quick sanity check - did you try it with --run-all? You're changing the java installation, but you didn't run the java tests.

@schmmd
Copy link
Member Author
schmmd commented Mar 27, 2019

@matt-gardner Dockerfile.pip didn't previously install the JVM and still doesn't after this PR, so that test isn't necessary. JVM tests ran as part of continuous build, so the Dockerfile used for continuous build ran the JVM tests.

For example, see:

[08:13:20][Step 3/10] allennlp/tests/predictors/wikitables_parser_test.py::TestWikiTablesParserPredictor::test_answer_present PASSED [ 74%]
[08:13:23][Step 3/10] allennlp/tests/predictors/wikitables_parser_test.py::TestWikiTablesParserPredictor::test_answer_present_with_batch_predict PASSED [ 74%]

@schmmd schmmd merged commit 263d340 into allenai:master Mar 27, 2019
@schmmd schmmd deleted the stretch branch March 27, 2019 16:00
@ibeltagy ibeltagy mentioned this pull request Apr 2, 2019
Merged
reiyw pushed a commit to reiyw/allennlp that referenced this pull request Nov 12, 2019
@schmmd @reiyw
Upgrade Dockerfile to stretch. (allenai#2647)
92b1fe7
TalSchuster pushed a commit to TalSchuster/allennlp-MultiLang that referenced this pull request Feb 20, 2020
@schmmd @TalSchuster
Upgrade Dockerfile to stretch. (allenai#2647)
40f31a2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@brendan-ai2 brendan-ai2 brendan-ai2 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.
4 participants
@schmmd @kyleclo @brendan-ai2 @matt-gardner
0