feat(integration-tests): Record required components in `PackageConfig` based on the CLP package config. by quinntaylormitchell · Pull Request #1658 · y-scope/clp

quinntaylormitchell · 2025-11-25T18:13:27Z

Description

This PR introduces the component_list data member to PackageConfig data class. The data member is used for tracking which containerized components a given CLP package configuration needs to run.

PR dependencies

This PR depends on the following being merged:

feat(integration-tests): Add fixtures and tests for starting and stopping CLP packages. #1437

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

Ran the following command from clp/integration-tests:

uv run pytest -m 'package'

Both the clp-json and clp-text tests passed.

Summary by CodeRabbit

Chores
- Component selection now adapts dynamically to deployment mode and retention/archive settings for more efficient setups.
New Features
- Package metadata includes an explicit list of required components.
- Start-up logs now show the package mode and its required component list for easier verification and debugging.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…ig file; address comments.

…er-level clp-config.yml

…-scope#1549 is merged.

…E_CONFIGS other than clp-text and clp-json

…nto one class.

…fter y-scope#1549).

coderabbitai · 2025-11-25T18:13:34Z

Walkthrough

A utility to compute required CLP components from a CLP config was added; PackageConfig now includes a component_list field. Fixtures compute and pass that list to PackageConfig, and a test logs the package's required component list at start.

Changes

Cohort / File(s)	Summary
Component list utility `integration-tests/tests/utils/clp_mode_utils.py`	Added helper `_to_docker_compose_service_name`, component name constants and groupings (`CLP_BASE_COMPONENTS`, `CLP_QUERY_COMPONENTS`, `CLP_API_SERVER_COMPONENT`, `CLP_GARBAGE_COLLECTOR_COMPONENT`, `CLP_MCP_SERVER_COMPONENT`) and new public function `get_required_component_list(config)` that builds the runtime CLP component list based on deployment type and optional settings.
Configuration data model `integration-tests/tests/utils/config.py`	Added new public field `component_list: list[str]` to `PackageConfig`.
Fixture integration `integration-tests/tests/fixtures/package_config.py`	Imported and used `get_required_component_list` to compute `required_components` and pass `component_list=required_components` when creating `PackageConfig`.
Test logging `integration-tests/tests/test_package_start.py`	Added logging to emit `fixt_package_instance.package_config.component_list` (newline-joined) along with the package mode on successful start.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify conditional logic in get_required_component_list (deployment type, retention checks, optional servers).
Confirm component_list is correctly propagated through fixtures and accessible where logged.
Check naming conversion in _to_docker_compose_service_name matches actual Docker Compose service names used in tests.

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.
Title check	✅ Passed	The title accurately describes the main change: adding a new data member `component_list` to `PackageConfig` to track required CLP components based on configuration.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Ruff (0.14.7)

