[Arch] Add runtime image build CI & clean up runtime build using jinja2 template#3055
Conversation
add mamba clean
make that function accessible through cli
…amically generated
xingyaoww
changed the title
jinja2 templatejinja2 template
| # Print the full name of the image | ||
| echo "Loaded Docker image: $image_name" | ||
|
|
||
| SANDBOX_CONTAINER_IMAGE=$image_name TEST_IN_CI=true poetry run pytest --cov=agenthub --cov=opendevin --cov-report=xml -s ./tests/unit/test_runtime.py |
There was a problem hiding this comment.
SANDBOX_CONTAINER_IMAGE=$image_name - this puzzles me. Is runtime image a special type of sandbox image? If so, why do we need a separate yaml file? Is it because the integration of runtime image hasn't finished yet?
There was a problem hiding this comment.
YES. Runtime to some extend is a type of Sandbox (not the SSHBox type of Sandbox class, but conceptually sandbox). And i kinda hope to make the transition relatively smooth so we can re-use these configs without introducing new stuff.
The idea is once this has been extensively test to work, we simply remove all the old Sandbox classes, ServerRuntime, and test_sandbox.py so we ended up with only one workflow for build and test runtime.
|
|
||
| import docker | ||
| import toml | ||
| from jinja2 import Environment, FileSystemLoader |
There was a problem hiding this comment.
Any benifit that introduce jinja2?
There was a problem hiding this comment.
It seems like it makes manipulating the Dockerfile a bit more straightforward, so this seems OK to me? Also, we already use jinja2 in OD, so it's not an extra dependency.
There was a problem hiding this comment.
yep! that's exactly the motivation - basically get rid of our existing string concatenation that makes creating dockerfile too complex and hard to maintain.
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
Looks good, thanks! Just a few comments.
|
|
||
| import docker | ||
| import toml | ||
| from jinja2 import Environment, FileSystemLoader |
There was a problem hiding this comment.
It seems like it makes manipulating the Dockerfile a bit more straightforward, so this seems OK to me? Also, we already use jinja2 in OD, so it's not an extra dependency.
| if base_image == new_image_name: | ||
| logger.info( | ||
| f'Using existing od_runtime image [{base_image}]. Will NOT build a new image.' | ||
| ) | ||
| else: | ||
| logger.info(f'New image name: {new_image_name}') |
There was a problem hiding this comment.
Just confirming, will this cause any issues? For instance, is there a possibility that the OpenDevin code gets updated, but the image name stays the same so it doesn't build a new image?
There was a problem hiding this comment.
Yes - it might be for the developer. I haven't thought of a good way to do versioning easily for this (e.g., people could modify code on top of the same commit, so commit hash is not useful either). Currently when developing, we ask people to set their env var to force update the code.
But it should not cause issues for user in production, bc the sandbox image is already pre-build and code is not changed.
There was a problem hiding this comment.
Seems like a good enough solution for now, happy to merge.
5 participants
What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
Need to merge this first: #3051
Give a summary of what the PR does, explaining any non-trivial design decisions
od_runtime:od_v{od_release_version}_image_{base_image_name}_tag_{base_image_tag}.test_runtime.pyintoghcr-runtimeworkflow, similar to existingghcrworkflow.Other references