Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Sep 30, 2025

Description

This PR adds the mechanism to the package init scripts to select the "clp-package" image built inside the project directory. For the background, "clp-package" includes all needed assets for different CLP Package components, which removes the need for extensive and tedious files mounts with the existing "clp-execution-base-ubuntu-jammy" image in the migration to Docker Compose in #1177 .

As a result, the "clp-execution-base-ubuntu-jammy" image will no longer be in use, and its build script and related GH workflow can be removed.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Expect all workflows to pass. (They are still running at the time of this writing.)
  2. Ran task -> cd build/clp-package/ -> ./sbin/start-clp.sh:
    • Observed the package was started successfully. Inspected the Docker Desktop GUI and the CLP-specific package components are all started with the image identified by id stored in build/clp-package/clp-package-image.id.
    • Ran ./sbin/compress.sh ~/samples/hive-24hr/ and observed the command exited successfully. When the containing was running, confirmed in the Docker Desktop GUI that the same image (the image identified by id stored in build/clp-package/clp-package-image.id) is used.

Summary by CodeRabbit

  • New Features

    • Config now prefers an explicit container image reference (from a container-id file or VERSION) for runtime image selection.
  • Refactor

    • Unified container-image handling across startup and tooling; removed legacy OS-specific detection.
  • Documentation

    • Removed docs and references for the Ubuntu Jammy execution image and its CI workflow.
  • Chores

    • Removed Ubuntu Jammy image assets, build scripts and CI workflow; default task now depends on docker-images packaging and packages syncs the image-id artifact.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Walkthrough

Removes the Ubuntu Jammy execution image, its CI workflow and docs; switches CLPConfig to prefer a clp-package-image.id file (fallback to VERSION) to set container_image_ref; updates call sites to use container_image_ref; adjusts taskfiles to require docker-images:package by default and copy clp-package-image.id into package outputs; simplifies the runtime-image action metadata by removing image_type.

Changes

