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

fixing passing port as a build argument to containers #258

Conversation

Gregory-Pereira
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira commented Apr 13, 2024

I discovered when playing around with builds that the way we were passing PORT arguments wasnt working, because we both were exposing a static port number in the Containerfile and because we were passing the actual argument for it incorrectly:

# Tested from the `object_detection_python` model_server but all builds were the same
$ make PORT=20203 build
podman build --squash-all \
	  --build-arg PORT=20203 \
	  -t quay.io/ai-lab/model_servers/object_detection_python:latest . -f base/Containerfile
...
STEP 6/7: EXPOSE 8000
...
Successfully tagged quay.io/ai-lab/model_servers/object_detection_python:latest
bd834bd87c2605cf5e0cc93ee3e4c23c9b7fb030040cea6eb56b0e401efd4e27

Then verified inspecting the image:

podman inspect bd834bd87c2605cf5e0cc93ee3e4c23c9b7fb030040cea6eb56b0e401efd4e27
...
"ExposedPorts": {
    "8000/tcp": {},
    "8080/tcp": {}
},

The new builds produce the following:

$ make PORT=20203 build
...
STEP 2/8: ARG PORT
...
STEP 7/8: EXPOSE $PORT
...
Successfully tagged quay.io/ai-lab/model_servers/object_detection_python:latest
bd834bd87c2605cf5e0cc93ee3e4c23c9b7fb030040cea6eb56b0e401efd4e27
$ podman inspect bd834bd87c2605cf5e0cc93ee3e4c23c9b7fb030040cea6eb56b0e401efd4e27
...
"ExposedPorts": {
    "20203/tcp": {},
    "8080/tcp": {}
},

@Gregory-Pereira
Copy link
Collaborator Author

Note: I chose not to add this to any Bootc Containerfiles for the NLP recipes, due to my lack of understanding about what is actually happening there.

@rhatdan
Copy link
Member

rhatdan commented Apr 13, 2024

LGTM, but this should also be for bootc containers. It would only be used when they run as an OCI Container not when they are installed onto a full OS.

@Gregory-Pereira
Copy link
Collaborator Author

Gregory-Pereira commented Apr 13, 2024

LGTM, but this should also be for bootc containers. It would only be used when they run as an OCI Container not when they are installed onto a full OS.

So if I am understanding this it should be baked into the bootc build commands but not whatever actually runs the bootc image?

@Gregory-Pereira
Copy link
Collaborator Author

@rhatdan I have included my attempt at the bootc changes, could you give it a once over whenever? I ended up including it in the bootc build command and the run command because I think I misunderstood, my current understanding is that these make targets would only run bootc as a OCI container, but that could be wrong too. Also included testing path changes to try to get this picked up by tests, I will move those to a separate PR.

@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2024

@MichaelClifford PTAL, are the exposed PORTS above static and defined in Code?

@Gregory-Pereira Gregory-Pereira force-pushed the pass-ports-to-container-builds branch 2 times, most recently from 6320fc1 to 4e615b3 Compare April 15, 2024 15:01
@Gregory-Pereira Gregory-Pereira force-pushed the pass-ports-to-container-builds branch from 4e615b3 to 040b855 Compare April 15, 2024 15:06
@MichaelClifford
Copy link
Collaborator

For the app's the port could change depending on the model server they are connecting to. Our llamacpp_python uses 8001 where as ollama uses 11434. (We could probably make some changes so they all use the same port though)

And all the Apps just use 8501 which is streamlit's default. So those don't ever really change.

@@ -101,7 +102,7 @@ bootc: quadlet

.PHONY: bootc-run
bootc-run:
podman run -d --rm --name $(APP)-bootc -p 8080:8501 --privileged \
podman run -d --rm --name $(APP)-bootc -p 8080:$(PORT) --privileged \
Copy link
Member

Choose a reason for hiding this comment

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

This should be ${PORT}:80 or something like that.

@Gregory-Pereira
Copy link
Collaborator Author

Not sure this is going to end up working. I dusted it off and I am encountering an issue with all the recipe pieces where its very difficult to pass build time ARGs to runtime ARGs in entrypoint, which is discussed in this stack overflow thread: https://stackoverflow.com/questions/40902445/using-variable-interpolation-in-string-in-docker. In addition to this, it would require us in the recipe's make file or recipe's common makefile to specify the server port and build it from there, which goes against our intended design. For those reasons I think this is better off dropped and we have to use static ports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants