Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
import argparse
import logging
import subprocess
import sys
from pathlib import Path
from typing import Final, List

from clp_py_utils.clp_config import (
ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME,
StorageEngine,
StorageType,
)
from clp_py_utils.s3_utils import generate_container_auth_options

from clp_package_utils.general import (
CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH,
dump_container_config,
generate_container_config,
generate_container_name,
generate_container_start_cmd,
get_clp_home,
load_config_file,
validate_and_load_db_credentials_file,
validate_dataset_name,
)

# Command/Argument Constants
LIST_COMMAND: Final[str] = "list"
DEL_COMMAND: Final[str] = "del"

logger = logging.getLogger(__file__)


def main(argv: List[str]) -> int:
clp_home = get_clp_home()
default_config_file_path = clp_home / CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH

# Top-level parser and options
args_parser = argparse.ArgumentParser(
description="Views the list of existing datasets or deletes dataset(s)."
)
args_parser.add_argument(
"--config",
"-c",
default=str(default_config_file_path),
help="CLP package configuration file.",
)
args_parser.add_argument(
"--verbose",
"-v",
action="store_true",
help="Enable debug logging.",
)

# Top-level commands
subparsers = args_parser.add_subparsers(
dest="subcommand",
required=True,
)
subparsers.add_parser(
LIST_COMMAND,
help="Lists existing datasets.",
)

# Options for delete subcommand
del_parser = subparsers.add_parser(
DEL_COMMAND,
help="Deletes dataset(s) from the database and the file system.",
)
del_parser.add_argument(
"datasets",
nargs="*",
help="dataset(s) to delete.",
)
del_parser.add_argument(
"-a",
"--all",
dest="del_all",
action="store_true",
help="Delete all existing datasets.",
)

parsed_args = args_parser.parse_args(argv[1:])
subcommand = parsed_args.subcommand
if parsed_args.verbose:
logger.setLevel(logging.DEBUG)
else:
logger.setLevel(logging.INFO)

# Validate and load config file
try:
config_file_path = Path(parsed_args.config)
clp_config = load_config_file(config_file_path, default_config_file_path, clp_home)
clp_config.validate_logs_dir()

# Validate and load necessary credentials
validate_and_load_db_credentials_file(clp_config, clp_home, False)
except:
logger.exception("Failed to load config.")
return -1
Comment thread
haiqi96 marked this conversation as resolved.
Resolved

storage_engine = clp_config.package.storage_engine
if StorageEngine.CLP_S != storage_engine:
logger.error(f"Datasets operation is not supported for storage engine: {storage_engine}.")
Comment thread
haiqi96 marked this conversation as resolved.
Resolved
return -1

# Validate input depending on subcommands
if DEL_COMMAND == subcommand:
datasets = parsed_args.datasets
del_all_flag = parsed_args.del_all
if not del_all_flag and len(datasets) == 0:
args_parser.error("No dataset name is specified for deletion.")
Comment thread
haiqi96 marked this conversation as resolved.
Resolved
if del_all_flag and len(datasets) != 0:
args_parser.error(
"The -a/--all flag cannot be used together with specified dataset names."
Comment thread
haiqi96 marked this conversation as resolved.
Resolved
)

try:
clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True)
table_prefix = clp_db_connection_params["table_prefix"]
for dataset in datasets:
validate_dataset_name(table_prefix, dataset)
except:
logger.exception("Invalid dataset name.")
return -1

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment on lines +114 to +122

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Catch the specific validation error for clearer diagnostics

Use ValueError to scope the exception and avoid hiding unrelated issues.

Apply:

-        except:
-            logger.exception("Invalid dataset name.")
+        except ValueError:
+            logger.exception("Invalid dataset name.")
             return -1
📝 Committable suggestion

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

Suggested change
try:
clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True)
table_prefix = clp_db_connection_params["table_prefix"]
for dataset in datasets:
validate_dataset_name(table_prefix, dataset)
except:
logger.exception("Invalid dataset name.")
return -1
try:
clp_db_connection_params = clp_config.database.get_clp_connection_params_and_type(True)
table_prefix = clp_db_connection_params["table_prefix"]
for dataset in datasets:
validate_dataset_name(table_prefix, dataset)
except ValueError:
logger.exception("Invalid dataset name.")
return -1
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 118 to 126, the except block catches all exceptions which hides
unrelated errors; change the broad except to catch ValueError specifically so
only dataset validation errors are handled, log the exception details with
logger.exception or logger.error including the caught exception, and keep
returning -1 for validation failures while allowing other exceptions to
propagate.

container_name = generate_container_name("dataset-manager")

container_clp_config, mounts = generate_container_config(clp_config, clp_home)
generated_config_path_on_container, generated_config_path_on_host = dump_container_config(
container_clp_config, clp_config, container_name
)

