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.

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

Create docker container to build graph file #1319

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

BryanQuigley
Copy link
Contributor

otp command similar to all otp docker images I found

Overview

It lets you create a graph file by running a single docker command.

Notes

I did try to see if it the container would "just work" for #1275, but it didn't.

Testing Instructions

  • Try to follow the build new instructions for setting up project.

Checklist

(no changes to these)

Connects #1270

otp command similar to all otp docker images I found
@ddohler ddohler self-assigned this Mar 24, 2022
Copy link
Contributor

@ddohler ddohler left a comment

Choose a reason for hiding this comment

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

Works great! I wasn't able to test routing from within the Django app because my app VM is somewhat messed up from the in-progress Django upgrade work, but I did test routing using the built-in OTP interface and everything seemed to work as I would have expected. I confirmed that the Graph.obj file generated by the container ended up inside the VM after running vagrant provision otp, and that the VM didn't rebuild the graph if the container-built Graph.obj was present. I also compared against a pre-existing Graph.obj file that I had lying around and they seem to be roughly the same size (959MB for the new one, 950MB for the old one).

I had a few minor comments for potential small cleanups, but nothing critical.

ENV VERSION=1.4.0 \
JAVA_MX=15G

ADD https://repo1.maven.org/maven2/org/opentripplanner/otp/$VERSION/otp-$VERSION-shaded.jar /usr/local/share/java/
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this will cause Docker to re-download the OTP jar on each rebuild so that it can calculate a checksum and decide whether to use its cached filesystem layer or not. We've historically had struggles with reliability when downloading from Maven in places that get run a lot (CI), so in the past we've sometimes manually copied dependencies to our own S3 buckets.

But I think that would be overkill for something that most people will run once and never again. Without significant development activity on the horizon for this project I think this is fine as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the Docker docs discourage the use of ADD for remote fetch in favor of something like RUN curl ... so that Docker caches based on the command string rather than the file contents, although I don't think their reasoning directly applies to this particular case because we're just copying a single file directly to a directory and not doing anything with it afterward.

RUN ln -s otp-$VERSION-shaded.jar /usr/local/share/java/otp.jar

COPY otp /usr/local/bin/
RUN chmod 755 /usr/local/bin/*
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add an ENTRYPOINT here then you'll be able to avoid specifying otp in the README command, and you might be able to remove the wrapper script entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ENTRYPOINT didn't do what i had hoped in the Dockerfile. I tried adding it to docker-compose which did appear to work, but just let me get ride of otp - not the need for a separate file. Might be a docker-compose version thing - will investigate further if I pick up #1275

@@ -0,0 +1,3 @@
#!/bin/sh

exec java -Xmx"$JAVA_MX" -jar /usr/local/share/java/otp.jar "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed when building the graph that I received a warning that build-config.json and router-config.json were absent and that default configuration would be used. I searched the repo to see if we were using either of those and found a template for router-config.json which sets up the configuration for using bike-sharing stations. However, that seems like it's only a run-time configuration and isn't used when building the graph.

So I don't think we need to work to include the router-config.json during the graph build step here, but we should thoroughly test the bike-sharing functionality the first time we deploy using this graph building process, and we'll need to include that file if we ever work on #1275.

@BryanQuigley BryanQuigley merged commit 849e730 into develop Apr 5, 2022
@BryanQuigley BryanQuigley deleted the otp_docker_image_graphing branch April 7, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants