-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add docker-server and docker-server-pytorch #1602
Add docker-server and docker-server-pytorch #1602
Conversation
24aa633
to
86b123a
Compare
@@ -22,14 +23,45 @@ dependencies { | |||
// runtimeOnly project(':log-to-logbuffer') | |||
} | |||
|
|||
application { | |||
distributions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the removal of the grpc-api-server-docker, it probably makes sense to move this up to grpc-api-server itself?
I'm content to have this change happen later, probably in the jetty branch where I'm renaming grpc-api to server anyway, and adding jetty vs netty variants, but just want to be sure this makes sense as a plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd support a rename.
### Run | ||
|
||
```shell | ||
./gradlew grpc-api-server-native:run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still runnable, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the way I've got it configured, not via the gradle run. I'll add new instructions on how to run locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Jianfeng and I have a short-ish set of steps to make py work from either netty or jetty run locally, without docker.
I'll follow up with trying to implement this same basic pattern for the web image's contents - breaking it up into "here's an archive with contents that jetty can serve statically" and "here's a docker image based on nginx" will fit well into the work i'm doing. |
86b123a
to
3800052
Compare
file for future work:
|
3800052
to
d3be4a3
Compare
This doesn't have "arch support in bootstrap.properties" - happy to extend support, but I've not tried to build arm64. I'm assuming it may be better as a follow up to get full stack arm64 support? |
stdout.toLogBuffer=true | ||
stderr.toLogBuffer=true | ||
|
||
jpy.jpyLib=/usr/local/lib/python3.7/dist-packages/jpy.cpython-37m-x86_64-linux-gnu.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file could be modified when copied, such that the arch and py version can be parameterized by a future map.
definitely not requesting that you try arm, just requesting that the arg be made available, plumbed through the gradle/docker barrier so we have one place to change, as long as we're already providing the ability to parameterize the server build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually like to parameterize for architecture first before trying to architecture for python version; and before parameterizing for python version, I think we should work on building up the stack as a virtual environment.
This bootstrap file is meant to aid in just the docker image building process; I expect we'll need to parameterize our native run instructions more explicitly to pick up python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameterizing for arch outside of properties like this is only an extra 1-3 lines in Docker.groovy - with the other images moved out externally (and already building aarch64), this should be the last piece we need to fix.
### Run | ||
|
||
```shell | ||
./gradlew grpc-api-server-native:run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Jianfeng and I have a short-ish set of steps to make py work from either netty or jetty run locally, without docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, with the "file and followup on aarch64 in that properties file" follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubber stamp for the gitlab file; there's one comment that maybe worth deleting, and I'm not uptodate on what crane is doing, but the changes to the file itself look valid.
uses: docker/metadata-action@v3 | ||
with: | ||
images: ghcr.io/${{ github.repository_owner }}/grpc-api | ||
# grpc-api deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what up w/ this comment?
I ran a successful build in my repo - you should be able to pull:
Note: these are large images.