necessary_mounts = [
mounts.clp_home,
mounts.logs_dir,
]
if clp_config.archive_output.storage.type == StorageType.FS:
necessary_mounts.append(mounts.archives_output_dir)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
aws_mount, aws_env_vars = generate_container_auth_options(
clp_config, ARCHIVE_MANAGER_PSEUDO_COMPONENT_NAME
)
if aws_mount:
necessary_mounts.append(mounts.aws_config_dir)

container_start_cmd = generate_container_start_cmd(
container_name, necessary_mounts, clp_config.execution_container
)

if len(aws_env_vars) != 0:
for aws_env_var in aws_env_vars:
container_start_cmd.extend(["-e", aws_env_var])

# fmt: off
dataset_manager_cmd = [
"python3",
"-m", "clp_package_utils.scripts.native.dataset_manager",
"--config", str(generated_config_path_on_container),
]
# fmt: on

dataset_manager_cmd.append(subcommand)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
# Add subcommand-specific arguments
if DEL_COMMAND == subcommand:
if parsed_args.del_all:
dataset_manager_cmd.append("--all")
else:
dataset_manager_cmd.extend(parsed_args.datasets)
elif LIST_COMMAND != subcommand:
logger.error(f"Unsupported subcommand: `{subcommand}`.")
return -1

cmd = container_start_cmd + dataset_manager_cmd
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

proc = subprocess.run(cmd)
ret_code = proc.returncode
if 0 != ret_code:
logger.error("Dataset manager failed.")
logger.debug(f"Docker command failed: {' '.join(cmd)}")

Comment on lines +171 to +178

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Handle subprocess failures explicitly to improve diagnostics

Using check=True raises on failure and lets you log the return code consistently while preserving stdout/stderr from the child process.

Apply:

-    proc = subprocess.run(cmd)
-    ret_code = proc.returncode
-    if 0 != ret_code:
-        logger.error("Dataset manager failed.")
-        logger.debug(f"Docker command failed: {' '.join(cmd)}")
+    try:
+        proc = subprocess.run(cmd, check=True)
+        ret_code = proc.returncode
+    except subprocess.CalledProcessError as e:
+        ret_code = e.returncode
+        logger.error("Dataset manager failed.")
+        logger.debug(f"Docker command failed with rc={ret_code}: {' '.join(cmd)}")
📝 Committable suggestion

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

Suggested change
cmd = container_start_cmd + dataset_manager_cmd
proc = subprocess.run(cmd)
ret_code = proc.returncode
if 0 != ret_code:
logger.error("Dataset manager failed.")
logger.debug(f"Docker command failed: {' '.join(cmd)}")
cmd = container_start_cmd + dataset_manager_cmd
try:
proc = subprocess.run(cmd, check=True)
ret_code = proc.returncode
except subprocess.CalledProcessError as e:
ret_code = e.returncode
logger.error("Dataset manager failed.")
logger.debug(f"Docker command failed with rc={ret_code}: {' '.join(cmd)}")
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 171 to 178, the subprocess.run call does not raise on non-zero exit
and only logs a generic error; change to call subprocess.run(cmd, check=True,
capture_output=True, text=True) and wrap it in a try/except
subprocess.CalledProcessError as e: block, logging e.returncode and
e.stdout/e.stderr (or e.output) with logger.error/logger.debug so failures
include the exit code and child process output for diagnostics.

# Remove generated files
generated_config_path_on_host.unlink()

Comment on lines +179 to +181

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Guard removal of generated config file

If creation failed or the file was already removed, this will raise and hide the original outcome. Guard the cleanup.

Apply:

-    generated_config_path_on_host.unlink()
+    try:
+        generated_config_path_on_host.unlink()
+    except FileNotFoundError:
+        pass
+    except Exception:
+        logger.debug(f"Failed to remove generated config at {generated_config_path_on_host}")
📝 Committable suggestion

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

Suggested change
# Remove generated files
generated_config_path_on_host.unlink()
# Remove generated files
try:
generated_config_path_on_host.unlink()
except FileNotFoundError:
pass
except Exception:
logger.debug(f"Failed to remove generated config at {generated_config_path_on_host}")
🤖 Prompt for AI Agents
In components/clp-package-utils/clp_package_utils/scripts/dataset_manager.py
around lines 179 to 181, the call to generated_config_path_on_host.unlink() can
raise and mask earlier errors if the file was never created or already removed;
wrap the unlink in a safe guard by either checking
generated_config_path_on_host.exists() before unlinking or catching
FileNotFoundError (and only suppressing that exception) so cleanup does not
raise unexpected errors and original failures remain visible.

return ret_code


if "__main__" == __name__:
sys.exit(main(sys.argv))
Loading