-
Notifications
You must be signed in to change notification settings - Fork 83
feat(clp-package): Expose dataset selection in package CLI scripts and web UI. #1050
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.
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
Changes from 37 commits
869b02d
6f4ba3a
74c2cd2
dfb2a93
05e14b4
7f4a220
6500555
cd9b7e0
7c9a792
94cdc38
de6f50c
42fb6f4
b15b697
926c76d
7b51f37
6f67991
b057bbe
5493843
34a8234
e3e2930
80512cd
8f7136e
17cd392
4608789
bfb2539
99e21da
f22d3bb
df60151
9622389
0c210d9
280d2a9
250dd94
aa55301
5b48c8d
46b0db2
af95be8
dde5d54
3f1c3af
06695f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,11 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| import typing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import StorageType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_DEFAULT_DATASET_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StorageEngine, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StorageType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_package_utils.general import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -18,6 +22,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| get_clp_home, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_config_file, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_and_load_db_credentials_file, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_dataset_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Command/Argument Constants | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,6 +66,12 @@ def main(argv: typing.List[str]) -> int: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| default=str(default_config_file_path), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="CLP package configuration file.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args_parser.add_argument( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--dataset", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default=None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="The dataset that the archives belong to.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Top-level commands | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| subparsers: argparse._SubParsersAction[argparse.ArgumentParser] = args_parser.add_subparsers( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -163,6 +174,20 @@ def main(argv: typing.List[str]) -> int: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"Archive deletion is not supported for storage type: {storage_type}.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| storage_engine: StorageEngine = clp_config.package.storage_engine | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dataset = parsed_args.dataset | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if StorageEngine.CLP_S == storage_engine: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dataset = CLP_DEFAULT_DATASET_NAME if dataset is None else dataset | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_dataset_name(clp_db_connection_params["table_prefix"], dataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(e) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif dataset is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"Dataset selection is not supported for storage engine: {storage_engine}.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+177
to
+189
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve exception handling and logging format. The dataset validation logic is consistent with search.py, but has the same issues:
Apply this diff to improve exception handling and logging: storage_engine: StorageEngine = clp_config.package.storage_engine
dataset = parsed_args.dataset
if StorageEngine.CLP_S == storage_engine:
dataset = CLP_DEFAULT_DATASET_NAME if dataset is None else dataset
try:
clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True)
validate_dataset_name(clp_db_connection_params["table_prefix"], dataset)
- except Exception as e:
- logger.error(e)
+ except ValueError as e:
+ logger.error("Dataset validation failed: %s", e)
return -1
elif dataset is not None:
- logger.error(f"Dataset selection is not supported for storage engine: {storage_engine}.")
+ logger.error("Dataset selection is not supported for storage engine: %s", storage_engine)
return -1📝 Committable suggestion
Suggested change
🧰 Tools🪛 Pylint (3.3.7)[warning] 184-184: Catching too general exception Exception (W0718) [warning] 188-188: Use lazy % formatting in logging functions (W1203) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Validate input depending on subcommands | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (DEL_COMMAND == subcommand and DEL_BY_FILTER_SUBCOMMAND == parsed_args.del_subcommand) or ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FIND_COMMAND == subcommand | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -196,9 +221,12 @@ def main(argv: typing.List[str]) -> int: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| "python3", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "-m", "clp_package_utils.scripts.native.archive_manager", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--config", str(generated_config_path_on_container), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| str(subcommand), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # fmt : on | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if dataset is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| archive_manager_cmd.append("--dataset") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| archive_manager_cmd.append(dataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| archive_manager_cmd.append(subcommand) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Add subcommand-specific arguments | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if DEL_COMMAND == subcommand: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import CLPConfig, StorageEngine, StorageType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_py_utils.clp_config import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_DEFAULT_DATASET_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLPConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StorageEngine, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StorageType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bill-hbrhbr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from clp_package_utils.general import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -22,6 +27,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| JobType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_config_file, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_and_load_db_credentials_file, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_dataset_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_path_could_be_dir, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -174,6 +180,24 @@ def handle_extract_stream_cmd( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| job_command = parsed_args.command | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if EXTRACT_JSON_CMD == job_command and StorageEngine.CLP_S != storage_engine: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"JSON extraction is not supported for storage engine `{storage_engine}`.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dataset = parsed_args.dataset | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if StorageEngine.CLP_S == storage_engine: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dataset = CLP_DEFAULT_DATASET_NAME if dataset is None else dataset | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validate_dataset_name(clp_db_connection_params["table_prefix"], dataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(e) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif dataset is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"Dataset selection is not supported for storage engine: {storage_engine}.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+183
to
+199
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Improve logging format and consider more specific exception handling. The dataset validation logic is correct, but there are two improvements to consider:
- if EXTRACT_JSON_CMD == job_command and StorageEngine.CLP_S != storage_engine:
- logger.error(f"JSON extraction is not supported for storage engine `{storage_engine}`.")
+ if EXTRACT_JSON_CMD == job_command and StorageEngine.CLP_S != storage_engine:
+ logger.error("JSON extraction is not supported for storage engine `%s`.", storage_engine)
return -1
- elif dataset is not None:
- logger.error(f"Dataset selection is not supported for storage engine: {storage_engine}.")
+ elif dataset is not None:
+ logger.error("Dataset selection is not supported for storage engine: %s.", storage_engine)
return -1📝 Committable suggestion
Suggested change
🧰 Tools🪛 Pylint (3.3.7)[warning] 185-185: Use lazy % formatting in logging functions (W1203) [warning] 194-194: Catching too general exception Exception (W0718) [warning] 198-198: Use lazy % formatting in logging functions (W1203) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_name = generate_container_name(str(JobType.IR_EXTRACTION)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| container_clp_config, mounts = generate_container_config(clp_config, clp_home) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| generated_config_path_on_container, generated_config_path_on_host = dump_container_config( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -185,7 +209,6 @@ def handle_extract_stream_cmd( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # fmt: off | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| job_command = parsed_args.command | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "python3", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "-m", "clp_package_utils.scripts.native.decompress", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -207,6 +230,9 @@ def handle_extract_stream_cmd( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append(str(parsed_args.target_uncompressed_size)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif EXTRACT_JSON_CMD == job_command: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append(str(parsed_args.archive_id)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if dataset is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append("--dataset") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append(dataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if parsed_args.target_chunk_size: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append("--target-chunk-size") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| extract_cmd.append(str(parsed_args.target_chunk_size)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -267,6 +293,12 @@ def main(argv): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # JSON extraction command parser | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json_extraction_parser = command_args_parser.add_parser(EXTRACT_JSON_CMD) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json_extraction_parser.add_argument("archive_id", type=str, help="Archive ID") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json_extraction_parser.add_argument( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--dataset", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default=None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="The dataset that the archives belong to.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json_extraction_parser.add_argument( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--target-chunk-size", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=int, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the parser used is
ir_extraction_parserinstead ofjson_extraction_parser, below error is raised:@coderabbitai create an issue to track this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
I can see the issue - the
--datasetargument was only added to thejson_extraction_parserbut not to their_extraction_parser, yet thehandle_extract_stream_cmdfunction tries to accessparsed_args.datasetregardless of which parser was used.Let me create an issue to track this bug:
@junhaoliao I've created an issue to track this bug. The problem is that the
--datasetargument was only added to thejson_extraction_parserbut not to their_extraction_parser, yet the code tries to accessparsed_args.datasetin both cases. This needs to be fixed by either adding the--datasetargument to the IR extraction parser or handling the missing attribute gracefully.🏁 Script executed:
Length of output: 1343
Perfect! I've successfully created issue #1200 to track this bug. The issue documents:
--datasetargument was only added to thejson_extraction_parserbut not to their_extraction_parserAttributeError: 'Namespace' object has no attribute 'dataset'when using IR extraction--datasetargument to the IR extraction parser or handle the missing attribute gracefullyThe issue has been assigned to you as requested and includes backlinks to this PR and comment for proper tracking.
🧬 Code Graph Analysis Results
✏️ Learnings added