feat(clp-package)!: Containerize the scripts in sbin via Docker Compose; Remove redundant assets from the package (resolves #1358).
#1512
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Member
Author
There was a problem hiding this comment. we need to make sure the @coderabbitai create an issue to track this
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've created issue #1558 to track this concern about ensuring You're right to flag this—the 🧬 Code Graph Analysis Results[content below identifies relevant snippets to address the user comment; do not perform the user comment analysis here]
Note: The user comment mentions ensuring .resolve() calls properly resolve relative and symlinks in host path space so that "mounted_path"s are valid inside containers. The above snippets illustrate:
🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| package_root=$(readlink -f "$script_dir/..") | ||
|
|
||
| if [[ -n "${CLP_HOME:-}" ]]; then | ||
| export CLP_HOME="$CLP_HOME" | ||
| else | ||
| export CLP_HOME="$package_root" | ||
| fi | ||
|
|
||
| image_id_file="$CLP_HOME/clp-package-image.id" | ||
| version_file="$CLP_HOME/VERSION" | ||
|
|
||
| if [[ -f "$image_id_file" ]]; then | ||
| image_id="$(tr -d '[:space:]' < "$image_id_file")" | ||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| export CLP_PACKAGE_CONTAINER_IMAGE_REF="$image_id" | ||
| elif [[ -f "$version_file" ]]; then | ||
| version="$(tr -d '[:space:]' < "$version_file")" | ||
junhaoliao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| export CLP_PACKAGE_CONTAINER_IMAGE_REF="clp-package:$version" | ||
| else | ||
| echo "Error: Neither clp-package-image.id nor VERSION file exists." >&2 | ||
| return 1 2>/dev/null || exit 1 | ||
| fi | ||
|
|
||
| uid="$(id --user 2>/dev/null || echo "1000")" | ||
| gid="$(getent group docker | cut -d: -f3 2>/dev/null || echo "999")" | ||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| export CLP_FIRST_PARTY_SERVICE_UID_GID="$uid:$gid" | ||
|
|
||
| export CLP_PWD_HOST="$(pwd)" | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if [[ -z "${CLP_DOCKER_PLUGIN_DIR:-}" ]]; then | ||
| for dir in \ | ||
kirkrodrigues marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| "$HOME/.docker/cli-plugins" \ | ||
| "/mnt/wsl/docker-desktop/cli-tools/usr/local/lib/docker/cli-plugins" \ | ||
| "/usr/local/lib/docker/cli-plugins" \ | ||
| "/usr/libexec/docker/cli-plugins"; do | ||
|
|
||
| compose_plugin_path="$dir/docker-compose" | ||
| if [[ -f "$compose_plugin_path" ]]; then | ||
| export CLP_DOCKER_PLUGIN_DIR="$dir" | ||
| break | ||
| fi | ||
| done | ||
| if [[ -z "${CLP_DOCKER_PLUGIN_DIR:-}" ]]; then | ||
| echo "Warning: Docker plugin directory not found; Docker Compose may not work inside container." >&2 | ||
| fi | ||
| fi | ||
|
|
||
| socket="$(docker context inspect --format '{{.Endpoints.docker.Host}}' 2>/dev/null | sed -E 's|^unix://||')" | ||
| if [[ -S "$socket" ]]; then | ||
| export CLP_DOCKER_SOCK_PATH="$socket" | ||
| fi | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
Member
Author
There was a problem hiding this comment. we should or so the scripts early exit on any error or on any unset variables, and make a pipeline fail if any command in it fails @coderabbitai create an issue to track the improvement
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've created issue #1556 to track adding error handling flags to the shell scripts in the package template. The issue covers all the scripts in 🧠 Learnings used |
||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/../.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.archive_manager \ | ||
| "$@" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/../.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.dataset_manager \ | ||
| "$@" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.compress_from_s3 \ | ||
| "$@" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.compress \ | ||
| "$@" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.decompress \ | ||
| "$@" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.search \ | ||
| "$@" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.start_clp \ | ||
| "$@" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| common_env_path="$script_dir/.common-env.sh" | ||
| source "$common_env_path" | ||
|
|
||
| docker compose -f "$CLP_HOME/docker-compose.runtime.yaml" run --rm clp-runtime \ | ||
| python3 \ | ||
| -m clp_package_utils.scripts.stop_clp \ | ||
| "$@" |
|
Member
Author
There was a problem hiding this comment. we don't really have to use Docker Compose for this container definition, though Docker Compose helps it keep structured and avoids duplicate code.
Member
Author
There was a problem hiding this comment. host network mode should be used or the start_clp.py script cannot properly check what IP addresses it can bind to on the host. @coderabbitai file an issue
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've created issue #1560 to track enabling host network mode for the 🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| x-service-defaults: &service_defaults | ||
| image: "${CLP_PACKAGE_CONTAINER_IMAGE_REF:-clp-package}" | ||
| logging: | ||
| driver: "local" | ||
| stop_grace_period: "60s" | ||
|
|
||
| services: | ||
| clp-runtime: | ||
| <<: *service_defaults | ||
| hostname: "clp_runtime" | ||
| user: "${CLP_FIRST_PARTY_SERVICE_UID_GID:-1000:999}" | ||
| environment: | ||
| CLP_HOME: "${CLP_HOME}" | ||
| CLP_PWD_HOST: "${CLP_PWD_HOST:-${PWD}}" | ||
| volumes: | ||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - "${CLP_DOCKER_PLUGIN_DIR}:/usr/local/lib/docker/cli-plugins:ro" | ||
| - "${CLP_DOCKER_SOCK_PATH}:/var/run/docker.sock" | ||
| - "${CLP_HOME}:${CLP_HOME}" | ||
| - "/usr/bin/docker:/usr/bin/docker:ro" | ||
| stdin_open: true | ||
| tty: true | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
mounts like this have to be removed, or they conflict with the contents inside the docker image.
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.
Sorry, can you explain more?
Uh oh!
There was an error while loading. Please reload this page.
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.
sorry, that comment was so unclear that it took me time to recall why it was done -
on the host, clp_home is essentially the clp-package directory root. previously we were mounting the package root into the container so the python modules in
lib/python3/site-packagescan be accessed in the container. however, we have now removed such contents from the package (recall "now the package is only 180K"); if we mount an empty directory into/opt/clpin the container, anything in/opt/clpis essentially gone. we should just use the assets in the docker image and remove such mounts then