-
Notifications
You must be signed in to change notification settings - Fork 83
feat(deployment)!: Migrate package orchestration to Docker Compose (resolves #1177); Temporarily remove support for multi-node deployments. #1178
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 all commits
934a83c
5bf23c2
83cc9d1
82abc07
0fb2294
c8ffb94
83e902a
5f2e5cd
edfa9c9
7b3965e
cd84be8
aa12bdb
5365722
21ef703
d2cdfbc
4f56709
f0db07f
655600d
5f24ce7
3df20fc
c6f81ad
db9c20f
ea03e17
60994ee
7e25d75
3e24e4e
cbb9ce1
3eb8dfe
669fa9c
acee071
3c45cfa
ed20110
4d1f5aa
3a698bb
cca84f4
537398a
42442b8
a3288ae
5b36779
93882af
a4d8e1f
63e6d72
0531a7b
f93aec7
6a20628
0935658
ad6192a
9469db4
110e9fa
045dde6
fa87d92
b6ac2c6
bfd8c7b
2b7959b
f9eb88c
0f5bd45
0656928
66dcbb2
09ef298
d3b6a67
6148a65
0ab99f0
263be6f
51d1b55
0066386
bc9ab99
8008138
34b60bd
9f63481
2344db9
2ba93d7
ce94225
5225870
cecf5b2
0c0c562
762884b
fc515db
268c144
c1d7b23
fc7aae3
cda0348
e506f6d
93034fe
7ebdbb6
ca62009
82cc48c
8f6ba1b
2aff301
5c91bf3
049e269
fc70c05
2cb1807
08472d1
09658e5
e657feb
c79259a
af96fc8
320dd11
08644f4
dd56d04
5298540
140187f
68586f0
cba044a
8a6790f
f41e22a
da6c17f
02fcbe7
6b0e1f2
ddcf760
4134b1a
f483abd
7d4f47d
064f365
7beb199
3507a1b
eb2754a
cae66df
dffc2f2
c0ef4ff
95404d0
e1db192
630329b
d9da763
c200502
5751ddf
3e9734e
ab64f4a
2d9906f
3635883
5e0d0d2
e771800
6e878cf
f2b0967
01558ba
8a3985b
c691735
787c7d7
323cb58
8b97bc2
ccb8f66
40b88a0
e58512a
d7b40b0
37a0a8a
3cdf245
95d900a
f6cbc7f
5bf0887
26dfaca
99e983f
0b028dc
f11963c
91cb9fc
1d57a89
f4254fa
932c334
a8cee64
f0e1740
6679c2d
416c31a
9ab85d2
1345de3
25c3812
3c3e23d
7c43aad
63c700a
0f7c78c
5e07756
ff4f06c
e381c3d
f426c82
73ec68f
4e707ae
6f53877
e7addd3
d4e81f3
a5a497d
0dd11ce
d27a0b1
a6478e6
7bbd134
04e56f0
0fc91d7
f307d94
e5a90dd
3015ec4
ff43c78
699979a
092a86c
8d75171
f53339f
18d1109
9d9bd46
225c6c5
127aacc
f5ea3e2
22593d8
bc69b26
1d2ab61
d4c921a
e6bb002
90ee7da
b0d0bcc
955a0ac
106fdda
7a1ea6e
a732939
c687f78
19e8a9d
c53c0f1
a2fa02c
05810be
2a02ecb
678de80
c30adae
07d1e35
0bf6b70
2986082
e499443
b788c33
6fc219c
0926180
e2daf32
736c805
4956106
91cacb0
3ad7f70
e8e4d58
c208942
6cea88b
8c5a167
f02aff4
667d753
930a2dd
340a5e7
2bc2d2b
f0fa5ae
26c3e32
164e27e
7759c5b
0fdf31c
7447fef
5227292
a8ca533
ed25fc3
4926450
127cb79
649276f
7139740
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import enum | ||
| import errno | ||
| import json | ||
| import os | ||
| import pathlib | ||
| import re | ||
|
|
@@ -15,6 +16,9 @@ | |
| CLP_DEFAULT_CREDENTIALS_FILE_PATH, | ||
| CLP_SHARED_CONFIG_FILENAME, | ||
| CLPConfig, | ||
| CONTAINER_AWS_CONFIG_DIRECTORY, | ||
| CONTAINER_CLP_HOME, | ||
| CONTAINER_INPUT_LOGS_ROOT_DIR, | ||
| DB_COMPONENT_NAME, | ||
| QueryEngine, | ||
| QUEUE_COMPONENT_NAME, | ||
|
|
@@ -42,15 +46,50 @@ | |
| EXTRACT_IR_CMD = "i" | ||
| EXTRACT_JSON_CMD = "j" | ||
|
|
||
| # Paths | ||
|
Member
Author
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. had to move this to clp_config.py to avoid circular import |
||
| CONTAINER_AWS_CONFIG_DIRECTORY = pathlib.Path("/") / ".aws" | ||
| CONTAINER_CLP_HOME = pathlib.Path("/") / "opt" / "clp" | ||
| CONTAINER_INPUT_LOGS_ROOT_DIR = pathlib.Path("/") / "mnt" / "logs" | ||
| CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH = pathlib.Path("etc") / "clp-config.yml" | ||
|
|
||
| DOCKER_MOUNT_TYPE_STRINGS = ["bind"] | ||
|
|
||
|
|
||
| class DockerDependencyError(OSError): | ||
| """Base class for errors related to Docker dependencies.""" | ||
|
|
||
|
|
||
| class DockerNotAvailableError(DockerDependencyError): | ||
| """Raised when Docker or Docker Compose is unavailable.""" | ||
|
|
||
| def __init__(self, base_message: str, process_error: subprocess.CalledProcessError) -> None: | ||
| message = base_message | ||
| output_chunks: list[str] = [] | ||
| for stream in (process_error.stdout, process_error.stderr): | ||
| if stream is None: | ||
| continue | ||
| if isinstance(stream, bytes): | ||
| text = stream.decode(errors="replace") | ||
| else: | ||
| text = str(stream) | ||
| text = text.strip() | ||
| if text: | ||
| output_chunks.append(text) | ||
| if len(output_chunks) > 0: | ||
| message = "\n".join([base_message, *output_chunks]) | ||
| super().__init__(errno.ENOENT, message) | ||
|
|
||
|
|
||
| class DockerComposeProjectNotRunningError(DockerDependencyError): | ||
| """Raised when a Docker Compose project is not running but should be.""" | ||
|
|
||
| def __init__(self, project_name: str) -> None: | ||
| super().__init__(errno.ESRCH, f"Docker Compose project '{project_name}' is not running.") | ||
|
|
||
|
|
||
| class DockerComposeProjectAlreadyRunningError(DockerDependencyError): | ||
| """Raised when a Docker Compose project is already running but should not be.""" | ||
|
|
||
| def __init__(self, project_name: str) -> None: | ||
| super().__init__( | ||
| errno.EEXIST, f"Docker Compose project '{project_name}' is already running." | ||
| ) | ||
|
|
||
|
|
||
| class DockerMountType(enum.IntEnum): | ||
| BIND = 0 | ||
|
|
||
|
|
@@ -98,13 +137,6 @@ def __init__(self, clp_home: pathlib.Path, docker_clp_home: pathlib.Path): | |
| self.generated_config_file: Optional[DockerMount] = None | ||
|
|
||
|
|
||
| def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None: | ||
| try: | ||
| validate_path_could_be_dir(data_dir) | ||
| except ValueError as ex: | ||
| raise ValueError(f"{component_name} data directory is invalid: {ex}") | ||
|
|
||
|
|
||
| def get_clp_home(): | ||
| # Determine CLP_HOME from an environment variable or this script's path | ||
| clp_home = None | ||
|
|
@@ -132,63 +164,30 @@ def generate_container_name(job_type: str) -> str: | |
| return f"clp-{job_type}-{str(uuid.uuid4())[-4:]}" | ||
|
|
||
|
|
||
| def check_dependencies(): | ||
| def check_docker_dependencies(should_compose_project_be_running: bool, project_name: str): | ||
| """ | ||
| Checks if Docker and Docker Compose are installed, and whether a Docker Compose project is | ||
| running. | ||
|
|
||
| :param should_compose_project_be_running: | ||
| :param project_name: The Docker Compose project name to check. | ||
| :raise DockerNotAvailableError: If any Docker dependency is not installed. | ||
| :raise DockerComposeProjectNotRunningError: If the project isn't running when it should be. | ||
| :raise DockerComposeProjectAlreadyRunningError: If the project is running when it shouldn't be. | ||
| """ | ||
| try: | ||
| subprocess.run( | ||
| "command -v docker", | ||
| shell=True, | ||
| stdout=subprocess.PIPE, | ||
| subprocess.check_output( | ||
| ["docker", "--version"], | ||
| stderr=subprocess.STDOUT, | ||
|
Member
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. Since we're concatenating the output in the exception anyway, do we still need to pipe stderr to stdout?
Member
Author
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. Yes, if we keep using
That we read both If we want to avoid piping stderr to stdout, an alternative is to use
Member
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. I think the return value of All that said, I think the clearest thing would be to just run |
||
| check=True, | ||
| ) | ||
| except subprocess.CalledProcessError: | ||
| raise EnvironmentError("docker is not installed or available on the path") | ||
| try: | ||
| subprocess.run( | ||
| ["docker", "ps"], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=True | ||
| ) | ||
| except subprocess.CalledProcessError: | ||
| raise EnvironmentError("docker cannot run without superuser privileges (sudo).") | ||
| except subprocess.CalledProcessError as e: | ||
| raise DockerNotAvailableError("docker is not installed or available on the path", e) from e | ||
|
|
||
|
|
||
| def is_container_running(container_name): | ||
| # fmt: off | ||
| cmd = [ | ||
| "docker", "ps", | ||
| # Only return container IDs | ||
| "--quiet", | ||
| "--filter", f"name={container_name}" | ||
| ] | ||
| # fmt: on | ||
| proc = subprocess.run(cmd, stdout=subprocess.PIPE) | ||
| if proc.stdout.decode("utf-8"): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def is_container_exited(container_name): | ||
| # fmt: off | ||
| cmd = [ | ||
| "docker", "ps", | ||
| # Only return container IDs | ||
| "--quiet", | ||
| "--filter", f"name={container_name}", | ||
| "--filter", "status=exited" | ||
| ] | ||
| # fmt: on | ||
| proc = subprocess.run(cmd, stdout=subprocess.PIPE) | ||
| if proc.stdout.decode("utf-8"): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def validate_log_directory(logs_dir: pathlib.Path, component_name: str) -> None: | ||
| try: | ||
| validate_path_could_be_dir(logs_dir) | ||
| except ValueError as ex: | ||
| raise ValueError(f"{component_name} logs directory is invalid: {ex}") | ||
| is_running = _is_docker_compose_project_running(project_name) | ||
| if should_compose_project_be_running and not is_running: | ||
| raise DockerComposeProjectNotRunningError(project_name) | ||
| if not should_compose_project_be_running and is_running: | ||
| raise DockerComposeProjectAlreadyRunningError(project_name) | ||
|
|
||
|
|
||
| def validate_port(port_name: str, hostname: str, port: int): | ||
|
|
@@ -309,6 +308,19 @@ def generate_container_config( | |
| return container_clp_config, docker_mounts | ||
|
|
||
|
|
||
| def generate_docker_compose_container_config(clp_config: CLPConfig) -> CLPConfig: | ||
|
Member
Author
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.
|
||
| """ | ||
| Copies the given config and transforms mount paths and hosts for Docker Compose. | ||
|
|
||
| :param clp_config: | ||
| :return: The container config. | ||
| """ | ||
| container_clp_config = clp_config.model_copy(deep=True) | ||
| container_clp_config.transform_for_container() | ||
|
|
||
| return container_clp_config | ||
|
|
||
|
|
||
| def generate_worker_config(clp_config: CLPConfig) -> WorkerConfig: | ||
| worker_config = WorkerConfig() | ||
| worker_config.package = clp_config.package.model_copy(deep=True) | ||
|
|
@@ -345,17 +357,15 @@ def dump_container_config( | |
| return config_file_path_on_container, config_file_path_on_host | ||
|
|
||
|
|
||
| def dump_shared_container_config( | ||
| container_clp_config: CLPConfig, clp_config: CLPConfig | ||
| ) -> Tuple[pathlib.Path, pathlib.Path]: | ||
| def dump_shared_container_config(container_clp_config: CLPConfig, clp_config: CLPConfig): | ||
| """ | ||
| Dumps the given container config to `CLP_SHARED_CONFIG_FILENAME` in the logs directory, so that | ||
| it's accessible in the container. | ||
|
|
||
| :param container_clp_config: | ||
| :param clp_config: | ||
| """ | ||
| return dump_container_config(container_clp_config, clp_config, CLP_SHARED_CONFIG_FILENAME) | ||
| dump_container_config(container_clp_config, clp_config, CLP_SHARED_CONFIG_FILENAME) | ||
|
|
||
|
|
||
| def generate_container_start_cmd( | ||
|
|
@@ -431,11 +441,6 @@ def load_config_file( | |
| validate_path_for_container_mount(clp_config.data_directory) | ||
| validate_path_for_container_mount(clp_config.logs_directory) | ||
|
|
||
| # Make data and logs directories node-specific | ||
|
Member
Author
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. BREAKING |
||
| hostname = socket.gethostname() | ||
| clp_config.data_directory /= hostname | ||
| clp_config.logs_directory /= hostname | ||
|
|
||
| return clp_config | ||
|
|
||
|
|
||
|
|
@@ -488,35 +493,44 @@ def validate_and_load_redis_credentials_file( | |
| clp_config.redis.load_credentials_from_file(clp_config.credentials_file_path) | ||
|
|
||
|
|
||
| def validate_db_config(clp_config: CLPConfig, data_dir: pathlib.Path, logs_dir: pathlib.Path): | ||
| def validate_db_config( | ||
| clp_config: CLPConfig, | ||
| component_config: pathlib.Path, | ||
| data_dir: pathlib.Path, | ||
| logs_dir: pathlib.Path, | ||
| ): | ||
| if not component_config.exists(): | ||
| raise ValueError(f"{DB_COMPONENT_NAME} configuration file missing: '{component_config}'.") | ||
|
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. Add file type validation for component config paths. All three validation functions ( Apply this diff to if not component_config.exists():
raise ValueError(f"{DB_COMPONENT_NAME} configuration file missing: '{component_config}'.")
+ if not component_config.is_file():
+ raise ValueError(f"{DB_COMPONENT_NAME} configuration at '{component_config}' is not a file.")Apply the same pattern to if not component_config.exists():
raise ValueError(
f"{REDIS_COMPONENT_NAME} configuration file missing: '{component_config}'."
)
+ if not component_config.is_file():
+ raise ValueError(
+ f"{REDIS_COMPONENT_NAME} configuration at '{component_config}' is not a file."
+ )Apply the same pattern to if not component_config.exists():
raise ValueError(
f"{RESULTS_CACHE_COMPONENT_NAME} configuration file missing: '{component_config}'."
)
+ if not component_config.is_file():
+ raise ValueError(
+ f"{RESULTS_CACHE_COMPONENT_NAME} configuration at '{component_config}' is not a file."
+ )Based on learnings Also applies to: 487-489, 514-516 🧰 Tools🪛 Ruff (0.14.0)467-467: Avoid specifying long messages outside the exception class (TRY003) 🤖 Prompt for AI Agents |
||
| _validate_data_directory(data_dir, DB_COMPONENT_NAME) | ||
| validate_log_directory(logs_dir, DB_COMPONENT_NAME) | ||
| _validate_log_directory(logs_dir, DB_COMPONENT_NAME) | ||
|
|
||
| validate_port(f"{DB_COMPONENT_NAME}.port", clp_config.database.host, clp_config.database.port) | ||
|
Comment on lines
+496
to
507
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. 🛠️ Refactor suggestion | 🟠 Major Add file type validation for component_config. The function checks Apply this diff: if not component_config.exists():
raise ValueError(f"{DB_COMPONENT_NAME} configuration file missing: '{component_config}'.")
+ if not component_config.is_file():
+ raise ValueError(f"{DB_COMPONENT_NAME} configuration at '{component_config}' is not a file.")🧰 Tools🪛 Ruff (0.14.0)464-464: Avoid specifying long messages outside the exception class (TRY003) 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def validate_queue_config(clp_config: CLPConfig, logs_dir: pathlib.Path): | ||
| validate_log_directory(logs_dir, QUEUE_COMPONENT_NAME) | ||
| _validate_log_directory(logs_dir, QUEUE_COMPONENT_NAME) | ||
|
|
||
| validate_port(f"{QUEUE_COMPONENT_NAME}.port", clp_config.queue.host, clp_config.queue.port) | ||
|
|
||
|
|
||
| def validate_redis_config( | ||
| clp_config: CLPConfig, data_dir: pathlib.Path, logs_dir: pathlib.Path, base_config: pathlib.Path | ||
| clp_config: CLPConfig, | ||
| component_config: pathlib.Path, | ||
| data_dir: pathlib.Path, | ||
| logs_dir: pathlib.Path, | ||
| ): | ||
| _validate_data_directory(data_dir, REDIS_COMPONENT_NAME) | ||
| validate_log_directory(logs_dir, REDIS_COMPONENT_NAME) | ||
|
|
||
| if not base_config.exists(): | ||
| if not component_config.exists(): | ||
| raise ValueError( | ||
| f"{REDIS_COMPONENT_NAME} base configuration at {str(base_config)} is missing." | ||
| f"{REDIS_COMPONENT_NAME} configuration file missing: '{component_config}'." | ||
| ) | ||
| _validate_data_directory(data_dir, REDIS_COMPONENT_NAME) | ||
| _validate_log_directory(logs_dir, REDIS_COMPONENT_NAME) | ||
|
|
||
| validate_port(f"{REDIS_COMPONENT_NAME}.port", clp_config.redis.host, clp_config.redis.port) | ||
|
Comment on lines
516
to
529
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. Add file type validation for component_config. Same issue as Apply this diff: if not component_config.exists():
raise ValueError(
f"{REDIS_COMPONENT_NAME} configuration file missing: '{component_config}'."
)
+ if not component_config.is_file():
+ raise ValueError(
+ f"{REDIS_COMPONENT_NAME} configuration at '{component_config}' is not a file."
+ )🧰 Tools🪛 Ruff (0.14.0)487-489: Avoid specifying long messages outside the exception class (TRY003) 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def validate_reducer_config(clp_config: CLPConfig, logs_dir: pathlib.Path, num_workers: int): | ||
| validate_log_directory(logs_dir, REDUCER_COMPONENT_NAME) | ||
| _validate_log_directory(logs_dir, REDUCER_COMPONENT_NAME) | ||
|
|
||
| for i in range(0, num_workers): | ||
| validate_port( | ||
|
|
@@ -527,10 +541,17 @@ def validate_reducer_config(clp_config: CLPConfig, logs_dir: pathlib.Path, num_w | |
|
|
||
|
|
||
| def validate_results_cache_config( | ||
| clp_config: CLPConfig, data_dir: pathlib.Path, logs_dir: pathlib.Path | ||
| clp_config: CLPConfig, | ||
| component_config: pathlib.Path, | ||
| data_dir: pathlib.Path, | ||
| logs_dir: pathlib.Path, | ||
| ): | ||
| if not component_config.exists(): | ||
| raise ValueError( | ||
| f"{RESULTS_CACHE_COMPONENT_NAME} configuration file missing: '{component_config}'." | ||
| ) | ||
| _validate_data_directory(data_dir, RESULTS_CACHE_COMPONENT_NAME) | ||
| validate_log_directory(logs_dir, RESULTS_CACHE_COMPONENT_NAME) | ||
| _validate_log_directory(logs_dir, RESULTS_CACHE_COMPONENT_NAME) | ||
|
|
||
| validate_port( | ||
| f"{RESULTS_CACHE_COMPONENT_NAME}.port", | ||
|
|
@@ -707,3 +728,44 @@ def get_celery_connection_env_vars_list(container_clp_config: CLPConfig) -> List | |
| ] | ||
|
|
||
| return env_vars | ||
|
|
||
|
|
||
| def _is_docker_compose_project_running(project_name: str) -> bool: | ||
| """ | ||
| Checks if a Docker Compose project is running. | ||
|
|
||
| :param project_name: | ||
| :return: Whether at least one instance is running. | ||
| :raise DockerNotAvailableError: If Docker Compose is not installed or fails. The error message | ||
| includes the Docker command's output when available. | ||
| """ | ||
| cmd = ["docker", "compose", "ls", "--format", "json", "--filter", f"name={project_name}"] | ||
| try: | ||
| output = subprocess.check_output(cmd, stderr=subprocess.STDOUT) | ||
| running_instances = json.loads(output) | ||
| return len(running_instances) >= 1 | ||
| except subprocess.CalledProcessError as e: | ||
| raise DockerNotAvailableError( | ||
| "Docker Compose is not installed or not functioning properly.", e | ||
| ) from e | ||
|
|
||
|
|
||
| def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None: | ||
| try: | ||
| validate_path_could_be_dir(data_dir) | ||
| except ValueError as ex: | ||
| raise ValueError(f"{component_name} data directory is invalid: {ex}") | ||
|
Comment on lines
+753
to
+757
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 | 🔵 Trivial Add exception chaining to preserve traceback. Both validation helpers should chain the original exception to preserve the full traceback for debugging. Apply this diff to def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None:
try:
validate_path_could_be_dir(data_dir)
except ValueError as ex:
- raise ValueError(f"{component_name} data directory is invalid: {ex}")
+ raise ValueError(f"{component_name} data directory is invalid: {ex}") from exApply the same pattern to def _validate_log_directory(logs_dir: pathlib.Path, component_name: str):
"""
Validates that the logs directory path for a component is valid.
:param logs_dir:
:param component_name:
:raise ValueError: If the path is invalid or can't be a directory.
"""
try:
validate_path_could_be_dir(logs_dir)
except ValueError as ex:
- raise ValueError(f"{component_name} logs directory is invalid: {ex}")
+ raise ValueError(f"{component_name} logs directory is invalid: {ex}") from exBased on learnings Also applies to: 722-733 🧰 Tools🪛 Ruff (0.14.0)719-719: Within an (B904) 719-719: Avoid specifying long messages outside the exception class (TRY003) 🤖 Prompt for AI Agents |
||
|
|
||
|
Comment on lines
+753
to
+758
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. 🛠️ Refactor suggestion | 🟠 Major Preserve traceback and add annotations in validators. Chain exceptions; add explicit return type on _validate_log_directory. def _validate_data_directory(data_dir: pathlib.Path, component_name: str) -> None:
try:
validate_path_could_be_dir(data_dir)
- except ValueError as ex:
- raise ValueError(f"{component_name} data directory is invalid: {ex}")
+ except ValueError as ex:
+ raise ValueError(f"{component_name} data directory is invalid: {ex}") from ex
@@
-def _validate_log_directory(logs_dir: pathlib.Path, component_name: str):
+def _validate_log_directory(logs_dir: pathlib.Path, component_name: str) -> None:
@@
- except ValueError as ex:
- raise ValueError(f"{component_name} logs directory is invalid: {ex}")
+ except ValueError as ex:
+ raise ValueError(f"{component_name} logs directory is invalid: {ex}") from exAlso applies to: 760-771 🧰 Tools🪛 Ruff (0.14.1)757-757: Within an (B904) 757-757: Avoid specifying long messages outside the exception class (TRY003) 🤖 Prompt for AI Agents |
||
|
|
||
| def _validate_log_directory(logs_dir: pathlib.Path, component_name: str): | ||
| """ | ||
| Validates that the logs directory path for a component is valid. | ||
|
|
||
| :param logs_dir: | ||
| :param component_name: | ||
| :raise ValueError: If the path is invalid or can't be a directory. | ||
| """ | ||
| try: | ||
| validate_path_could_be_dir(logs_dir) | ||
| except ValueError as ex: | ||
| raise ValueError(f"{component_name} logs directory is invalid: {ex}") | ||
Uh oh!
There was an error while loading. Please reload this page.