refactor(clp-package): Migrate arg parsing to click in start_clp.py and stop_clp.py.#1754
Conversation
…top_clp.py; Simplify load_config_file function.
WalkthroughTwo CLI scripts were migrated from argparse to Click and updated to use typed Click parameters and centralized exception handling; Click was added as a dependency. Two modules had import statement reordering only; no functional changes elsewhere. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-10-20T20:19:40.504ZApplied to files:
📚 Learning: 2025-10-13T03:32:19.293ZApplied to files:
📚 Learning: 2025-11-10T05:19:56.600ZApplied to files:
📚 Learning: 2025-10-17T19:59:25.596ZApplied to files:
📚 Learning: 2025-10-07T07:54:32.427ZApplied to files:
📚 Learning: 2025-08-20T08:36:38.391ZApplied to files:
📚 Learning: 2025-06-18T20:48:48.990ZApplied to files:
📚 Learning: 2024-11-18T16:49:20.248ZApplied to files:
📚 Learning: 2025-08-19T14:41:28.901ZApplied to files:
📚 Learning: 2025-08-15T15:21:51.607ZApplied to files:
📚 Learning: 2025-05-06T09:48:55.408ZApplied to files:
📚 Learning: 2025-09-02T15:23:42.507ZApplied to files:
🪛 Ruff (0.14.8)components/clp-package-utils/clp_package_utils/scripts/start_clp.py47-47: Boolean-typed positional argument in function definition (FBT001) 48-48: Boolean-typed positional argument in function definition (FBT001) ⏰ 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). (5)
🔇 Additional comments (11)
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 |
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 (4)
components/clp-package-utils/clp_package_utils/general.py (1)
428-458: Consider resolvingconfig_file_pathbefore comparison.The comparison at line 445 (
config_file_path != default_config_file_path) may produce unexpected results if the caller passes a non-canonical path (e.g., containing..or symlinks), whiledefault_config_file_pathis derived fromget_clp_home()which returns a resolved path.Apply this diff to ensure consistent path comparison:
def load_config_file(config_file_path: pathlib.Path) -> ClpConfig: """ Load and validate a CLP configuration file. :param config_file_path: :return: The loaded and validated ClpConfig object. :raise ValueError: If the specified config file does not exist. """ clp_home = get_clp_home() default_config_file_path = clp_home / CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH + config_file_path = config_file_path.resolve() if config_file_path.exists():components/clp-package-utils/clp_package_utils/scripts/native/search.py (1)
234-235: Remove unused variables.After the refactor,
clp_homeanddefault_config_file_pathare no longer used in this function. Since--configis required andload_config_filenow derivesclp_homeinternally, these variables are dead code.Apply this diff to remove the unused variables:
def main(argv): - clp_home = get_clp_home() - default_config_file_path = clp_home / CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH - args_parser = argparse.ArgumentParser(description="Searches the compressed logs.")Also update the imports to remove unused symbols:
from clp_package_utils.general import ( - get_clp_home, load_config_file, )And remove the unused import:
from clp_py_utils.clp_config import ( - CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, Database, ResultsCache, )components/clp-package-utils/clp_package_utils/scripts/decompress.py (1)
42-63: Refactor: Remove unuseddefault_config_file_pathparameter.The
validate_and_load_configfunction signature still acceptsdefault_config_file_path(line 45), but this parameter is no longer used after theload_config_filesimplification. Consider updating the function signature to remove this unused parameter and updating all call sites accordingly (lines 91-93, 186-188).Apply this diff to the function signature:
def validate_and_load_config( clp_home: pathlib.Path, config_file_path: pathlib.Path, - default_config_file_path: pathlib.Path, ) -> ClpConfig | None: """ Validates and loads the config file. :param clp_home: :param config_file_path: - :param default_config_file_path: :return: The config object on success, None otherwise. """Then update the call sites at lines 91-93 and 186-188:
clp_config = validate_and_load_config( - clp_home, pathlib.Path(parsed_args.config), default_config_file_path + clp_home, pathlib.Path(parsed_args.config) )components/clp-package-utils/clp_package_utils/scripts/native/decompress.py (1)
179-199: Refactor: Remove unused parameters.The
validate_and_load_config_filefunction signature still acceptsclp_homeanddefault_config_file_path(lines 180, 182), but these parameters are no longer used after theload_config_filesimplification. Consider updating the function signature to remove these unused parameters and updating all call sites accordingly (lines 118-120, 224-226).Apply this diff to the function signature:
def validate_and_load_config_file( - clp_home: pathlib.Path, config_file_path: pathlib.Path, - default_config_file_path: pathlib.Path, ) -> ClpConfig | None: """ Validates and loads the config file. - :param clp_home: :param config_file_path: - :param default_config_file_path: :return: clp_config on success, None otherwise. """Then update the call sites at lines 118-120 and 224-226:
clp_config = validate_and_load_config_file( - clp_home, pathlib.Path(parsed_args.config), default_config_file_path + pathlib.Path(parsed_args.config) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
components/clp-package-utils/uv.lockis excluded by!**/*.lockcomponents/clp-py-utils/uv.lockis excluded by!**/*.lockcomponents/job-orchestration/uv.lockis excluded by!**/*.lockintegration-tests/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
components/clp-package-utils/clp_package_utils/general.py(2 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/compress_from_s3.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(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/compress.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/decompress.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/search.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/search.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(4 hunks)components/clp-package-utils/clp_package_utils/scripts/stop_clp.py(1 hunks)components/clp-package-utils/pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR #1152 discussion.
Learnt from: 20001020ycx
Repo: y-scope/clp PR: 1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.
📚 Learning: 2025-01-16T16:58:43.190Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/compress.pycomponents/clp-package-utils/clp_package_utils/scripts/compress_from_s3.pycomponents/clp-package-utils/clp_package_utils/scripts/decompress.pycomponents/clp-package-utils/clp_package_utils/scripts/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.pycomponents/clp-package-utils/clp_package_utils/scripts/compress.py
📚 Learning: 2025-09-28T15:00:22.170Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/compress.pycomponents/clp-package-utils/clp_package_utils/scripts/compress.py
📚 Learning: 2025-08-13T14:48:49.020Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 1144
File: components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py:106-114
Timestamp: 2025-08-13T14:48:49.020Z
Learning: For the dataset manager scripts in components/clp-package-utils/clp_package_utils/scripts/, the native script (native/dataset_manager.py) is designed to only be called through the wrapper script (dataset_manager.py), so dataset validation is only performed at the wrapper level rather than duplicating it in the native script.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
📚 Learning: 2025-06-24T08:54:14.438Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1004
File: components/clp-package-utils/clp_package_utils/scripts/native/decompress.py:139-144
Timestamp: 2025-06-24T08:54:14.438Z
Learning: In the CLP codebase, the get_orig_file_id function signature was changed after a recent merge to no longer accept a dataset parameter, making previous suggestions that reference this parameter invalid.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
📚 Learning: 2025-10-07T07:54:32.427Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Applied to files:
components/clp-package-utils/clp_package_utils/general.pycomponents/clp-package-utils/clp_package_utils/scripts/compress_from_s3.py
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/compress_from_s3.pycomponents/clp-package-utils/clp_package_utils/scripts/decompress.pycomponents/clp-package-utils/clp_package_utils/scripts/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/compress.py
📚 Learning: 2024-11-15T16:21:52.122Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/decompress.pycomponents/clp-package-utils/clp_package_utils/scripts/archive_manager.pycomponents/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/decompress.py
📚 Learning: 2025-10-20T20:19:40.504Z
Learnt from: 20001020ycx
Repo: y-scope/clp PR: 1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.pycomponents/clp-package-utils/clp_package_utils/scripts/start_clp.py
📚 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:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.pycomponents/clp-package-utils/clp_package_utils/scripts/archive_manager.py
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/archive_manager.py
🧬 Code graph analysis (11)
components/clp-package-utils/clp_package_utils/scripts/native/compress.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/search.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/native/search.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/general.py (2)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
ClpConfig(779-1060)components/clp-py-utils/clp_py_utils/core.py (1)
read_yaml_config_file(58-64)
components/clp-package-utils/clp_package_utils/scripts/compress_from_s3.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/decompress.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py (2)
components/clp-package-utils/clp_package_utils/controller.py (4)
DockerComposeController(859-1028)get_or_create_instance_id(1031-1049)stop(121-124)stop(984-1014)components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(146-162)load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
load_config_file(428-458)
⏰ 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). (5)
- GitHub Check: package-image
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (23)
components/clp-package-utils/pyproject.toml (1)
10-10: LGTM!The
click>=8.3.1dependency is correctly added in alphabetical order, consistent with the existing dependency list format.components/clp-package-utils/clp_package_utils/scripts/archive_manager.py (1)
173-182: LGTM!The call to
load_config_fileis correctly updated to use the simplified single-argument API. Theclp_homevariable is retained appropriately for use withvalidate_and_load_db_credentials_file.components/clp-package-utils/clp_package_utils/scripts/native/search.py (1)
298-306: LGTM!The call to
load_config_fileis correctly updated to use the simplified single-argument API.components/clp-package-utils/clp_package_utils/scripts/native/archive_manager.py (1)
189-197: LGTM!The call to
load_config_fileis correctly updated to use the simplified single-argument API. Theclp_homeanddefault_config_file_pathvariables are retained appropriately for computing the argparse default value.components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py (1)
99-99: LGTM!The simplified
load_config_filecall is correct and aligns with the refactored signature.components/clp-package-utils/clp_package_utils/scripts/native/compress.py (1)
359-359: LGTM!The simplified
load_config_filecall is correct and consistent with the refactored API.components/clp-package-utils/clp_package_utils/scripts/compress_from_s3.py (1)
228-228: LGTM!The simplified
load_config_filecall is correct and aligns with the refactored signature.components/clp-package-utils/clp_package_utils/scripts/native/dataset_manager.py (1)
220-220: LGTM!The simplified
load_config_filecall is correct and consistent with the refactored API.components/clp-package-utils/clp_package_utils/scripts/search.py (1)
96-96: LGTM!The simplified
load_config_filecall is correct and aligns with the refactored signature.components/clp-package-utils/clp_package_utils/scripts/compress.py (1)
181-181: LGTM!The simplified
load_config_filecall is correct and consistent with the refactored API.components/clp-package-utils/clp_package_utils/scripts/stop_clp.py (5)
1-2: LGTM!The shebang and module docstring are appropriate additions for a standalone script.
17-17: LGTM!Using
__name__for logger initialization is the standard Python best practice.
28-34: LGTM!The simplified function signature aligns with the refactored
load_config_fileAPI. Exception handling appropriately logs and exits with a non-zero status code.
36-42: LGTM!The stop logic correctly obtains the instance ID, creates the controller, and stops CLP with proper error handling.
45-46: LGTM!The entry point correctly invokes
main()without arguments, as Click handles argument parsing via decorators.components/clp-package-utils/clp_package_utils/scripts/start_clp.py (8)
1-2: LGTM!The shebang and module docstring are appropriate additions for a standalone script.
23-23: LGTM!Using
__name__for logger initialization is the standard Python best practice.
26-44: Click options are well-defined; note minor redundancy inget_clp_home()calls.The Click decorators are correctly structured. However,
get_clp_home()is called in the lambda default (line 31) and again explicitly at line 59. This is harmless since both calls should return the same value, but consider caching the result or restructuring if this becomes a concern.
56-77: LGTM!The config loading and validation block is well-structured. Using the simplified
load_config_file(config)API is consistent with the PR's refactoring goals. Exception handling appropriately logs and exits.
79-92: LGTM!Directory creation logic is correct. Using
exist_ok=Trueensures idempotency, andparents=Truehandles nested directory creation.
94-106: LGTM!The setup and start logic correctly handles the
--setup-onlyflag by exiting with code 0 after logging completion. The exception handling for startup failures is appropriate.
109-110: LGTM!The entry point correctly invokes
main()without arguments, as Click handles argument parsing via decorators.
45-54: Logger configuration may lack visible effect without a handler attached.Setting
logger.setLevel()controls which messages the logger processes, but without a handler (e.g., vialogging.basicConfig(),addHandler(),StreamHandler, orFileHandler), messages won't display. Confirm that logging is initialized elsewhere in the package (e.g., in__main__.py, package initialization, or the Docker entrypoint).
| @click.command() | ||
| @click.option( | ||
| "--config", | ||
| "-c", | ||
| type=click.Path(exists=False, path_type=pathlib.Path), | ||
| default=lambda: get_clp_home() / CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||
| help="CLP package configuration file.", | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Lambda default may produce confusing errors if get_clp_home() fails.
If CLP_HOME is not set and cannot be determined, get_clp_home() raises a ValueError during Click's option processing, resulting in a traceback rather than a user-friendly CLI error. Consider wrapping the lambda or handling this case more gracefully.
That said, this is consistent with the approach in start_clp.py and the original argparse behaviour likely had similar failure modes. If this is acceptable, no change is needed.
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/stop_clp.py around
lines 20 to 27, the Click option default calls get_clp_home() via a lambda which
can raise ValueError during option processing and produce an unfriendly
traceback; change it so the default does not call get_clp_home() at import/parse
time — e.g., set the option default to None or a sentinel, and compute the
config path inside the command body (or use a Click callback) where you can
catch ValueError and raise click.BadParameter with a clear message; ensure the
final path is a pathlib.Path and preserve the existing help text.
|
I might not be related to this PR. Just to express a concern regarding the maintenance of Click: The current MCP server uses Click, which is probably because it was used in my original MCP prototype. However, using click was purely for the sake of fast implementation. Honestly, I feel its documentation and underlying implementation are too immature, and the package may be at risk of deprecation in the future. Should we stick with our current, more mature solution? |
right, migrating to click removes a lot of boilerplate, reducing the code by nearly half
It seems the library is actively maintained, with over 200 commits since July. If I understand correctly from their docs, their implementation is essentially a wrapper over Python's official I'm proposing the switch only because it helps with code organization. If the library is that bad, we definitely want to re-consider. |
I haven't encountered practical difficulties. The docs aren't the best but it provides the minimal information needed to use tbe lib. It just seems that there aren't many open source projects using click. So I'm bit concerned about whether it's robust enough |
# Conflicts: # components/clp-package-utils/clp_package_utils/general.py # components/clp-package-utils/clp_package_utils/scripts/archive_manager.py # components/clp-package-utils/clp_package_utils/scripts/compress.py # components/clp-package-utils/clp_package_utils/scripts/compress_from_s3.py # components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py # components/clp-package-utils/clp_package_utils/scripts/decompress.py # components/clp-package-utils/clp_package_utils/scripts/search.py # components/clp-package-utils/clp_package_utils/scripts/start_clp.py # components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
load_config_file function.| from clp_py_utils.clp_config import ( | ||
| CLP_DEFAULT_CREDENTIALS_FILE_PATH, | ||
| CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||
| CLP_DEFAULT_CREDENTIALS_FILE_PATH, |
There was a problem hiding this comment.
as a result of running task lint:fix-py
| ClpConfig, | ||
| ClpDbUserType, | ||
| ClpDbNameType, | ||
| ClpDbUserType, |
There was a problem hiding this comment.
as a result of running task lint:fix-py
Discussed offline - it has been shown that |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/general.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/native/decompress.py(1 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(4 hunks)components/clp-package-utils/clp_package_utils/scripts/stop_clp.py(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
📚 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:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
📚 Learning: 2025-10-20T20:19:40.504Z
Learnt from: 20001020ycx
Repo: y-scope/clp PR: 1436
File: components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py:22-27
Timestamp: 2025-10-20T20:19:40.504Z
Learning: In the clp-mcp-server project (components/clp-mcp-server/clp_mcp_server/clp_mcp_server.py), the --config Click option should use exists=True to validate the configuration file path at option processing time, even though this requires the default path to exist on all machines.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.pycomponents/clp-package-utils/clp_package_utils/scripts/start_clp.py
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
📚 Learning: 2025-08-15T15:21:51.607Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 1186
File: components/clp-package-utils/clp_package_utils/scripts/archive_manager.py:40-40
Timestamp: 2025-08-15T15:21:51.607Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/archive_manager.py, the _validate_timestamps function correctly uses Optional[int] for end_ts parameter without a default value because it forces callers to explicitly pass the parsed argument value (which can be None), making the API contract clear and avoiding ambiguity about whether None came from defaults or actual parsed values.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py
📚 Learning: 2025-11-17T22:58:50.056Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
Applied to files:
components/clp-package-utils/clp_package_utils/general.pycomponents/clp-package-utils/clp_package_utils/scripts/native/decompress.py
📚 Learning: 2025-10-07T07:54:32.427Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Applied to files:
components/clp-package-utils/clp_package_utils/general.py
🧬 Code graph analysis (3)
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py (2)
components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(146-162)load_config_file(428-459)components/clp-py-utils/clp_py_utils/core.py (1)
resolve_host_path_in_container(82-103)
components/clp-package-utils/clp_package_utils/scripts/native/decompress.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
ClpDbUserType(203-208)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
components/clp-package-utils/clp_package_utils/general.py (2)
get_clp_home(146-162)load_config_file(428-459)
🪛 Ruff (0.14.8)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py
47-47: Boolean-typed positional argument in function definition
(FBT001)
48-48: Boolean-typed positional argument in function definition
(FBT001)
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (11)
components/clp-package-utils/clp_package_utils/general.py (1)
14-16: LGTM!Import reordering from linter auto-fix maintains alphabetical ordering within the import block.
components/clp-package-utils/clp_package_utils/scripts/native/decompress.py (1)
11-19: LGTM!Import reordering from linter auto-fix maintains alphabetical ordering within the import block.
components/clp-package-utils/clp_package_utils/scripts/stop_clp.py (4)
1-2: LGTM!Good addition of shebang and module docstring for consistency with the Click-based CLI pattern.
18-18: LGTM!Using
__name__for logger initialization is the idiomatic approach in Python.
21-28: Click option configuration looks good.Using
exists=Falseis appropriate here sinceload_config_filehandles the case where the default config file doesn't exist by creating a defaultClpConfig(). The lambda for dynamic default computation is a clean pattern.
47-48: LGTM!Entry point correctly calls
main()without arguments since Click handles argument parsing via decorators.components/clp-package-utils/clp_package_utils/scripts/start_clp.py (5)
1-2: LGTM!Good addition of shebang and module docstring for consistency with the Click-based CLI pattern.
23-23: LGTM!Using
__name__for logger initialization is the idiomatic approach in Python.
26-44: Click options well-defined.The option configuration is clean and consistent with
stop_clp.py. Usingexists=Falsefor the config path allowsload_config_fileto handle the missing default config gracefully. The--setup-onlyflag is a useful addition for validation workflows.Note: The static analysis FBT001 warnings about boolean-typed positional arguments are false positives here—Click passes these as keyword arguments via decorators, not as positional arguments in calling code.
99-103: LGTM!Clean handling of the
--setup-onlyflag with informative logging before exit.
110-111: LGTM!Entry point correctly calls
main()without arguments since Click handles argument parsing via decorators.
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
…nd stop_clp.py. (y-scope#1754) Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
…nd stop_clp.py. (y-scope#1754) Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
…nd stop_clp.py. (y-scope#1754) Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
Description
This PR migrates the argument parsing in
start_clp.pyandstop_clp.pyfrom argparse to click 8.3.1 and fixes all linter errors in those two files.Changes
Migrated start_clp.py and stop_clp.py to click
@click.command(),@click.option())Impact
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Refactor
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.