-
Notifications
You must be signed in to change notification settings - Fork 82
feat(deployment)!: Migrate package orchestration to Docker Compose (resolves #1177). #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
base: main
Are you sure you want to change the base?
Changes from all commits
fa2aff7
32ea452
217c63f
c229fe0
a850f69
1763e17
b283231
743268c
ee1b9f2
ec0cfa5
ab68c25
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
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.
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,12 +46,6 @@ | |||||||||||||||||||||||||||||||||||||
EXTRACT_IR_CMD = "i" | ||||||||||||||||||||||||||||||||||||||
EXTRACT_JSON_CMD = "j" | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
# Paths | ||||||||||||||||||||||||||||||||||||||
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"] | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -132,59 +130,58 @@ def generate_container_name(job_type: str) -> str: | |||||||||||||||||||||||||||||||||||||
return f"clp-{job_type}-{str(uuid.uuid4())[-4:]}" | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def check_dependencies(): | ||||||||||||||||||||||||||||||||||||||
def is_docker_compose_running(project_name: str) -> bool: | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
Checks if a Docker Compose project is running. | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
:param project_name: | ||||||||||||||||||||||||||||||||||||||
:return: True if at least one instance is running, else False. | ||||||||||||||||||||||||||||||||||||||
:raises EnvironmentError: If Docker Compose is not installed or fails. | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
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: | ||||||||||||||||||||||||||||||||||||||
raise EnvironmentError("docker-compose is not installed or not functioning properly.") | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def check_docker_dependencies(should_compose_run: bool, project_name: str): | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
Checks if Docker and Docker Compose are installed, and whether Docker Compose is running or not. | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
:param should_compose_run: | ||||||||||||||||||||||||||||||||||||||
:param project_name: The Docker Compose project name to check. | ||||||||||||||||||||||||||||||||||||||
:raises EnvironmentError: If any Docker dependency is not installed or Docker Compose state | ||||||||||||||||||||||||||||||||||||||
does not match expectation. | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||
subprocess.run( | ||||||||||||||||||||||||||||||||||||||
"command -v docker", | ||||||||||||||||||||||||||||||||||||||
shell=True, | ||||||||||||||||||||||||||||||||||||||
stdout=subprocess.PIPE, | ||||||||||||||||||||||||||||||||||||||
stdout=subprocess.DEVNULL, | ||||||||||||||||||||||||||||||||||||||
stderr=subprocess.STDOUT, | ||||||||||||||||||||||||||||||||||||||
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).") | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
is_running = is_docker_compose_running(project_name) | ||||||||||||||||||||||||||||||||||||||
if should_compose_run and not is_running: | ||||||||||||||||||||||||||||||||||||||
raise EnvironmentError("docker-compose is not running.") | ||||||||||||||||||||||||||||||||||||||
if not should_compose_run and is_running: | ||||||||||||||||||||||||||||||||||||||
raise EnvironmentError("docker-compose is already running.") | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+170
to
+174
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. Stop command must stay idempotent. Line 171 - is_running = is_docker_compose_running(project_name)
- if should_compose_run and not is_running:
- raise EnvironmentError("docker-compose is not running.")
- if not should_compose_run and is_running:
- raise EnvironmentError("docker-compose is already running.")
+ is_running = is_docker_compose_running(project_name)
+ if should_compose_run:
+ if not is_running:
+ return
+ elif is_running:
+ raise EnvironmentError("docker-compose is already running.") 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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): | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
Validate that a log directory path of a component is valid. | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def validate_log_directory(logs_dir: pathlib.Path, component_name: str) -> None: | ||||||||||||||||||||||||||||||||||||||
:param logs_dir: | ||||||||||||||||||||||||||||||||||||||
:param component_name: | ||||||||||||||||||||||||||||||||||||||
:raises ValueError: If the path is invalid or not a directory. | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||
validate_path_could_be_dir(logs_dir) | ||||||||||||||||||||||||||||||||||||||
except ValueError as ex: | ||||||||||||||||||||||||||||||||||||||
|
@@ -309,6 +306,19 @@ def generate_container_config( | |||||||||||||||||||||||||||||||||||||
return container_clp_config, docker_mounts | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def generate_docker_compose_container_config(clp_config: CLPConfig) -> CLPConfig: | ||||||||||||||||||||||||||||||||||||||
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 and the mounts. | ||||||||||||||||||||||||||||||||||||||
""" | ||||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||
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,40 @@ 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, base_config: pathlib.Path, data_dir: pathlib.Path, logs_dir: pathlib.Path | ||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||
if not base_config.exists(): | ||||||||||||||||||||||||||||||||||||||
raise ValueError( | ||||||||||||||||||||||||||||||||||||||
f"{DB_COMPONENT_NAME} base configuration at {str(base_config)} is missing." | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+496
to
+502
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) Consider validating base_config is a file, not just that it exists. The function checks if Apply this diff to add file type validation: if not base_config.exists():
raise ValueError(
f"{DB_COMPONENT_NAME} base configuration at {str(base_config)} is missing."
)
+ if not base_config.is_file():
+ raise ValueError(
+ f"{DB_COMPONENT_NAME} base configuration at {str(base_config)} is not a file."
+ ) 📝 Committable suggestion
Suggested change
🤖 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) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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, base_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(): | ||||||||||||||||||||||||||||||||||||||
raise ValueError( | ||||||||||||||||||||||||||||||||||||||
f"{REDIS_COMPONENT_NAME} base configuration at {str(base_config)} is missing." | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
517
to
521
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) Consider validating base_config is a file for Redis config. Same issue as the database config validation - should verify it's a file, not just that it exists. Apply this diff to add file type validation: if not base_config.exists():
raise ValueError(
f"{REDIS_COMPONENT_NAME} base configuration at {str(base_config)} is missing."
)
+ if not base_config.is_file():
+ raise ValueError(
+ f"{REDIS_COMPONENT_NAME} base configuration at {str(base_config)} is not a file."
+ ) 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||
_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) | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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 +537,14 @@ 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, base_config: pathlib.Path, data_dir: pathlib.Path, logs_dir: pathlib.Path | ||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||
if not base_config.exists(): | ||||||||||||||||||||||||||||||||||||||
raise ValueError( | ||||||||||||||||||||||||||||||||||||||
f"{RESULTS_CACHE_COMPONENT_NAME} base configuration at {str(base_config)} is missing." | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+540
to
+545
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) Consider validating base_config is a file for results cache config. Consistent with the other config validations, should verify it's a file. Apply this diff to add file type validation: if not base_config.exists():
raise ValueError(
f"{RESULTS_CACHE_COMPONENT_NAME} base configuration at {str(base_config)} is missing."
)
+ if not base_config.is_file():
+ raise ValueError(
+ f"{RESULTS_CACHE_COMPONENT_NAME} base configuration at {str(base_config)} is not a file."
+ ) 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||
_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", | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
from clp_py_utils.clp_config import ( | ||
CLP_DB_PASS_ENV_VAR_NAME, | ||
CLP_DB_USER_ENV_VAR_NAME, | ||
CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||
CLPConfig, | ||
Database, | ||
) | ||
Comment on lines
12
to
18
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. 💡 Verification agent 🧩 Analysis chainImport relocation to clp_py_utils.clp_config looks correct; default path construction remains consistent. No functional change; aligns with config centralization. Run to confirm there are no lingering imports from the old location: Also applies to: 281-284 🏁 Script executed: #!/bin/bash
rg -nP --glob '!**/site-packages/**' $'from\\s+clp_package_utils\\.general\\s+import\\s+[^\\n]*\\bCLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH\\b' Length of output: 133 🏁 Script executed: #!/bin/bash
# Search for any lingering imports of relocated constants from the old module
rg -nP --glob '!**/site-packages/**' $'from\\s+clp_package_utils\\.general\\s+import\\s+[^\\n]*(CLP_DB_PASS_ENV_VAR_NAME|CLP_DB_USER_ENV_VAR_NAME|CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH)\\b' -n
# Search for any import of the old general module
rg -nP --glob '!**/site-packages/**' 'from\s+clp_package_utils\.general\s+import' -n
# Search for any direct references to the old module namespace
rg -nP --glob '!**/site-packages/**' 'clp_package_utils\.general\.' -n Length of output: 1994 Update all remaining imports from
Replace these with the corresponding imports from 🤖 Prompt for AI Agents
|
||
|
@@ -25,7 +26,6 @@ | |
) | ||
|
||
from clp_package_utils.general import ( | ||
CLP_DEFAULT_CONFIG_FILE_RELATIVE_PATH, | ||
EXTRACT_FILE_CMD, | ||
EXTRACT_IR_CMD, | ||
EXTRACT_JSON_CMD, | ||
|
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.
@kirkrodrigues any reason we didn't make the
validate_and_load_
functions as instance methods?