Skip to content

feat(deployment): Add --setup-only flag to start-clp.sh to set up the package without starting components (resolves #1475). #1502

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 27, 2025
Merged

feat(deployment): Add --setup-only flag to start-clp.sh to set up the package without starting components (resolves #1475). #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
218d32e
feat(deployment): Add `--setup-only` flag to `start-clp.sh` for setup…
junhaoliao Oct 25, 2025
e2772c4
fix(deployment): Update `--setup-only` logic in `start-clp.sh` to ens…
junhaoliao Oct 25, 2025
e732f78
Merge branch 'main' into setup-only
junhaoliao Oct 26, 2025
83d2d80
Improve `--setup-only` prompt - Apply suggestions from code review
junhaoliao Oct 27, 2025
70b2b14
refactor(controller): Reorder abstract methods
junhaoliao Oct 27, 2025
3f9fcc2
Merge branch 'main' into setup-only
junhaoliao Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 75 additions & 76 deletions components/clp-package-utils/clp_package_utils/controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,22 @@ def __init__(self, clp_config: CLPConfig) -> None:
self._conf_dir = self._clp_home / "etc"

@abstractmethod
def start(self) -> None:
def set_up_env(self) -> None:
"""
Starts the components.
Sets up all components to run by preparing environment variables, directories, and
configuration files.
"""

@abstractmethod
def stop(self) -> None:
def start(self) -> None:
"""
Stops the components.
Starts the components.
"""

@abstractmethod
def _set_up_env(self) -> None:
def stop(self) -> None:
"""
Sets up all components to run by preparing environment variables, directories, and
configuration files.
Stops the components.
"""

def _set_up_env_for_database(self) -> EnvVarsDict:
Expand Down Expand Up @@ -642,75 +642,7 @@ def __init__(self, clp_config: CLPConfig, instance_id: str) -> None:
self._project_name = f"clp-package-{instance_id}"
super().__init__(clp_config)

def start(self) -> None:
"""
Starts CLP's components using Docker Compose.

:raise: Propagates `check_docker_dependencies`'s exceptions.
:raise: Propagates `subprocess.run`'s exceptions.
"""
check_docker_dependencies(
should_compose_project_be_running=False, project_name=self._project_name
)
self._set_up_env()

deployment_type = self._clp_config.get_deployment_type()
logger.info(f"Starting CLP using Docker Compose ({deployment_type} deployment)...")

cmd = ["docker", "compose", "--project-name", self._project_name]
if deployment_type == DeploymentType.BASE:
cmd += ["--file", "docker-compose.base.yaml"]
if self._clp_config.mcp_server is not None:
cmd += ["--profile", "mcp"]
cmd += ["up", "--detach", "--wait"]
subprocess.run(
cmd,
cwd=self._clp_home,
check=True,
)
logger.info("Started CLP.")

def stop(self) -> None:
"""
Stops CLP components deployed via Docker Compose.

:raise: Propagates `subprocess.run`'s exceptions.
"""
try:
check_docker_dependencies(
should_compose_project_be_running=True, project_name=self._project_name
)
except DockerComposeProjectNotRunningError:
logger.info(
"Docker Compose project '%s' is not running. Nothing to stop.",
self._project_name,
)
return
except DockerDependencyError as e:
logger.warning(
'Docker dependencies check failed: "%s". Attempting to stop CLP containers '
"anyway...",
e,
)
else:
logger.info("Stopping all CLP containers using Docker Compose...")

subprocess.run(
["docker", "compose", "--project-name", self._project_name, "down"],
cwd=self._clp_home,
check=True,
)
logger.info("Stopped CLP.")

@staticmethod
def _get_num_workers() -> int:
"""
:return: Number of worker processes to run.
"""
# This will change when we move from single to multi-container workers. See y-scope/clp#1424
return multiprocessing.cpu_count() // 2

def _set_up_env(self) -> None:
def set_up_env(self) -> None:
# Generate container-specific config.
container_clp_config = generate_docker_compose_container_config(self._clp_config)
num_workers = self._get_num_workers()
Expand Down Expand Up @@ -796,6 +728,73 @@ def _set_up_env(self) -> None:
continue
env_file.write(f"{key}={value}\n")

def start(self) -> None:
"""
Starts CLP's components using Docker Compose.

:raise: Propagates `check_docker_dependencies`'s exceptions.
:raise: Propagates `subprocess.run`'s exceptions.
"""
check_docker_dependencies(
should_compose_project_be_running=False, project_name=self._project_name
)
Comment on lines +731 to +740
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Oct 25, 2025

Choose a reason for hiding this comment

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

P1 Badge Missing environment setup before starting containers

The refactor made set_up_env() public but DockerComposeController.start() no longer calls it. The normal CLI path in start_clp.py only invokes controller.start() unless --setup-only is passed, so a default sbin/start-clp.sh run never writes the .env file or prepares per-component directories. Docker Compose will therefore launch with unset credentials and paths, causing startup to fail on a clean install. start() should still invoke set_up_env() when not in setup-only mode.

Useful? React with 👍 / 👎.

Copy link
Member Author

@junhaoliao junhaoliao Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@codex good point - i forgot to move controller.set_up_env() outside the if parsed_args.setup_only: block. Could you help me move the call so it appears right before the if parsed_args.setup_only: check?

Copy link
Member Author

@junhaoliao junhaoliao Oct 25, 2025

Choose a reason for hiding this comment

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

seems i have too much stuff running on codex at the moment. i'll do the change on my own then


deployment_type = self._clp_config.get_deployment_type()
logger.info(f"Starting CLP using Docker Compose ({deployment_type} deployment)...")
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Parameterise logs instead of f‑strings.

Satisfies Ruff G004 and avoids interpolation when log level is higher.

Apply:

-        logger.info(f"Starting CLP using Docker Compose ({deployment_type} deployment)...")
+        logger.info("Starting CLP using Docker Compose (%s deployment)...", deployment_type)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logger.info(f"Starting CLP using Docker Compose ({deployment_type} deployment)...")
logger.info("Starting CLP using Docker Compose (%s deployment)...", deployment_type)
🧰 Tools
🪛 Ruff (0.14.1)

743-743: Logging statement uses f-string

(G004)

🤖 Prompt for AI Agents 
components/clp-package-utils/clp_package_utils/controller.py around line 743:
the logger call uses an f-string which violates Ruff G004 and performs
interpolation even when the log level is higher; change it to a parameterised
log call by passing the format string and deployment_type as an argument (e.g.
logger.info("Starting CLP using Docker Compose (%s deployment)...",
deployment_type)) so interpolation is deferred to the logging framework.


cmd = ["docker", "compose", "--project-name", self._project_name]
if deployment_type == DeploymentType.BASE:
cmd += ["--file", "docker-compose.base.yaml"]
if self._clp_config.mcp_server is not None:
cmd += ["--profile", "mcp"]
cmd += ["up", "--detach", "--wait"]
subprocess.run(
cmd,
cwd=self._clp_home,
check=True,
)
logger.info("Started CLP.")

def stop(self) -> None:
"""
Stops CLP components deployed via Docker Compose.

:raise: Propagates `subprocess.run`'s exceptions.
"""
try:
check_docker_dependencies(
should_compose_project_be_running=True, project_name=self._project_name
)
except DockerComposeProjectNotRunningError:
logger.info(
"Docker Compose project '%s' is not running. Nothing to stop.",
self._project_name,
)
return
except DockerDependencyError as e:
logger.warning(
'Docker dependencies check failed: "%s". Attempting to stop CLP containers '
"anyway...",
e,
)
else:
logger.info("Stopping all CLP containers using Docker Compose...")

subprocess.run(
["docker", "compose", "--project-name", self._project_name, "down"],
cwd=self._clp_home,
check=True,
)
logger.info("Stopped CLP.")

@staticmethod
def _get_num_workers() -> int:
"""
:return: Number of worker processes to run.
"""
# This will change when we move from single to multi-container workers. See y-scope/clp#1424
return multiprocessing.cpu_count() // 2

Comment on lines +791 to +797
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Ensure at least one worker on single‑CPU hosts.

multiprocessing.cpu_count() // 2 can be 0. Floor to 1.

Apply:

-        return multiprocessing.cpu_count() // 2
+        return max(1, multiprocessing.cpu_count() // 2)
🤖 Prompt for AI Agents 
In components/clp-package-utils/clp_package_utils/controller.py around lines 791
to 797, the function currently returns multiprocessing.cpu_count() // 2 which
can evaluate to 0 on single-CPU hosts; change the return to ensure at least one
worker (e.g. return max(1, multiprocessing.cpu_count() // 2)) so the function
never returns 0 and update the docstring if needed to reflect the minimum of 1
worker.

Copy link
Member Author

@junhaoliao junhaoliao Oct 27, 2025

Choose a reason for hiding this comment

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

i think you're right. let's create an issue and fix in another PR

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

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

@junhaoliao, would you like me to create a GitHub issue to track this fix? I can include the relevant context about ensuring at least one worker on single-CPU hosts.

Copy link
Member Author

@junhaoliao junhaoliao Oct 27, 2025

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 27, 2025

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
# Create a GitHub issue to track the worker count fix

gh issue create \
  --title "Fix worker count calculation to return at least one worker on single-CPU hosts" \
  --body "## Problem

The \`_get_num_workers()\` method in \`components/clp-package-utils/clp_package_utils/controller.py\` currently returns \`multiprocessing.cpu_count() // 2\`, which evaluates to 0 on single-CPU hosts. This would prevent workers from starting properly.

## Location

\`components/clp-package-utils/clp_package_utils/controller.py\`, lines 791-797 (as of PR #1502)

## Impact

On systems with a single CPU core, no workers would be started, causing CLP components to fail.

## Solution

Ensure the method returns at least 1 worker by flooring the result to a minimum of 1.

## References

- Original discussion: https://github.com/y-scope/clp/pull/1502#discussion_r2462785178
- PR #1502: https://github.com/y-scope/clp/pull/1502
- Requested by: @junhaoliao" \
  --assignee junhaoliao

Length of output: 42

@junhaoliao, I've created issue #1509 to track this fix. The issue includes:

  • Description of the problem with _get_num_workers() returning 0 on single-CPU hosts
  • Location in the codebase
  • Impact on CLP components
  • Suggested solution
  • Backlinks to this PR and comment discussion
  • Assigned to you
🧬 Code Graph Analysis Results

[Relevant code snippets that could help understand the file and user comment]

  • File: components/clp-package-utils/clp_package_utils/general.py
    • Snippet: Docker-related dependency checks and errors
      • Lines 168-191
      def check_docker_dependencies(should_compose_project_be_running: bool, project_name: str):
    """
    Checks if Docker and Docker Compose are installed, and whether a Docker Compose project is
    running.

    :param should_compose_project_be_running:
    :param project_name: The Docker Compose project name to check.
    :raise DockerNotAvailableError: If any Docker dependency is not installed.
    :raise DockerComposeProjectNotRunningError: If the project isn't running when it should be.
    :raise DockerComposeProjectAlreadyRunningError: If the project is running when it shouldn't be.
    """
    try:
        subprocess.check_output(
            ["docker", "--version"],
            stderr=subprocess.STDOUT,
        )
    except subprocess.CalledProcessError as e:
        raise DockerNotAvailableError("docker is not installed or available on the path", e) from e

    is_running = _is_docker_compose_project_running(project_name)
    if should_compose_project_be_running and not is_running:
        raise DockerComposeProjectNotRunningError(project_name)
    if not should_compose_project_be_running and is_running:
        raise DockerComposeProjectAlreadyRunningError(project_name)
    • Snippet: Docker dependency error classes
      • Lines 78-82
      class DockerComposeProjectNotRunningError(DockerDependencyError):
    """Raised when a Docker Compose project is not running but should be."""

    def __init__(self, project_name: str) -> None:
        super().__init__(errno.ESRCH, f"Docker Compose project '{project_name}' is not running.")
      • Lines 53-54
      class DockerDependencyError(OSError):
    """Base class for errors related to Docker dependencies."""
    • Snippet: Dumping shared container config
      • Lines 361-369
      def dump_shared_container_config(container_clp_config: CLPConfig, clp_config: CLPConfig):
    """
    Dumps the given container config to `CLP_SHARED_CONFIG_FILENAME` in the logs directory, so that
    it's accessible in the container.

    :param container_clp_config:
    :param clp_config:
    """
    dump_container_config(container_clp_config, clp_config, CLP_SHARED_CONFIG_FILENAME)
    • Snippet: Generate Docker Compose container config
      • Lines 312-322
      def generate_docker_compose_container_config(clp_config: CLPConfig) -> CLPConfig:
    """
    Copies the given config and transforms mount paths and hosts for Docker Compose.

    :param clp_config:
    :return: The container config.
    """
    container_clp_config = clp_config.model_copy(deep=True)
    container_clp_config.transform_for_container()

    return container_clp_config
    • Snippet: Determine CLP_HOME
      • Lines 141-157
      def get_clp_home():
    # Determine CLP_HOME from an environment variable or this script's path
    clp_home = None
    if "CLP_HOME" in os.environ:
        clp_home = pathlib.Path(os.environ["CLP_HOME"])
    else:
        for path in pathlib.Path(__file__).resolve().parents:
            if "lib" == path.name:
                clp_home = path.parent
                break

    if clp_home is None:
        raise ValueError("CLP_HOME is not set and could not be determined automatically.")
    elif not clp_home.exists():
        raise ValueError("CLP_HOME set to nonexistent path.")

    return clp_home.resolve()
    • Snippet: Retention check utility
      • Lines 668-675
      def is_retention_period_configured(clp_config: CLPConfig) -> bool:
    if clp_config.archive_output.retention_period is not None:
        return True

    if clp_config.results_cache.retention_period is not None:
        return True

    return False
    • Snippet: Validation for DB config
      • Lines 498-509
      def validate_db_config(
    clp_config: CLPConfig,
    component_config: pathlib.Path,
    data_dir: pathlib.Path,
    logs_dir: pathlib.Path,
):
    if not component_config.exists():
        raise ValueError(f"{DB_COMPONENT_NAME} configuration file missing: '{component_config}'.")
    _validate_data_directory(data_dir, DB_COMPONENT_NAME)
    _validate_log_directory(logs_dir, DB_COMPONENT_NAME)

    validate_port(f"{DB_COMPONENT_NAME}.port", clp_config.database.host, clp_config.database.port)
    • Snippet: Validation for MCP server config
      • Lines 589-594
      def validate_mcp_server_config(clp_config: CLPConfig, logs_dir: pathlib.Path):
    _validate_log_directory(logs_dir, MCP_SERVER_COMPONENT_NAME)

    validate_port(
        f"{MCP_SERVER_COMPONENT_NAME}.port", clp_config.mcp_server.host, clp_config.mcp_server.port
    )
    • Snippet: Validation for Redis config
      • Lines 518-531
      def validate_redis_config(
    clp_config: CLPConfig,
    component_config: pathlib.Path,
    data_dir: pathlib.Path,
    logs_dir: pathlib.Path,
):
    if not component_config.exists():
        raise ValueError(
            f"{REDIS_COMPONENT_NAME} configuration file missing: '{component_config}'."
        )
    _validate_data_directory(data_dir, REDIS_COMPONENT_NAME)
    _validate_log_directory(logs_dir, REDIS_COMPONENT_NAME)

    validate_port(f"{REDIS_COMPONENT_NAME}.port", clp_config.redis.host, clp_config.redis.port)
    • Snippet: Validation for Queue config
      • Lines 512-515
      def validate_queue_config(clp_config: CLPConfig, logs_dir: pathlib.Path):
    _validate_log_directory(logs_dir, QUEUE_COMPONENT_NAME)

    validate_port(f"{QUEUE_COMPONENT_NAME}.port", clp_config.queue.host, clp_config.queue.port)
    • Snippet: Validation for Results Cache (Mongo) config
      • Lines 545-562
      def validate_results_cache_config(
    clp_config: CLPConfig,
    component_config: pathlib.Path,
    data_dir: pathlib.Path,
    logs_dir: pathlib.Path,
):
    if not component_config.exists():
        raise ValueError(
            f"{RESULTS_CACHE_COMPONENT_NAME} configuration file missing: '{component_config}'."
        )
    _validate_data_directory(data_dir, RESULTS_CACHE_COMPONENT_NAME)
    _validate_log_directory(logs_dir, RESULTS_CACHE_COMPONENT_NAME)

    validate_port(
        f"{RESULTS_CACHE_COMPONENT_NAME}.port",
        clp_config.results_cache.host,
        clp_config.results_cache.port,
    )
    • Snippet: Validation for WebUI config
      • Lines 577-586
      def validate_webui_config(
    clp_config: CLPConfig,
    client_settings_json_path: pathlib.Path,
    server_settings_json_path: pathlib.Path,
):
    for path in [client_settings_json_path, server_settings_json_path]:
        if not path.exists():
            raise ValueError(f"{WEBUI_COMPONENT_NAME} {path} is not a valid path to settings.json")

    validate_port(f"{WEBUI_COMPONENT_NAME}.port", clp_config.webui.host, clp_config.webui.port)
    • Snippet: Deployment type getters (from CLP config utils)
      • Lines 770-774
      def get_deployment_type(self) -> DeploymentType:
    if QueryEngine.PRESTO == self.package.query_engine:
        return DeploymentType.BASE
    else:
        return DeploymentType.FULL


def get_or_create_instance_id(clp_config: CLPConfig) -> str:
"""
Expand Down
11 changes: 11 additions & 0 deletions components/clp-package-utils/clp_package_utils/scripts/start_clp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ def main(argv):
action="store_true",
help="Enable debug logging.",
)
args_parser.add_argument(
"--setup-only",
action="store_true",
help="Validate configuration and prepare directories without starting services.",
)

parsed_args = args_parser.parse_args(argv[1:])

Expand Down Expand Up @@ -79,6 +84,12 @@ def main(argv):
try:
instance_id = get_or_create_instance_id(clp_config)
controller = DockerComposeController(clp_config, instance_id)
controller.set_up_env()
if parsed_args.setup_only:
logger.info(
"Completed setup. Services not started because `--setup-only` was specified."
)
return 0
controller.start()
except Exception as ex:
if type(ex) == ValueError:
Expand Down
5 changes: 5 additions & 0 deletions docs/src/user-docs/quick-start/clp-json.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ To start CLP, run:
sbin/start-clp.sh
```

:::{tip}
To validate configuration and prepare directories without launching services, add the
`--setup-only` flag (e.g., `sbin/start-clp.sh --setup-only`).
:::

:::{note}
If CLP fails to start (e.g., due to a port conflict), try adjusting the settings in
`etc/clp-config.yml` and then run the start command again.
Expand Down
5 changes: 5 additions & 0 deletions docs/src/user-docs/quick-start/clp-text.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ To start CLP, run:
sbin/start-clp.sh
```

:::{tip}
To validate configuration and prepare directories without launching services, add the
`--setup-only` flag (e.g., `sbin/start-clp.sh --setup-only`).
:::

:::{note}
If CLP fails to start (e.g., due to a port conflict), try adjusting the settings in
`etc/clp-config.yml` and then run the start command again.
Expand Down
Loading