refactor(clp-package)!: Move query-engine config to webui (fixes #2088).#2349
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Changesquery_engine Scope Migration and Optional Query Components
Sequence Diagram(s)sequenceDiagram
participant DockerComposeController
participant ClpConfig
participant DockerCompose
DockerComposeController->>ClpConfig: read optional query service configs
DockerComposeController->>DockerCompose: set CLP_QUERY_SCHEDULER_ENABLED
DockerComposeController->>DockerCompose: set CLP_QUERY_WORKER_ENABLED
DockerComposeController->>DockerCompose: set CLP_REDUCER_ENABLED
DockerComposeController->>ClpConfig: read compression_scheduler.type
DockerComposeController->>DockerCompose: choose compose file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/package-template/src/etc/clp-config.template.text.yaml (1)
9-11: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefault template can become invalid due to engine mismatch.
With
package.storage_engine: "clp"left active andwebui.query_enginecommented out, the config can fail validation whenwebui.query_enginefalls back to its default (clp-s). Keep these defaults aligned in the template.Proposed fix
webui: -# query_engine: "clp" + query_engine: "clp"Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/package-template/src/etc/clp-config.template.text.yaml` around lines 9 - 11, The template defaults are out of sync: package.storage_engine is set to clp while webui.query_engine is left unset, which can cause validation to fail when the query engine defaults to clp-s. Update the clp-config.template.text.yaml defaults so the package and webui settings stay aligned in the template, and verify the relevant config template entries around package and webui are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/clp-package-utils/clp_package_utils/controller.py`:
- Around line 1235-1236: The topology metrics emitted by
_emit_topology_metrics() are still hardcoded to report one query worker and one
reducer even when CLP_QUERY_WORKER_ENABLED or CLP_REDUCER_ENABLED disables those
services and Compose starts zero replicas. Update _emit_topology_metrics() in
Controller to derive the query worker and reducer gauges from the same optional
config state used by the Compose setup, so the reported replica counts match
whether the corresponding config objects are present or None.
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Line 821: The new optional default for query_scheduler can still crash WebUI
setup because BaseController._set_up_env_for_webui() dereferences
query_scheduler.max_datasets_per_query without handling None. Update the WebUI
config path in BaseController._set_up_env_for_webui() to explicitly handle a
missing query_scheduler, or add validation in the clp_config QueryScheduler/CLP
config setup so WebUI cannot reach this path when the scheduler is omitted.
- Line 733: Finish migrating `BaseController._set_up_env_for_webui()` off
`package.query_engine`; it still reads the removed
`self._clp_config.package.query_engine` for client/server settings and Presto
branching, which will break env setup. Update that controller logic to
consistently use `self._clp_config.webui.query_engine` everywhere the query
engine is checked, so all branches and settings derive from the new webui config
path.
---
Outside diff comments:
In `@components/package-template/src/etc/clp-config.template.text.yaml`:
- Around line 9-11: The template defaults are out of sync:
package.storage_engine is set to clp while webui.query_engine is left unset,
which can cause validation to fail when the query engine defaults to clp-s.
Update the clp-config.template.text.yaml defaults so the package and webui
settings stay aligned in the template, and verify the relevant config template
entries around package and webui are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38239b59-8a5a-4175-916d-1c572a7f10d4
📒 Files selected for processing (10)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.pycomponents/package-template/src/etc/clp-config.template.json.yamlcomponents/package-template/src/etc/clp-config.template.text.yamldocs/src/user-docs/guides-docker-compose-deployment.mddocs/src/user-docs/guides-using-presto.mdtools/deployment/package-helm/templates/configmap.yamltools/deployment/package/docker-compose-all.yamltools/deployment/package/docker-compose-base.yamltools/deployment/package/docker-compose-spider-base.yaml
💤 Files with no reviewable changes (2)
- tools/deployment/package/docker-compose-spider-base.yaml
- tools/deployment/package/docker-compose-base.yaml
…ating-to-allow-running-multiple-query-engines-simultaneously
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/clp-package-utils/clp_package_utils/controller.py`:
- Line 526: The logger calls in controller.py use eager f-strings, which
violates the G004 logging rule; update the affected statements in the
environment setup flow to use deferred "%s" formatting with logger.info/logger
calls instead of interpolating directly. Locate the messages around the existing
logger.info uses in the controller methods handling environment setup and make
the same formatting change for each mentioned occurrence so logging arguments
are passed separately from the message string.
- Around line 752-757: The client settings assembly in controller.py
unconditionally reads self._clp_config.query_scheduler.max_datasets_per_query,
which can fail when query_scheduler is absent. Update the settings update logic
in the setup path to check for a present query_scheduler before accessing
max_datasets_per_query, and only include MaxDatasetsPerQuery when that component
exists. Use the existing controller method that builds
client_settings_json_updates as the place to add the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a92482c-d70d-4b16-bea1-6292267c496a
📒 Files selected for processing (7)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-package-utils/clp_package_utils/general.pydocs/src/dev-docs/building-package.mddocs/src/user-docs/quick-start/clp-text.mdintegration-tests/tests/package_tests/clp_json/utils/mode.pyintegration-tests/tests/package_tests/clp_text/utils/mode.pytools/deployment/package-helm/Chart.yaml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/package-template/src/etc/clp-config.template.text.yaml (1)
9-11: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the text template’s query engine explicit.
After removing the active query-engine setting, this template now falls back to
webui.query_engine = "clp-s", while Line 10 still pinspackage.storage_engine: "clp".validate_query_engine_package_compatibility()rejects that pair, so the default clp-text template no longer loads. Re-enable the activewebui.query_engine: "clp"entry or switch this template tostorage_engine: "clp-s".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/package-template/src/etc/clp-config.template.text.yaml` around lines 9 - 11, The clp text template is missing an explicit active query engine, so it falls back to a mismatched default that fails compatibility validation. In the template that defines package.storage_engine, restore the webui.query_engine setting to "clp" so validate_query_engine_package_compatibility() accepts the default combination, or change the template’s storage_engine to "clp-s" if that is the intended pairing. Use the clp-config.template.text.yaml template entry and the query-engine/package compatibility check as the reference points.components/clp-py-utils/clp_py_utils/clp_config.py (1)
1042-1059: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject
webui.query_engine: prestowhenprestois missing.The
PRESTObranch only validatesstorage_engine. Ifprestois omitted, WebUI setup later dereferencescontainer_clp_config.presto.host/port, so startup crashes instead of failing config validation.Proposed fix
elif query_engine == QueryEngine.PRESTO: + if self.presto is None: + raise ValueError( + f"'presto' must be configured when query_engine is '{QueryEngine.PRESTO}'." + ) if storage_engine != StorageEngine.CLP_S: raise ValueError( f"query_engine '{QueryEngine.PRESTO}' is only compatible with " f"storage_engine '{StorageEngine.CLP_S}'." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/clp-py-utils/clp_py_utils/clp_config.py` around lines 1042 - 1059, The validate_query_engine_package_compatibility method only checks storage_engine for QueryEngine.PRESTO, so add a validation that rejects webui.query_engine set to PRESTO when the presto config is missing. Update the PRESTO branch in clp_config.py to verify the package/webui presto settings exist before allowing startup, so later WebUI code that reads container_clp_config.presto.host and port cannot crash. Keep the fix localized to validate_query_engine_package_compatibility and reference the existing QueryEngine.PRESTO and StorageEngine.CLP_S checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Around line 1042-1059: The validate_query_engine_package_compatibility method
only checks storage_engine for QueryEngine.PRESTO, so add a validation that
rejects webui.query_engine set to PRESTO when the presto config is missing.
Update the PRESTO branch in clp_config.py to verify the package/webui presto
settings exist before allowing startup, so later WebUI code that reads
container_clp_config.presto.host and port cannot crash. Keep the fix localized
to validate_query_engine_package_compatibility and reference the existing
QueryEngine.PRESTO and StorageEngine.CLP_S checks.
In `@components/package-template/src/etc/clp-config.template.text.yaml`:
- Around line 9-11: The clp text template is missing an explicit active query
engine, so it falls back to a mismatched default that fails compatibility
validation. In the template that defines package.storage_engine, restore the
webui.query_engine setting to "clp" so
validate_query_engine_package_compatibility() accepts the default combination,
or change the template’s storage_engine to "clp-s" if that is the intended
pairing. Use the clp-config.template.text.yaml template entry and the
query-engine/package compatibility check as the reference points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77fc4a09-b93e-4c83-9ee9-2c0134e096dc
📒 Files selected for processing (7)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.pycomponents/package-template/src/etc/clp-config.template.json.yamlcomponents/package-template/src/etc/clp-config.template.text.yamltools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package/docker-compose-all.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
735-735: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the default CLP query stack internally consistent.
webui.query_enginestill defaults toclp_s, butquery_schedulernow defaults toNoneand this validator rejects that combination. Even if a caller restores onlyquery_scheduler, the controller now treatsquery_worker=Noneandreducer=Noneas disabled services, so a CLP-backed WebUI can still come up with no workers to execute queries. Please either keep the CLP query components defaulted on, or validatequery_workerandreduceralongsidequery_schedulerwheneverwebui.query_engineisclp/clp_s.Also applies to: 818-825, 1041-1049
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/clp-py-utils/clp_py_utils/clp_config.py` at line 735, The CLP WebUI defaults are inconsistent: `webui.query_engine` remains set to `CLP_S`, but the related defaults/validation in `clp_config.py` allow `query_scheduler` to be `None` while `query_worker` and `reducer` can still be treated as disabled. Update the CLP query-stack defaults or extend the existing validator so that when `webui.query_engine` is `clp`/`clp_s`, `query_scheduler`, `query_worker`, and `reducer` are validated together and stay enabled consistently. Use the `QueryEngine`/`QueryEngineStr` handling in the affected config sections to locate and adjust the defaulting logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/clp-package-utils/clp_package_utils/controller.py`:
- Around line 794-797: When query_scheduler is disabled,
_read_and_update_settings_json() currently leaves MaxDatasetsPerQuery unchanged
because client_settings_json_updates only sets it inside the non-None branch.
Update the logic around self._clp_config.query_scheduler in controller.py so
MaxDatasetsPerQuery is explicitly cleared when query_scheduler is None, either
by setting the update value to null or by removing the key from
client_settings_json_updates before settings.json is written.
---
Outside diff comments:
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Line 735: The CLP WebUI defaults are inconsistent: `webui.query_engine`
remains set to `CLP_S`, but the related defaults/validation in `clp_config.py`
allow `query_scheduler` to be `None` while `query_worker` and `reducer` can
still be treated as disabled. Update the CLP query-stack defaults or extend the
existing validator so that when `webui.query_engine` is `clp`/`clp_s`,
`query_scheduler`, `query_worker`, and `reducer` are validated together and stay
enabled consistently. Use the `QueryEngine`/`QueryEngineStr` handling in the
affected config sections to locate and adjust the defaulting logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cee5125-146f-40f1-8a88-bdeb2e910901
📒 Files selected for processing (2)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.py
Nathan903
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
818-826: 🩺 Stability & Availability | 🔴 CriticalRestore a non-null
query_schedulerdefault or stop constructingClpConfig()at class definition time.query_enginestill defaults toQueryEngine.CLP_S, soClpConfig()now fails validation becausequery_schedulerisNone. That also breaksWorkerConfig.tmp_directory = ClpConfig().tmp_directoryat import time.components/clp-py-utils/clp_py_utils/clp_config.py:818-826, 1128🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/clp-py-utils/clp_py_utils/clp_config.py` around lines 818 - 826, The ClpConfig defaults are inconsistent because query_engine still points to QueryEngine.CLP_S while query_scheduler is now None, causing ClpConfig() validation to fail and breaking WorkerConfig.tmp_directory initialization at import time. Restore a non-null default for query_scheduler in ClpConfig, or refactor the class so ClpConfig() is not instantiated during class definition in WorkerConfig; update the relevant defaults in ClpConfig and any dependent initialization paths so the config can be constructed safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Around line 818-826: The ClpConfig defaults are inconsistent because
query_engine still points to QueryEngine.CLP_S while query_scheduler is now
None, causing ClpConfig() validation to fail and breaking
WorkerConfig.tmp_directory initialization at import time. Restore a non-null
default for query_scheduler in ClpConfig, or refactor the class so ClpConfig()
is not instantiated during class definition in WorkerConfig; update the relevant
defaults in ClpConfig and any dependent initialization paths so the config can
be constructed safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78f6738b-0cd5-487a-8e7a-c92c24e0d809
📒 Files selected for processing (3)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.pycomponents/package-template/src/etc/clp-config.template.text.yaml
💤 Files with no reviewable changes (1)
- components/clp-package-utils/clp_package_utils/controller.py
…ating-to-allow-running-multiple-query-engines-simultaneously
Nathan903
force-pushed
the
2088-decouple-query_engine-from-deployment-gating-to-allow-running-multiple-query-engines-simultaneously
branch
from
276ced3 to
8eb5d65
Compare
…ating-to-allow-running-multiple-query-engines-simultaneously
…pose Presto guide
…ating-to-allow-running-multiple-query-engines-simultaneously # Conflicts: # components/clp-package-utils/clp_package_utils/controller.py # components/package-template/src/etc/clp-config.template.text.yaml # tools/deployment/package-helm/templates/configmap.yaml
junhaoliao
requested changes
Jun 29, 2026
Comment on lines
+278
to
+286
| {{- if .Values.clpConfig.query_scheduler }} | ||
| {{- with .Values.clpConfig.query_scheduler.max_datasets_per_query }} | ||
| "MaxDatasetsPerQuery": {{ . | int }}, | ||
| {{- else }} | ||
| "MaxDatasetsPerQuery": null, | ||
| {{- end }} | ||
| {{- else }} | ||
| "MaxDatasetsPerQuery": null, | ||
| {{- end }} |
Member
There was a problem hiding this comment.
the with-else syntax should already be achieving the same behaviour?
Suggested change
| {{- if .Values.clpConfig.query_scheduler }} | |
| {{- with .Values.clpConfig.query_scheduler.max_datasets_per_query }} | |
| "MaxDatasetsPerQuery": {{ . | int }}, | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} | |
| {{- with .Values.clpConfig.query_scheduler.max_datasets_per_query }} | |
| "MaxDatasetsPerQuery": {{ . | int }}, | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} |
Contributor
Author
There was a problem hiding this comment.
the if check prevents Helm from crashing from this kind of error if the config does not have query_scheduler
executing "test-chart/templates/configmap.yaml" at <.Values.clpConfig.query_scheduler.max_datasets_per_query>:
nil pointer evaluating interface {}.max_datasets_per_query
Member
There was a problem hiding this comment.
i see what you mean. let's write this instead
Suggested change
| {{- if .Values.clpConfig.query_scheduler }} | |
| {{- with .Values.clpConfig.query_scheduler.max_datasets_per_query }} | |
| "MaxDatasetsPerQuery": {{ . | int }}, | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} | |
| {{- with .Values.clpConfig.query_scheduler }} | |
| "MaxDatasetsPerQuery": {{ .max_datasets_per_query | int }}, | |
| {{- else }} | |
| "MaxDatasetsPerQuery": null, | |
| {{- end }} |
Contributor
Author
There was a problem hiding this comment.
I think this would cause issue with max_datasets_per_query: null since it would convert null to a 0. In tools\deployment\package-helm\values.yaml:200 it writes
max_datasets_per_query: 10 # Set to null to disable the limit
…ating-to-allow-running-multiple-query-engines-simultaneously
Co-authored-by: Junhao Liao <junhao@junhao.ca>
…ating-to-allow-running-multiple-query-engines-simultaneously
junhaoliao
approved these changes
Jul 2, 2026
Nathan903
deleted the
2088-decouple-query_engine-from-deployment-gating-to-allow-running-multiple-query-engines-simultaneously
branch
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Fixes #2088
query_enginefrompackagetowebui(webui.query_engine).docker-compose-base.yamlanddocker-compose-spider-base.yaml). Components likequery-scheduler,query-worker, andreducerare now dynamically turned on/off inside the primary Compose file using replica counts (deploy: replicas: 0), controlled by environment variables injected by the controller.query_scheduler,query_worker, andreducer(or set them toNone) in their configuration. The controller handles this gracefully and spins up0replicas of those services to save resources.Breaking Changes
package.query_engineconfiguration key has been removed. Users must update theirclp-config.yamland Helmvalues.yamlfiles to usewebui.query_engineinstead.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
webui.query_engine.CLP_*_ENABLED.Bug Fixes
0replicas/concurrency when query-worker/reducer services are disabled or omitted.Documentation
webui.query_engine(instead ofpackage.query_engine).