Cohort / File(s) Summary
CI workflow removal
.github/workflows/clp-execution-image-build.yaml, docs/src/dev-docs/tooling-gh-workflows.md
Deletes the clp-execution-image-build GitHub Actions workflow and its documentation block; callers no longer reference that workflow.
Execution image (Ubuntu Jammy) removal
tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile, tools/docker-images/clp-execution-base-ubuntu-jammy/build.sh, tools/docker-images/clp-execution-base-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh, docs/src/dev-docs/tooling-containers.md
Removes the Ubuntu Jammy execution image Dockerfile, build script, setup script, and associated docs entries.
Config: container image resolution
components/clp-py-utils/clp_py_utils/clp_config.py
Replaces os-release detection with file-based resolution: adds clp-package-image.id and VERSION paths; renames loader to load_container_image_ref; sets container_image_ref from file or derives ghcr.io/y-scope/clp/clp-package:{VERSION}.
CLP usage updates
components/clp-package-utils/.../general.py, components/clp-package-utils/.../scripts/*.py, components/clp-package-utils/.../scripts/start_clp.py
Call sites updated to pass/use clp_config.container_image_ref instead of clp_config.execution_container.
Build/task flow updates
taskfile.yaml, taskfiles/docker-images.yaml
Changes default task dependency to docker-images:package; adds rsync to copy clp-package-image.id into package build outputs; removes /etc/os-release from package sources.
Action metadata simplification
.github/actions/clp-build-runtime-image/action.yaml, .github/workflows/clp-artifact-build.yaml
Removes the image_type input/argument and conditional branching; action now unconditionally computes image metadata as clp-package and callers no longer pass image_type.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLPConfig as CLPConfig
  participant FS as Filesystem

  User->>CLPConfig: load_container_image_ref()
  alt container_image_ref pre-configured
    CLPConfig-->>User: use configured container_image_ref
  else not pre-configured
    CLPConfig->>FS: exists `clp-package-image.id`?
    alt exists
      CLPConfig->>FS: read `clp-package-image.id`
      note right of CLPConfig #DDEBF7: Prefer image id file
      CLPConfig-->>User: set container_image_ref = file value
    else
      CLPConfig->>FS: read `VERSION`
      note right of CLPConfig #F7F0DB: Fallback → ghcr.io/y-scope/clp/clp-package:{version}
      CLPConfig-->>User: set container_image_ref = derived name
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title succinctly describes the two primary changes in this PR by identifying the replacement of the package execution container with the clp-package image and the update to the default task, using clear, descriptive phrasing aligned with conventional commit style and directly reflecting the modifications in the diff.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@junhaoliao junhaoliao changed the title feat(clp-package): Select execution container from image id or version file. feat(clp-package): Select execution container from image id or version file; Change task default task to docker-images:package. Sep 30, 2025
@junhaoliao junhaoliao changed the title feat(clp-package): Select execution container from image id or version file; Change task default task to docker-images:package. feat(clp-package): Replace package execution container with clp-package; Change task default task to docker-images:package. Sep 30, 2025
)
if self._image_id_path.exists():
with open(self._image_id_path) as image_id_file:
self.execution_container = image_id_file.read().strip()
Copy link
Member Author

Choose a reason for hiding this comment

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

we may change this attribute name to package_container though i feel the name execution_container is more intuitive to those who are unfamiliar with the history of the execution container

@junhaoliao junhaoliao marked this pull request as ready for review September 30, 2025 07:59
@junhaoliao junhaoliao requested a review from a team as a code owner September 30, 2025 07:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62bea59 and 2959d60.

📒 Files selected for processing (9)
  • .github/workflows/clp-execution-image-build.yaml (0 hunks)
  • components/clp-py-utils/clp_py_utils/clp_config.py (4 hunks)
  • docs/src/dev-docs/tooling-containers.md (0 hunks)
  • docs/src/dev-docs/tooling-gh-workflows.md (0 hunks)
  • taskfile.yaml (1 hunks)
  • taskfiles/docker-images.yaml (1 hunks)
  • tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile (0 hunks)
  • tools/docker-images/clp-execution-base-ubuntu-jammy/build.sh (0 hunks)
  • tools/docker-images/clp-execution-base-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh (0 hunks)
💤 Files with no reviewable changes (6)
  • docs/src/dev-docs/tooling-gh-workflows.md
  • tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile
  • .github/workflows/clp-execution-image-build.yaml
  • tools/docker-images/clp-execution-base-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
  • tools/docker-images/clp-execution-base-ubuntu-jammy/build.sh
  • docs/src/dev-docs/tooling-containers.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
🧬 Code graph analysis (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
  • make_config_path_absolute (42-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-static-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-static-linked-bins
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: package-image
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: build-macos (macos-15, false)
  • GitHub Check: build-macos (macos-15, true)
  • GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (6)
components/clp-py-utils/clp_py_utils/clp_config.py (4)

90-92: LGTM!

The new path constants are well-named and follow the existing pattern of defining relative paths that will be resolved later via make_config_path_absolute.


904-905: LGTM!

The private attributes are properly defined and replace the legacy _os_release_file_path attribute with a cleaner file-based approach.


922-923: LGTM!

The path resolution logic correctly applies make_config_path_absolute to the new private attributes, consistent with the existing pattern in this method.


1016-1023: Consider adding error handling for file I/O operations.

The file reads on lines 1017 and 1021 don't have explicit error handling. If either file is missing or unreadable, an unhandled exception will be raised. Verify whether this fail-fast behaviour is intentional or if graceful error messages would improve the user experience.

If graceful error handling is desired, consider wrapping the file reads in try-except blocks with descriptive error messages:

if self._image_id_path.exists():
    try:
        with open(self._image_id_path) as image_id_file:
            self.execution_container = image_id_file.read().strip()
    except Exception as e:
        raise ValueError(f"Failed to read image ID from '{self._image_id_path}': {e}")

if not self.execution_container:
    try:
        with open(self._version_file_path) as version_file:
            clp_package_version = version_file.read().strip()
        self.execution_container = f"ghcr.io/y-scope/clp/clp-package:{clp_package_version}"
    except Exception as e:
        raise ValueError(f"Failed to read version from '{self._version_file_path}': {e}")
taskfile.yaml (1)

55-55: Default task now builds Docker images—verify this aligns with expected workflow.

Changing the default dependency from package to docker-images:package means running task with no arguments will now build the Docker image (and the package it depends on). This increases the scope of the default build compared to the previous behavior.

Based on the PR objectives, this change is intentional and correct. However, confirm that:

  1. CI workflows and documentation reflect this new default behaviour.
  2. Developers are aware that task now includes Docker image building.
taskfiles/docker-images.yaml (1)

15-15: No action needed: build.sh writes clp-package-image.id to build/ before the rsync step.

CLP_DEFAULT_DATA_DIRECTORY_PATH = pathlib.Path("var") / "data"
CLP_DEFAULT_DATASET_NAME = "default"
CLP_METADATA_TABLE_PREFIX = "clp_"
CLP_PACKAGE_IMAGE_ID_PATH = pathlib.Path("clp-package-image.id")
Copy link
Member

Choose a reason for hiding this comment

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

How about CLP_PACKAGE_CONTAINER_IMAGE_ID_PATH? Similarly, how about _container_image_id_path?

self.execution_container = image_id_file.read().strip()

self.execution_container = "ghcr.io/y-scope/clp/" + self.execution_container
if self.execution_container == "":
Copy link
Member

Choose a reason for hiding this comment

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

self.execution_container is None by default which means this branch won't get triggered unless the image ID file is empty, right? It also feels a little inconsistent to validate that the image ID file isn't empty, but we don't do the same for the version file.

I think we should expect both of them to have content and let Python throw an exception if they don't. So I would write this as:

        if self._image_id_path.exists():
            with open(self._image_id_path) as image_id_file:
                self.execution_container = image_id_file.read().strip()
        else:
            with open(self._version_file_path) as version_file:
                clp_package_version = version_file.read().strip()
            self.execution_container = f"ghcr.io/y-scope/clp/clp-package:{clp_package_version}"

)
if self._image_id_path.exists():
with open(self._image_id_path) as image_id_file:
self.execution_container = image_id_file.read().strip()
Copy link
Member

Choose a reason for hiding this comment

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

How about container_image_ref?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2959d60 and 99c919c.

📒 Files selected for processing (2)
  • components/clp-py-utils/clp_py_utils/clp_config.py (4 hunks)
  • taskfiles/docker-images.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
🧬 Code graph analysis (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
  • make_config_path_absolute (42-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: package-image
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-static-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-static-linked-bins
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: rust-checks (ubuntu-22.04)
🔇 Additional comments (4)
taskfiles/docker-images.yaml (1)

15-15: LGTM! The rsync command correctly propagates the image ID file.

The command properly copies the clp-package-image.id file from the build directory to the package build directory, which aligns with the PR's objective to enable the package to use the locally built image.

components/clp-py-utils/clp_py_utils/clp_config.py (3)

92-92: LGTM! The version file path constant is clear and appropriate.


922-923: LGTM! Path resolution is correctly applied to both private attributes.

The changes appropriately resolve the image ID path and version file path to absolute paths using make_config_path_absolute.


1016-1022: LGTM! The fallback logic correctly implements the if-else pattern.

The implementation now properly uses an if-else structure: if the image ID file exists, it reads the image ID; otherwise, it reads the VERSION file to construct the default container reference. This addresses the concerns raised in previous review comments about the fallback logic.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Can we get rid of the logic to build execution images here?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)

1015-1026: Add validation for empty file contents.

After reading and stripping the file contents (lines 1022 and 1025), the method should validate that container_image_ref or clp_package_version is not empty. Without this check, an empty or whitespace-only file will result in an empty string being used as the container image reference, which will cause downstream Docker commands to fail.

Apply this diff to add validation:

     def load_container_image_ref(self):
         if self.container_image_ref is not None:
             # Accept configured value for debug purposes
             return
 
         if self._container_image_id_path.exists():
             with open(self._container_image_id_path) as image_id_file:
                 self.container_image_ref = image_id_file.read().strip()
+                if not self.container_image_ref:
+                    raise ValueError(
+                        f"Empty container image reference in '{self._container_image_id_path}'"
+                    )
         else:
             with open(self._version_file_path) as version_file:
                 clp_package_version = version_file.read().strip()
+                if not clp_package_version:
+                    raise ValueError(
+                        f"Empty version string in '{self._version_file_path}'"
+                    )
             self.container_image_ref = f"ghcr.io/y-scope/clp/clp-package:{clp_package_version}"

Based on past review comments that suggested validating file contents to avoid silent failures.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99c919c and e6fddbc.

📒 Files selected for processing (9)
  • .github/actions/clp-build-runtime-image/action.yaml (1 hunks)
  • components/clp-package-utils/clp_package_utils/general.py (1 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/compress.py (1 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/decompress.py (2 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/search.py (1 hunks)
  • components/clp-package-utils/clp_package_utils/scripts/start_clp.py (7 hunks)
  • components/clp-py-utils/clp_py_utils/clp_config.py (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
🧬 Code graph analysis (2)
components/clp-package-utils/clp_package_utils/general.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
  • load_container_image_ref (1015-1026)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
  • make_config_path_absolute (42-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: build-macos (macos-14, true)
  • GitHub Check: musllinux_1_2-x86_64-deps-image
  • GitHub Check: ubuntu-jammy-deps-image
  • GitHub Check: manylinux_2_28-x86_64-deps-image
  • GitHub Check: centos-stream-9-deps-image
  • GitHub Check: rust-checks (ubuntu-24.04)
  • GitHub Check: rust-checks (ubuntu-22.04)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (12)
components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)

232-232: LGTM! Container image reference updated consistently.

The change from execution_container to container_image_ref aligns with the PR's migration to the clp-package image.

components/clp-package-utils/clp_package_utils/scripts/decompress.py (2)

135-135: LGTM! Container image reference updated consistently.

The change from execution_container to container_image_ref aligns with the PR's migration to the clp-package image.


214-214: LGTM! Container image reference updated consistently.

The change from execution_container to container_image_ref aligns with the PR's migration to the clp-package image.

components/clp-package-utils/clp_package_utils/scripts/search.py (1)

137-137: LGTM! Container image reference updated consistently.

The change from execution_container to container_image_ref aligns with the PR's migration to the clp-package image.

components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)

152-152: LGTM! Container image reference updated consistently.

The change from execution_container to container_image_ref aligns with the PR's migration to the clp-package image.

components/clp-package-utils/clp_package_utils/scripts/compress.py (1)

252-252: LGTM!

The change from clp_config.execution_container to clp_config.container_image_ref is consistent with the broader refactoring across the codebase.

components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)

264-264: LGTM!

All seven occurrences consistently replace clp_config.execution_container with clp_config.container_image_ref across different component startup functions. The changes are uniform and align with the PR's objective.

Also applies to: 309-309, 630-630, 765-765, 972-972, 1026-1026, 1104-1104

components/clp-py-utils/clp_py_utils/clp_config.py (4)

90-92: LGTM!

The new constants CLP_PACKAGE_CONTAINER_IMAGE_ID_PATH and CLP_VERSION_FILE_PATH are clearly named and follow the established naming convention. Based on past review comments, the naming has been appropriately refined.


878-878: LGTM!

The container_image_ref field is well-defined with an appropriate optional type, allowing it to be populated during configuration loading via load_container_image_ref().


904-907: LGTM!

The private attributes _container_image_id_path and _version_file_path are properly defined with PrivateAttr and appropriate defaults. The naming is consistent with the module-level constants. Based on learnings.


924-927: LGTM!

The path resolution for _container_image_id_path and _version_file_path correctly uses make_config_path_absolute() and follows the established pattern for other configuration paths in this method.

.github/actions/clp-build-runtime-image/action.yaml (1)

55-56: No references to clp-build-runtime-image action found; hard-coded Dockerfile path has no downstream impact.


clp_config.make_config_paths_absolute(clp_home)
clp_config.load_execution_container_name()
clp_config.load_container_image_ref()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

LGTM! Central configuration loading updated to new container image reference mechanism.

The change from load_execution_container_name() to load_container_image_ref() centralizes the container image reference loading. Based on the related code snippet, this method reads from either clp-package-image.id or falls back to the VERSION file to construct the image reference.

Verify that load_container_image_ref() handles missing files appropriately:


🏁 Script executed:

#!/bin/bash
# Description: Verify error handling in load_container_image_ref for missing image ID and VERSION files

# Search for the implementation of load_container_image_ref
ast-grep --pattern $'def load_container_image_ref($$$):
  $$$'

# Check for file existence checks and error handling
rg -nP -A10 'def load_container_image_ref' --type=py

Length of output: 2391


Add error handling for missing VERSION file in load_container_image_ref
The method guards against a missing container-image ID file but unconditionally opens the version file, which will raise a FileNotFoundError if the VERSION file is absent. Add an existence check or wrap the version-file open in exception handling to provide a clear error message or fallback.
components/clp-py-utils/clp_py_utils/clp_config.py Lines 1019–1025

🤖 Prompt for AI Agents
components/clp-package-utils/clp_package_utils/general.py around line 430: the
call to clp_config.load_container_image_ref() can raise a FileNotFoundError
because the function opens the VERSION file unconditionally; update
load_container_image_ref (components/clp-py-utils/clp_py_utils/clp_config.py
lines ~1019–1025) to check for the VERSION file's existence before opening it or
wrap the open call in a try/except that catches FileNotFoundError and either
returns a sensible fallback (e.g., None or a default version) or raises a new
exception with a clear, descriptive error message that includes the path checked
so callers can handle missing VERSION files gracefully.

image_name="clp-${{inputs.image_type}}"
fi
dockerfile_path="$base_path/clp-package/Dockerfile"
image_name="clp-${{inputs.image_type}}"
Copy link
Member

Choose a reason for hiding this comment

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

We can now get rid of inputs.image_type, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

i hesitated, thinking we might still need inputs.image_type to build a different minimal worker image for multi-node deployments. since that isn't coming any time soon and inputs.image_type is unused for now, i agree we should remove

@junhaoliao
Copy link
Member Author

@kirkrodrigues shall we remove the os-release copy from the package, or do you think that's worth keeping?

@kirkrodrigues
Copy link
Member

@kirkrodrigues shall we remove the os-release copy from the package, or do you think that's worth keeping?

I think we can remove it.

- "{{.G_CORE_COMPONENT_BUILD_DIR}}/indexer"
- "{{.G_CORE_COMPONENT_BUILD_DIR}}/reducer-server"
- "{{.TASKFILE}}"
- "/etc/os-release"
Copy link
Member Author

@junhaoliao junhaoliao Oct 1, 2025

Choose a reason for hiding this comment

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

A different distro might cause the output of npm ci to be different though most of the time i don't think it would cause issues in the Node.js dependencies

@junhaoliao junhaoliao requested review from kirkrodrigues and removed request for kirkrodrigues October 1, 2025 22:39
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

For the PR title, how about:

feat(clp-package): Replace package execution container with `clp-package`; Change default taskfile task to `docker-images:package`.

@junhaoliao
Copy link
Member Author

This PR changes the default task to be docker-images:package, which requires the docker binary (and also the builds plugin) to be available in the build environment. For those who build inside containers, we may need to create a new build container image or add Docker to the existing clp-core-dependencies-x86-ubuntu-jammy image.

@coderabbitai create an issue to track the request

@junhaoliao junhaoliao changed the title feat(clp-package): Replace package execution container with clp-package; Change task default task to docker-images:package. feat(clp-package): Replace package execution container with clp-package; Change default taskfile task to docker-images:package. Oct 1, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track the Docker requirement for container-based builds

gh issue create \
  --repo y-scope/clp \
  --title "Add Docker binary and builds plugin to build container image for new default task" \
  --body "## Context

After PR #1357, the default task has been changed to \`docker-images:package\`, which requires the \`docker\` binary and the builds plugin to be available in the build environment.

## Problem

For users who build inside containers, these dependencies may not be available. We need to either:
- Create a new build container image with Docker support, or
- Add Docker to the existing \`clp-core-dependencies-x86-ubuntu-jammy\` image

## References

- PR: https://github.com/y-scope/clp/pull/1357
- Comment: https://github.com/y-scope/clp/pull/1357#discussion_r1886488792

Requested by: @junhaoliao" \
  --assignee junhaoliao

Length of output: 42


@junhaoliao: I've created issue #1367 to track the request for adding Docker binary and builds plugin support to the build container image. The issue has been assigned to you and includes references back to this PR and comment.


🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.436Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.

@junhaoliao junhaoliao merged commit aba7ec4 into y-scope:main Oct 2, 2025
32 checks passed
@junhaoliao junhaoliao deleted the select-container branch October 2, 2025 01:04
# Accept configured value for debug purposes
return

os_release = dotenv_values(self._os_release_file_path)
Copy link
Member Author

Choose a reason for hiding this comment

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

we should have removed the dotenv_values import and related dependency installation in this PR. @coderabbitai file an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if dotenv_values is used anywhere else in the codebase
rg -n 'dotenv_values' --type=py

# Check for python-dotenv dependency in requirements files
fd -e txt -e in -e toml -e cfg --exec grep -Hn 'python-dotenv' {}

# Also check for pyproject.toml dependencies
fd pyproject.toml --exec grep -A 20 'dependencies' {}

Length of output: 1741


🏁 Script executed:

#!/bin/bash
# Check for dotenv usage specifically in the clp-py-utils component
rg -n 'dotenv' components/clp-py-utils/ --type=py

Length of output: 85


🏁 Script executed:

#!/bin/bash
# File an issue to track the removal of unused dotenv_values import and dependency
gh issue create \
  --title "Remove unused python-dotenv dependency from clp-py-utils" \
  --body "## Context
After PR #1357, the \`dotenv_values\` import is no longer used in \`components/clp-py-utils/clp_py_utils/clp_config.py\`.

## Tasks
- [ ] Remove the unused import: \`from dotenv import dotenv_values\` from \`components/clp-py-utils/clp_py_utils/clp_config.py\` (line 6)
- [ ] Remove the \`python-dotenv\` dependency from \`components/clp-py-utils/pyproject.toml\`

## Notes
- The \`python-dotenv\` dependency should remain in \`tools/deployment/presto-clp/scripts/requirements.txt\` as it's still used by \`tools/deployment/presto-clp/scripts/init.py\`

## References
- PR: https://github.com/y-scope/clp/pull/1357
- Comment: https://github.com/y-scope/clp/pull/1357#discussion_r2442446179
- Requested by: @junhaoliao" \
  --assignee junhaoliao

Length of output: 42


@junhaoliao, I've filed issue #1443 to track the removal of the unused python-dotenv dependency and import.

Summary:

  • The dotenv_values import in components/clp-py-utils/clp_py_utils/clp_config.py (line 6) is no longer used after this PR
  • The python-dotenv dependency in components/clp-py-utils/pyproject.toml can be removed
  • The dependency should remain in tools/deployment/presto-clp/scripts/requirements.txt since it's still actively used by tools/deployment/presto-clp/scripts/init.py

The issue has been assigned to you with the tasks and context needed for the cleanup.


🧬 Code Graph Analysis Results

components/clp-py-utils/clp_py_utils/clp_config.py

[Snippet 1]

  • Filename: components/clp-py-utils/clp_py_utils/clp_config.py

  • Line(s): 6-6

  • Content:
    from dotenv import dotenv_values

  • Rationale: This import is directly related to the user comment about removing dotenv_values usage and the related dependency installation.

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