integration-tests/tests/test_package_start.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load extended configuration /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml (/integration-tests/pyproject.toml extends /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml)
�[1mCause:�[0m Failed to read /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml
�[1mCause:�[0m No such file or directory (os error 2)

integration-tests/tests/utils/clp_mode_utils.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load extended configuration /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml (/integration-tests/pyproject.toml extends /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml)
�[1mCause:�[0m Failed to read /tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml
�[1mCause:�[0m No such file or directory (os error 2)

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 · 2025-12-03T09:15:28Z

    # Get the ClpConfig for this mode.
    clp_config_obj = get_clp_config_from_mode(mode_name)

+    # Compute the list of required components for this mode.


these comments feel a bit redundant since the method names have been pretty self-explanatory

Suggested change

# Get the ClpConfig for this mode.

clp_config_obj = get_clp_config_from_mode(mode_name)

# Compute the list of required components for this mode.

clp_config_obj = get_clp_config_from_mode(mode_name)

will remove.

junhaoliao · 2025-12-03T09:17:52Z

    package_config = PackageConfig(
        path_config=fixt_package_path_config,
        mode_name=mode_name,
+        component_list=required_components,


it's not immediately clear how the component_list would be used yet. just want to make they are logged before they are used for better traceability in the tests

I'll log the list of components in test_package_start.py for now.

junhaoliao · 2025-12-03T09:20:00Z

+    _to_docker_compose_service_name(WEBUI_COMPONENT_NAME),
+]
+
+CLP_QUERY_COMPONENTS = [


i believe the Presto coordinator is left out intentionally. shall we add a comment / TODO to explain why it's not listed as a component in the integration tests for now, and that we plan to add it, once we integrate the Presto Docker Compose project with the CLP Docker Compose project?

It is left out intentionally. Will add the TODO as you describe.

junhaoliao · 2025-12-03T09:20:33Z

+CLP_API_SERVER_COMPONENT = _to_docker_compose_service_name(API_SERVER_COMPONENT_NAME)
+
+CLP_GARBAGE_COLLECTOR_COMPONENT = _to_docker_compose_service_name(GARBAGE_COLLECTOR_COMPONENT_NAME)
+
+CLP_MCP_SERVER_COMPONENT = _to_docker_compose_service_name(MCP_SERVER_COMPONENT_NAME)


Suggested change

CLP_API_SERVER_COMPONENT = _to_docker_compose_service_name(API_SERVER_COMPONENT_NAME)

CLP_GARBAGE_COLLECTOR_COMPONENT = _to_docker_compose_service_name(GARBAGE_COLLECTOR_COMPONENT_NAME)

CLP_MCP_SERVER_COMPONENT = _to_docker_compose_service_name(MCP_SERVER_COMPONENT_NAME)

CLP_API_SERVER_COMPONENT = _to_docker_compose_service_name(API_SERVER_COMPONENT_NAME)

CLP_GARBAGE_COLLECTOR_COMPONENT = _to_docker_compose_service_name(GARBAGE_COLLECTOR_COMPONENT_NAME)

CLP_MCP_SERVER_COMPONENT = _to_docker_compose_service_name(MCP_SERVER_COMPONENT_NAME)

junhaoliao

for the title, how about

feat(integration-tests): Record required components in `PackageConfig` based on the CLP package config.

Bill-hbrhbr

nit picks

coderabbitai

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 9f0f4ce and bc720bc.

📒 Files selected for processing (2)

integration-tests/tests/test_package_start.py (1 hunks)
integration-tests/tests/utils/clp_mode_utils.py (3 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.

📚 Learning: 2025-11-10T05:19:56.600Z

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.

Applied to files:

integration-tests/tests/utils/clp_mode_utils.py

📚 Learning: 2025-10-27T07:07:37.901Z

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.

Applied to files:

integration-tests/tests/utils/clp_mode_utils.py

🧬 Code graph analysis (2)

integration-tests/tests/test_package_start.py (1)

integration-tests/tests/fixtures/package_instance.py (1)

fixt_package_instance (18-34)

integration-tests/tests/utils/clp_mode_utils.py (1)

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

ClpConfig (713-963)

DeploymentType (95-97)

Package (150-174)

QueryEngine (126-129)

get_deployment_type (915-918)

⏰ 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). (4)

GitHub Check: package-image
GitHub Check: build (ubuntu-24.04)
GitHub Check: lint-check (ubuntu-24.04)
GitHub Check: lint-check (macos-15)

🔇 Additional comments (2)

integration-tests/tests/test_package_start.py (1)

27-31: Component logging wiring looks correct and helpful for debugging.

Using package_config.component_list and logging it as a newline‑joined list under the mode name is straightforward and keeps the output readable. This should make it much easier to see which services are expected to be running for a given package mode.

integration-tests/tests/utils/clp_mode_utils.py (1)

91-117: Required component computation matches current deployment semantics.

Initialising component_list from CLP_BASE_COMPONENTS, then extending with CLP_QUERY_COMPONENTS only for DeploymentType.FULL, and conditionally adding API server, garbage collector, and MCP server based on the presence/retention settings on the config, matches the described ClpConfig.get_deployment_type() and component behaviour. The logic is straightforward, side‑effect free on the constants, and should give a stable, predictable component list for the tests to log and eventually validate against.

coderabbitai · 2025-12-03T18:32:12Z

+# TODO: This will eventually be replaced by a formalized mapping between component and service.
+def _to_docker_compose_service_name(name: str) -> str:
+    """
+    Convert a component name to a Docker Compose service name.
+
+    :param name:
+    :return: Service name with underscores replaced by hyphens
+    """
+    return name.replace("_", "-")
+
+
+# TODO: Modify these component lists when the Presto Docker Compose project is integrated with the
+# CLP Docker compose project.
+CLP_BASE_COMPONENTS = [
+    _to_docker_compose_service_name(DB_COMPONENT_NAME),
+    _to_docker_compose_service_name(QUEUE_COMPONENT_NAME),
+    _to_docker_compose_service_name(REDIS_COMPONENT_NAME),
+    _to_docker_compose_service_name(REDUCER_COMPONENT_NAME),
+    _to_docker_compose_service_name(RESULTS_CACHE_COMPONENT_NAME),
+    _to_docker_compose_service_name(COMPRESSION_SCHEDULER_COMPONENT_NAME),
+    _to_docker_compose_service_name(COMPRESSION_WORKER_COMPONENT_NAME),
+    _to_docker_compose_service_name(WEBUI_COMPONENT_NAME),
+]
+
+CLP_QUERY_COMPONENTS = [
+    _to_docker_compose_service_name(QUERY_SCHEDULER_COMPONENT_NAME),
+    _to_docker_compose_service_name(QUERY_WORKER_COMPONENT_NAME),
+]
+
+CLP_API_SERVER_COMPONENT = _to_docker_compose_service_name(API_SERVER_COMPONENT_NAME)
+CLP_GARBAGE_COLLECTOR_COMPONENT = _to_docker_compose_service_name(GARBAGE_COLLECTOR_COMPONENT_NAME)
+CLP_MCP_SERVER_COMPONENT = _to_docker_compose_service_name(MCP_SERVER_COMPONENT_NAME)
+


🧹 Nitpick | 🔵 Trivial

Component → service-name mapping and constants are clear; consider immutable sequences.

The _to_docker_compose_service_name() helper plus the centralised CLP_* component constants give a single place to reason about docker‑compose service names, which will make future config/debugging much easier. The TODO around Presto/Compose integration also documents the current omission appropriately (consistent with earlier decisions about separate Presto flows). Based on learnings, this aligns with the current deployment split.

If you want to tighten this further, you could make CLP_BASE_COMPONENTS and CLP_QUERY_COMPONENTS tuples instead of lists to emphasise that they are constants and should not be mutated, relying on list(CLP_BASE_COMPONENTS) in get_required_component_list() as you already do.

🤖 Prompt for AI Agents

In integration-tests/tests/utils/clp_mode_utils.py around lines 43 to 75, the CLP_BASE_COMPONENTS and CLP_QUERY_COMPONENTS are defined as mutable lists but intended to be constants; change both to tuples (e.g. replace [...] with (...)) so they are immutable, keep the helper _to_docker_compose_service_name usage the same, and ensure any call sites that expect a list continue to call list(CLP_BASE_COMPONENTS) or list(CLP_QUERY_COMPONENTS) as needed (no other behavior changes required).

Bill-hbrhbr

Leaving to @junhaoliao for final pass and PR title suggestion.

…` based on the CLP package config. (y-scope#1658) Co-authored-by: Bingran Hu <bingran.hu@yscope.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

quinntaylormitchell added 30 commits October 17, 2025 21:01

Test a generic clp package start up and spin down (default: clp-text).

8e75e15

Merge branch 'main' into feature branch

44ee20e

Expand to clp-text and clp-json; store JSON config pairs in temp conf…

bbe0575

…ig file; address comments.

Merge branch 'main' into feature branch

5cd47f8

Include and employ clp_py_utils.clp_config; refrain from using the us…

c5375eb

…er-level clp-config.yml

Lint.

2be837f

Refactor; move all utility functions to utils files as appropriate.

bf14b5e

Add comments.

27b7f5b

Streamline logger calls.

e041f0e

Deduplicate fixture code.

75672fc

Merge branch 'main' into feature branch

0822a79

Change method of clp_config import; temporary, will change again once y…

2df9515

…-scope#1549 is merged.

Generalize code so that it's better equipped to handle future CLP_MOD…

63f3ae4

…E_CONFIGS other than clp-text and clp-json

Merge branch 'main' into feature branch

0011447

Combine PackageConfig, PackageModeConfig, and PackageInstanceConfig i…

0ff9465

…nto one class.

Employ a 'CLPConfig'-centric approach rather than 'dict'-centric.

cdcfaee

Store list of required components for each mode

53e7343

Generalize the method by which the mode of operation is determined.

796ffe3

Move all mode-related utilities to their own file.

bae12de

Minor error.

9c9ef74

Move load_yaml_to_dict to general utilities file.

0f3dcd5

Add taskfile tasks for package tests.

f9865a8

Address rabbit comments.

865e9ed

Add mariadb_config install for linting purposes (will be unecessary a…

09d5200

…fter y-scope#1549).

Lint YAML.

5fffc32

Merge branch 'main' into feature branch

571f313

Adapt code after y-scope#1549 merge.

e348be0

Merge branch 'main' into feature branch

30f186f

Merge branch 'main' into feature branch

e5da7e9

Merge branch 'main' into feature branch

292ec70

quinntaylormitchell changed the title ~~feat(integration-tests): Add component_list data member to PackageConfig data class.~~ feat(integration-tests): Record required components for the clp package in component_list data member within PackageConfig. Nov 25, 2025

This was referenced Nov 25, 2025

feat(integration-tests): Test compression with the CLP package. #1651

Closed

feat(integration-tests): Test search with the CLP package. #1652

Closed

quinntaylormitchell added 3 commits December 2, 2025 20:43

Merge branch 'main' into feature branch

7b2b1eb

Accommodate 'no API server for clp-text' rule.

9680274

Remove erroneous code.

28100c9

quinntaylormitchell marked this pull request as ready for review December 2, 2025 21:28

quinntaylormitchell requested a review from a team as a code owner December 2, 2025 21:28

quinntaylormitchell requested a review from Bill-hbrhbr December 2, 2025 21:28

junhaoliao requested changes Dec 3, 2025

View reviewed changes

quinntaylormitchell changed the title ~~feat(integration-tests): Record required components for the clp package in component_list data member within PackageConfig.~~ feat(integration-tests): Record required components in PackageConfig based on the CLP package config. Dec 3, 2025

quinntaylormitchell added 2 commits December 3, 2025 15:47

Address Junhao's comments.

ce53dbd

Address Junhao's comments.

9f0f4ce

quinntaylormitchell requested a review from junhaoliao December 3, 2025 15:49

Bill-hbrhbr reviewed Dec 3, 2025

View reviewed changes

Comment thread integration-tests/tests/utils/clp_mode_utils.py Outdated

Comment thread integration-tests/tests/utils/clp_mode_utils.py Outdated

Comment thread integration-tests/tests/utils/clp_mode_utils.py Outdated

Comment thread integration-tests/tests/test_package_start.py Outdated

quinntaylormitchell added 2 commits December 3, 2025 18:26

Address Bill's comments.

b213fb3

Address Bill's comments.

bc720bc

quinntaylormitchell requested a review from Bill-hbrhbr December 3, 2025 18:28

coderabbitai Bot reviewed Dec 3, 2025

View reviewed changes

Bill-hbrhbr approved these changes Dec 3, 2025

View reviewed changes

quinntaylormitchell mentioned this pull request Dec 3, 2025

feat(integration-tests): Add port range assignment based on a configurable base port to avoid package spin-up failures. #1662

Merged

3 tasks

junhaoliao approved these changes Dec 4, 2025

View reviewed changes

Merge branch 'main' into feature branch

a72e2c8

quinntaylormitchell merged commit 762ed50 into y-scope:main Dec 4, 2025
19 checks passed

Conversation

quinntaylormitchell commented Nov 25, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR dependencies

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junhaoliao left a comment

Choose a reason for hiding this comment

Uh oh!

Bill-hbrhbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Bill-hbrhbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

quinntaylormitchell commented Nov 25, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 25, 2025 •

edited

Loading