new version with training fixes - 0.1.4 - #14
Conversation
[DEV] feat: adding ruff
[DEV] feat: adding mypy
[DEV] infra: setting actions to be executed on dev
…ove logging config
feat: centralizing env vars with the singleton pattern
updating examples
WalkthroughThis update introduces significant architectural and infrastructure changes to the project. Environment variable management is centralized with a new Changes
Sequence Diagram(s)Centralized Environment Variable HandlingsequenceDiagram
participant User
participant Pipeline/Module
participant EnvVars
participant os.environ
User->>Pipeline/Module: Initiate operation (e.g., training, evaluation)
Pipeline/Module->>EnvVars: Update/merge env vars
EnvVars->>EnvVars: Validate/prepare values
Pipeline/Module->>EnvVars: load_to_environment()
EnvVars->>os.environ: Set environment variables
Pipeline/Module->>Pipeline/Module: Proceed with logic using updated env
Stream Proxy Request HandlingsequenceDiagram
participant Client
participant FastAPI App
participant stream_proxy_routes
participant stream_proxy_handlers
participant Upstream Service
Client->>FastAPI App: GET /stream?container_id=...
FastAPI App->>stream_proxy_routes: proxy_stream(...)
stream_proxy_routes->>Upstream Service: Async HTTP request
Upstream Service-->>stream_proxy_routes: Response/stream
stream_proxy_routes->>stream_proxy_handlers: create_stream_generator(...)
stream_proxy_handlers-->>FastAPI App: Stream response
FastAPI App-->>Client: Streamed data
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 36
🔭 Outside diff range comments (2)
drfc_manager/utils/paths.py (1)
19-19: Consider security implications of 0o777 permissions.Setting directory permissions to
0o777gives full read/write/execute access to all users, which may be overly permissive and could pose security risks.Consider using more restrictive permissions like
0o755or0o750unless the broad access is specifically required for your use case.- os.chmod(dir_path, 0o777) + os.chmod(dir_path, 0o755)drfc_manager/transformers/training.py (1)
71-73: Add exception chaining.except Exception as e: - raise BaseExceptionTransformers("Failed to upload model metadata", e) + raise BaseExceptionTransformers("Failed to upload model metadata", e) from e
🧹 Nitpick comments (52)
drfc_manager/utils/minio/links.py (2)
6-6: Port replacement is brittle – parse the URL instead of a blind.replace()A blind
str.replace("9000", "9001")risks corrupting hostnames or paths that legitimately contain the substring 9000.
Parsing the URL and adjusting only the port component is safer and still concise.-import urllib.parse +import urllib.parse + +from urllib.parse import urlsplit, urlunsplit-minio_console_url = minio_url.replace("9000", "9001").rstrip("/") +parts = urlsplit(minio_url.rstrip("/")) +# Only swap the port when it really is `9000` +netloc = parts.hostname if parts.port is None else f"{parts.hostname}:{'9001' if parts.port == 9000 else parts.port}" +minio_console_url = urlunsplit(parts._replace(netloc=netloc))(Feel free to adjust style – the key point is to avoid accidental replacements.)
7-9: Encodebucket_nameas well to cover edge-cases
bucket_nameis interpolated raw in the URL. Although typical names are DNS-compatible, S3 allows dots and, depending on deployment, other characters that should be percent-encoded.-object_path_encoded = urllib.parse.quote(object_path, safe="") -return urljoin( - minio_console_url + "/", f"browser/{bucket_name}/{object_path_encoded}" -) +bucket_encoded = urllib.parse.quote(bucket_name, safe="") +object_encoded = urllib.parse.quote(object_path, safe="") +return urljoin( + minio_console_url + "/", f"browser/{bucket_encoded}/{object_encoded}" +)Optional but makes the helper future-proof.
drfc_manager/transformers/general.py (2)
31-31: Name assignment is fine but loses static valueIf
_log’s name is purely cosmetic you can ignore this, otherwise consider using@transformer(name=f"log: {message[:30]}...")ifgloesupports it to avoidtype: ignore.
64-75: Flatten conditional – removes redundantelifand clarifies flowMinor readability refactor; also satisfies pylint R1705.
- if exists and not overwrite: - logger.info(f"Model prefix {prefix} exists and overwrite is False.") - return True - elif exists and overwrite: - logger.info( - f"Model prefix {prefix} exists but overwrite is True. Proceeding (Overwrite logic TBD)." - ) - return False - else: - logger.info(f"Model prefix {prefix} does not exist. Proceeding.") - return False + if exists: + if not overwrite: + logger.info(f"Model prefix {prefix} exists and overwrite is False.") + return True + + logger.info( + f"Model prefix {prefix} exists but overwrite is True. Proceeding (overwrite logic TBD)." + ) + return False + + logger.info(f"Model prefix {prefix} does not exist. Proceeding.") + return Falsepyproject.toml (1)
42-47: Duplicatebump2versiondeclarationIt appears in both default and
devsections; resolve to a single entry.drfc_manager/models/model_operations.py (1)
40-46: Movereimport back to module scopeImporting
reinsidegenerate_model_nameincurs an extra (albeit tiny) overhead on every call and diverges from the code-base pattern of keeping imports at the top. Consider moving it back to the module level so the import happens once and keeps the function body focused on business logic.-import re +import re.github/workflows/mypy.yml (1)
43-44: Add trailing newlineYAML-lint reports a missing newline at EOF. Add one to keep linters green.
drfc_manager/utils/str_to_bool.py (1)
2-3: Expand the docstring to list accepted truthy stringsBeing explicit helps future readers avoid guessing what the helper treats as truthy. Consider amending the docstring to show the exact accepted literals.
- """Converts string representations of truth to bool.""" + """Return `True` when the lower-cased value matches + any of: "yes", "true", "t", "1"; otherwise `False`."""drfc_manager/utils/minio/exceptions/file_upload_exception.py (1)
26-28: Drop the redundantelseand leverage exception chainingRuff/Pylint flag the
elseafterreturnas dead code, andraise … from epreserves the original traceback.- def __str__(self): - if self.original_exception: - return f"{self.message} Original Exception: {str(self.original_exception)}" - else: - return self.message + def __str__(self): + if self.original_exception: + return ( + f"{self.message} Original Exception: {self.original_exception}" + ) + return self.messageWhile not in diff-scope, the same tweak can be mirrored in
FileUploadException.Also applies to: 41-44
drfc_manager/utils/minio/utilities.py (1)
30-33: Use exception chaining for clearer tracebacks
raise … from elinks the original stack trace automatically.- raise FunctionConversionException( - message="Failed to convert reward function to BytesIO.", - original_exception=e, - ) + raise FunctionConversionException( + message="Failed to convert reward function to BytesIO.", + original_exception=e, + ) from eREADME.md (1)
166-169: Consider reducing exclamation marks for a more professional tone.The contributing section is a valuable addition that aligns with adding CONTRIBUTING.md. However, consider reducing the exclamation marks for a more professional tone.
-We welcome contributions! Please see our [CONTRIBUTING.md](CONTRIBUTING.md) for guidelines and instructions. +We welcome contributions! Please see our [CONTRIBUTING.md](CONTRIBUTING.md) for guidelines and instructions.drfc_manager/models/storage_operations.py (1)
8-8: Fix formatting: add blank line before function definition.Static analysis indicates missing blank line before function definition.
+ def check_model_exists(storage_client: Any, model_name: str) -> bool:drfc_manager/models/env_operations.py (1)
6-6: Fix formatting: add blank line before class definition.Static analysis indicates missing blank line before class definition.
+ @dataclass(frozen=True) class EnvConfig:drfc_manager/helpers/files_manager.py (1)
24-25: Clarify file deletion behavior.The function only deletes files, not subdirectories. This behavior should be documented or the function name should be more specific.
Consider either:
- Renaming to
delete_files_in_folderto be more explicit- Adding a docstring to clarify the behavior
- Adding an option to handle subdirectories if needed
def delete_files_on_folder(folder_name: str) -> None: + """Delete all files in the specified folder (subdirectories are not deleted)."""tests/unit/utils/test_logging.py (2)
16-17: Consider using pytest fixtures for cleanup instead of manual file removal.The manual cleanup with
os.remove()could be replaced with proper pytest teardown to ensure cleanup even if the test fails.- # Cleanup - os.remove(log_path)Since you're using
tmp_path, the temporary directory and its contents will be automatically cleaned up by pytest after the test completes.
39-40: Consider using pytest fixtures for cleanup instead of manual file removal.Similar to the previous test, the manual cleanup with
os.remove()could be replaced with proper pytest teardown.- # Cleanup - os.remove(local_yaml_path)Since you're using
tmp_path, the temporary directory and its contents will be automatically cleaned up by pytest after the test completes.tests/unit/helpers/test_training_params.py (1)
39-40: Consider using pytest fixtures for cleanup instead of manual file removal.Similar to the logging tests, the manual cleanup with
os.remove()could be replaced with proper pytest teardown.- # Cleanup - os.remove(local_yaml_path)Since you're using
tmp_path, the temporary directory and its contents will be automatically cleaned up by pytest after the test completes.examples/evaluation.ipynb (2)
51-51: Consider parameterizing the hardcoded model name.The hardcoded model name
'rl-deepracer-sagemaker-v2'in the notebook could be made configurable to improve reusability.-model_name = 'rl-deepracer-sagemaker-v2' +# Update this model name as needed for your evaluation +model_name = 'rl-deepracer-sagemaker-v2'Adding a comment would help users understand they should update this value for their specific models.
132-132: Consider using centralized configuration for MinIO URL.The hardcoded MinIO URL
"http://jaguaribe.quixada.ufc.br:9001"should ideally come from the centralized configuration system to maintain consistency with the broader refactoring effort.-minio_url = "http://jaguaribe.quixada.ufc.br:9001" +# Get MinIO URL from settings or use default +minio_url = getattr(settings.minio, 'console_url', "http://jaguaribe.quixada.ufc.br:9001")This would make the notebook more portable and consistent with the centralized configuration approach.
tests/fixtures/stream.py (1)
33-62: Consider adding type hints for better maintainability.The fixtures are well-designed for testing different streaming scenarios, but adding type hints would improve code clarity and IDE support.
+from typing import AsyncIterator as AsyncIteratorType +from unittest.mock import AsyncMock @pytest.fixture -def normal_response(): +def normal_response() -> AsyncMock: mock = AsyncMock() mock.aiter_bytes = lambda chunk_size=None: AsyncIterator([b"chunk1", b"chunk2"]) return mock @pytest.fixture -def empty_response(): +def empty_response() -> AsyncMock: mock = AsyncMock() mock.aiter_bytes = lambda chunk_size=None: AsyncIterator([]) return mock @pytest.fixture -def read_error_response(): +def read_error_response() -> AsyncMock: mock = AsyncMock() mock.aiter_bytes = lambda chunk_size=None: ErrorAsyncIterator( httpx.ReadError("Test error") ) return mock @pytest.fixture -def unexpected_error_response(): +def unexpected_error_response() -> AsyncMock: mock = AsyncMock() mock.aiter_bytes = lambda chunk_size=None: ErrorAsyncIterator( Exception("Unexpected error") ) return mockdrfc_manager/utils/docker/utilities.py (1)
9-27: Consider the redundant existence check.The function now uses
get_docker_compose_pathwhich already checks for file existence and raisesFileNotFoundErrorif the file doesn't exist. The additionalcompose_path.exists()check on line 22 is redundant sinceget_docker_compose_pathwould have already raised an exception if the file didn't exist.def adjust_composes_file_names(composes_names: List[str]) -> List[str]: """ Adjusts the names of Docker Compose files. Args: composes_names (List[str]): List of Docker Compose file names. Returns: List[str]: Adjusted list containing the paths to Docker Compose files. """ compose_files = [] for compose_name in composes_names: compose_path = get_docker_compose_path(compose_name) - if compose_path.exists(): - compose_files.append(str(compose_path)) - else: - raise FileNotFoundError(f"Docker compose file not found: {compose_path}") + compose_files.append(str(compose_path)) return compose_filesThis simplification is possible because
get_docker_compose_pathalready handles the existence check and raisesFileNotFoundErrorwhen the file doesn't exist.CONTRIBUTING.md (2)
1-76: Comprehensive contributor guide with minor style suggestion.The contributing guidelines are well-structured and cover all essential aspects of the development workflow. The inclusion of pre-commit hooks, linting with Ruff, and clear PR procedures aligns well with the CI/CD improvements in this PR.
Consider reducing the number of exclamation marks for a more professional tone (static analysis flagged this as excessive):
-Thank you for your interest in contributing to **drfc-manager**! 🎉 +Thank you for your interest in contributing to **drfc-manager**! 🎉 -Thank you for helping make **drfc-manager** better! 🚗💨 +Thank you for helping make **drfc-manager** better! 🚗💨
36-39: Consider uncommenting the test sections when ready.The commented-out test sections suggest that comprehensive testing isn't fully implemented yet. Consider creating a follow-up issue to uncomment these sections once the test suite is complete.
Would you like me to create a follow-up issue to track the completion of the testing documentation once the test suite is fully implemented?
Also applies to: 70-70
drfc_manager/models/data_extraction.py (1)
92-94: Consider adding error context for reward function loading.The reward function loading logic could benefit from more specific error handling to distinguish between different failure scenarios (file not found, invalid Python code, missing function, etc.).
try: reward_code = storage_client.download_py_object( f"{source_model}/reward_function.py" ) + except FileNotFoundError: + logger.info(f"No reward function file found for model: {source_model}") + return create_default_reward_function(), None + except Exception as e: + logger.warning(f"Failed to download reward function for {source_model}: {e}") + return create_default_reward_function(), None + + try: namespace: Dict[str, Any] = {} exec(reward_code, namespace) reward_function = namespace.get("reward_function") if not callable(reward_function): raise ValueError("reward_function is not callable") return reward_function, reward_code except Exception as e: - logger.warning(f"Failed to load reward function from source model: {e}") + logger.warning(f"Failed to execute reward function for {source_model}: {e}") return create_default_reward_function(), Nonetests/unit/viewers/test_stream_proxy_routes.py (1)
52-78: Consider adding more HTTP status code test cases.The HTTP ping tests are good, but could benefit from testing more status codes to ensure comprehensive coverage.
# Test service unavailable (503 - common in container orchestration) mock_response = AsyncMock() mock_response.status_code = 503 mock_client.get.return_value = mock_response with pytest.raises(StreamProxyPingError) as exc_info: await check_http_ping(mock_client, "http://localhost:8080") assert "server error status" in str(exc_info.value) + # Test client error (4xx should be treated as success per implementation) + mock_response = AsyncMock() + mock_response.status_code = 404 + mock_client.get.return_value = mock_response + result = await check_http_ping(mock_client, "http://localhost:8080") + assert result[0] is True # 4xx should be treated as success + + # Test timeout scenario + mock_client.get.side_effect = httpx.TimeoutException("Request timeout") + with pytest.raises(StreamProxyPingError) as exc_info: + await check_http_ping(mock_client, "http://localhost:8080") + assert "Timeout" in str(exc_info.value)drfc_manager/evaluation/get_compose_files.py (1)
78-80: Fix formatting: Remove extra blank lines.Static analysis flagged excessive blank lines here.
- - separator = settings.docker.dr_docker_file_sep + separator = settings.docker.dr_docker_file_sepdrfc_manager/utils/logging_config.py (2)
29-29: Fix formatting issues.Static analysis flagged formatting problems.
- console_env = os.environ.get('DRFC_CONSOLE_LOGGING', 'false').lower() in ('1','true','yes') + console_env = os.environ.get('DRFC_CONSOLE_LOGGING', 'false').lower() in ('1', 'true', 'yes')
33-37: Fix excessive blank lines.Static analysis flagged too many blank lines.
- - for h in logging.root.handlers[:]: - logging.root.removeHandler(h) - - handlers: list[Union[logging.StreamHandler, logging.FileHandler]] = [] + for h in logging.root.handlers[:]: + logging.root.removeHandler(h) + + handlers: list[Union[logging.StreamHandler, logging.FileHandler]] = []drfc_manager/viewers/stream_proxy_utils.py (2)
59-62: Consider refactoring to reduce parameter count.The function has 6 parameters which exceeds the typical limit of 5. Consider using a configuration object or data class to group related parameters.
+from dataclasses import dataclass + +@dataclass +class StreamConfig: + host: str + port: int + topic: str + quality: int + width: int + height: int + -def build_stream_url( - host: str, port: int, topic: str, quality: int, width: int, height: int -) -> str: - return f"http://{host}:{port}/stream?topic={topic}&quality={quality}&width={width}&height={height}" +def build_stream_url(config: StreamConfig) -> str: + return f"http://{config.host}:{config.port}/stream?topic={config.topic}&quality={config.quality}&width={config.width}&height={config.height}"
111-143: Consider using a data class for health response parameters.The function has 8 parameters which significantly exceeds the recommended limit. Consider grouping related parameters into data classes.
+from dataclasses import dataclass + +@dataclass +class HealthCheckData: + target_host: str + target_port: int + socket_status: str + ping_status: str + containers: List[str] + error_details: Dict[str, Any] + target_reachable: bool + target_responsive: bool + -def build_health_response( - target_host: str, - target_port: int, - socket_status: str, - ping_status: str, - containers: List[str], - error_details: Dict[str, Any], - target_reachable: bool, - target_responsive: bool, -) -> Dict[str, Any]: +def build_health_response(data: HealthCheckData) -> Dict[str, Any]:drfc_manager/helpers/training_params.py (2)
7-12: Add blank line to comply with PEP 8.Add an extra blank line before the function definition:
from drfc_manager.types.env_vars import EnvVars env_vars = EnvVars() + def _setting_envs(train_time: str, model_name: str) -> Dict[str, Any]:
12-74: Consider refactoring to reduce function complexity.The function has 52 statements (pylint limit is 50). Consider extracting race-type specific configurations into separate helper functions.
Would you like me to help refactor this function by extracting the race-type specific logic into separate helper functions? This would improve maintainability and reduce the statement count.
drfc_manager/pipelines/evaluation.py (2)
259-267: Improve readability of condition check.The Yoda condition makes the code less readable.
- if ( - clone - and "cloned_prefix" in locals() - and env_vars.DR_LOCAL_S3_MODEL_PREFIX == cloned_prefix - ): + if ( + clone + and "cloned_prefix" in locals() + and cloned_prefix == env_vars.DR_LOCAL_S3_MODEL_PREFIX + ):
299-302: Simplify nested if statements.Combine the nested conditions for cleaner code.
- if result and "output" in result and result["output"]: - if result["output"].strip(): - logger.debug(result["output"]) + if result and "output" in result and result["output"] and result["output"].strip(): + logger.debug(result["output"])drfc_manager/viewers/stream_proxy_routes.py (2)
208-376: Consider refactoring this complex function.With 85 statements and 15 branches, this function is difficult to maintain and test.
Consider extracting the streaming logic into smaller, focused functions:
handle_successful_response()for the 200 status code pathhandle_error_response()for non-200 responsescleanup_resources()for resource cleanup logicWould you like me to help refactor this function into smaller, more maintainable components?
253-327: Consider extracting the streaming generator logic.The inline generator and resource cleanup could be extracted for better testability.
Instead of defining
stream_generatorandclose_resourcesinline, consider extracting them as separate functions that can be tested independently. This would also reduce the complexity of the main function.drfc_manager/pipelines/viewer.py (1)
123-209: Consider simplifying this complex function.With 65 statements and 13 branches, this function is difficult to follow and maintain.
Consider extracting the signal escalation logic (SIGTERM → wait → SIGKILL) into a separate helper function. This would make the code more readable and reusable.
examples/training.ipynb (1)
72-74: Consider uncommenting the envs display for documentation.The commented
#envscould be useful for users to see available environment variables.envs = EnvVars() -#envs +# Uncomment the line below to see all available environment variables +# envsdrfc_manager/pipelines/training.py (4)
43-45: Add blank line to comply with PEP 8.The code is missing a blank line before the function definition.
storage_manager = MinioStorageManager(settings) env_vars = EnvVars() + @transformer def sleep_15_seconds(_):
64-71: Remove extra blank lines.There are unnecessary blank lines that should be removed.
raise DockerError(f"Missing critical environment variables: {', '.join(missing_vars)}") - logger.info("Environment variables loaded:") logger.info(f"DR_SIMAPP_SOURCE: {env_vars.DR_SIMAPP_SOURCE}") logger.info(f"DR_SIMAPP_VERSION: {env_vars.DR_SIMAPP_VERSION}") - - - -
98-106: Consider safer attribute copying for EnvVars update.The current implementation copies all non-private attributes, which could include unintended attributes. Consider using a more explicit approach.
env_vars_instance = EnvVars() if env_vars: - env_vars_instance.update(**{k: v for k, v in env_vars.__dict__.items() if not k.startswith('_')}) + # Only update known EnvVars fields + update_dict = {} + for k, v in env_vars.__dict__.items(): + if not k.startswith('_') and hasattr(env_vars_instance, k): + update_dict[k] = v + env_vars_instance.update(**update_dict) env_vars_instance.load_to_environment()
201-212: Consider using a configuration object to reduce parameter count.The function has 10 parameters, which makes it hard to maintain and call correctly. Consider using a dataclass or configuration object.
Would you like me to generate a
ClonePipelineConfigdataclass to encapsulate these parameters and simplify the function signature?drfc_manager/transformers/training.py (1)
218-226: Consider removing or deprecating this no-op function.This function no longer performs any operations. If it's no longer needed, consider removing it entirely from the codebase and its callers. If it needs to be kept for compatibility, mark it as deprecated.
@partial_transformer +@deprecated("This function is no longer needed as SageMaker handles IP upload") def upload_ip_config(_, model_name: str): """Upload Redis IP config (ip.json and done flag) to S3""" # The SageMaker container will upload its own IP address when it starts # This function is called by the DRFC manager before starting containers # We need to wait for the SageMaker container to upload its IP logger.info("Skipping Redis IP config upload - SageMaker container will upload its own IP") passdrfc_manager/viewers/streamlit_viewer.py (3)
45-61: Add blank line before function definition.DEFAULT_CAMERA_ID = "kvs_stream" DEFAULT_CAMERA_TOPIC = "/racecar/deepracer/kvs_stream" + def init_session_state():
334-335: Add blank line after function definition.return streams_to_show + init_session_state()
304-311: Verify the behavioral change for "All" selections.The logic has changed to show all camera streams for only the first worker when both selections are "All". This is a breaking change from previous behavior. Ensure this aligns with user expectations.
Would you like me to implement an option to toggle between showing all cameras for the first worker vs. showing a default camera for all workers?
drfc_manager/utils/minio/storage_manager.py (1)
40-43: Consider deprecating AppConfig parameter.The constructor accepts an optional
AppConfigparameter but the class primarily uses theenv_varssingleton for configuration. This dual approach could lead to confusion. Consider fully migrating to the EnvVars pattern.drfc_manager/types/env_vars.py (1)
189-189: Replace print statement with logger.Debug output should use the logging framework instead of print statements.
- print("Initializing EnvVars for the first time") + logger.debug("Initializing EnvVars for the first time")drfc_manager/utils/docker/docker_manager.py (4)
183-183: Remove redundant load_to_environment() callThis call is redundant as
load_to_environment()was already called in lines 171, 176, and 180.-self.env_vars.load_to_environment() -logger.info("Loaded all environment variables")
116-117: Simplify nested if statementsCombine the nested if statements into a single condition for better readability.
-if workers > 1 and getattr(self.env_vars, 'DR_DOCKER_STYLE', 'compose') != "swarm": - if self._setup_multiworker_env(): +if (workers > 1 and + getattr(self.env_vars, 'DR_DOCKER_STYLE', 'compose') != "swarm" and + self._setup_multiworker_env()):
416-416: Use != operator instead of not ==Replace
not cmd[-1] == "-f"with the more idiomaticcmd[-1] != "-f"for better readability.For line 416:
-if not cmd[-1] == "-f": +if cmd[-1] != "-f":For line 442:
-if not cmd[-1] == "-f": +if cmd[-1] != "-f":For line 467:
-if not cmd[-1] == "-c": +if cmd[-1] != "-c":Also applies to: 442-442, 467-467
312-312: Remove or use the unused result variableThe
resultvariable is assigned but never used. Either use it for validation or remove the assignment.-result = self._run_command(cmd, env=env) # noqa: F841 +self._run_command(cmd, env=env)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lockrepo_logo.pngis excluded by!**/*.png
📒 Files selected for processing (73)
.bumpversion.cfg(1 hunks).github/workflows/mypy.yml(1 hunks).github/workflows/ruff.yml(1 hunks).github/workflows/unit-tests.yml(1 hunks).pre-commit-config.yaml(1 hunks)CONTRIBUTING.md(1 hunks)README.md(4 hunks)drfc_manager/__init__.py(1 hunks)drfc_manager/config/drfc-images/docker-compose-training.yml(1 hunks)drfc_manager/config_env.py(1 hunks)drfc_manager/evaluation/get_compose_files.py(2 hunks)drfc_manager/evaluation/setup_evaluation_env.py(0 hunks)drfc_manager/evaluation/start_evaluation_stack.py(0 hunks)drfc_manager/evaluation/stop_evaluation_stack.py(1 hunks)drfc_manager/helpers/config_envs.py(0 hunks)drfc_manager/helpers/files_manager.py(1 hunks)drfc_manager/helpers/training_params.py(1 hunks)drfc_manager/models/data_extraction.py(1 hunks)drfc_manager/models/env_operations.py(1 hunks)drfc_manager/models/model_operations.py(3 hunks)drfc_manager/models/storage_operations.py(1 hunks)drfc_manager/pipelines/__init__.py(2 hunks)drfc_manager/pipelines/evaluation.py(5 hunks)drfc_manager/pipelines/metrics.py(7 hunks)drfc_manager/pipelines/training.py(5 hunks)drfc_manager/pipelines/viewer.py(14 hunks)drfc_manager/transformers/exceptions/base.py(1 hunks)drfc_manager/transformers/general.py(4 hunks)drfc_manager/transformers/helpers.py(1 hunks)drfc_manager/transformers/training.py(6 hunks)drfc_manager/types/config.py(0 hunks)drfc_manager/types/constants.py(1 hunks)drfc_manager/types/docker.py(2 hunks)drfc_manager/types/env_vars.py(6 hunks)drfc_manager/types/hyperparameters.py(3 hunks)drfc_manager/types/model_metadata.py(6 hunks)drfc_manager/utils/docker/docker_manager.py(3 hunks)drfc_manager/utils/docker/exceptions/base.py(2 hunks)drfc_manager/utils/docker/utilities.py(1 hunks)drfc_manager/utils/env_utils.py(1 hunks)drfc_manager/utils/logging.py(2 hunks)drfc_manager/utils/logging_config.py(1 hunks)drfc_manager/utils/minio/exceptions/file_upload_exception.py(2 hunks)drfc_manager/utils/minio/links.py(1 hunks)drfc_manager/utils/minio/storage_client.py(1 hunks)drfc_manager/utils/minio/storage_manager.py(2 hunks)drfc_manager/utils/minio/utilities.py(1 hunks)drfc_manager/utils/paths.py(2 hunks)drfc_manager/utils/redis/manager.py(0 hunks)drfc_manager/utils/str_to_bool.py(1 hunks)drfc_manager/viewers/exceptions.py(1 hunks)drfc_manager/viewers/stream_proxy.py(1 hunks)drfc_manager/viewers/stream_proxy_handlers.py(1 hunks)drfc_manager/viewers/stream_proxy_routes.py(1 hunks)drfc_manager/viewers/stream_proxy_utils.py(1 hunks)drfc_manager/viewers/streamlit_viewer.py(8 hunks)example.env(0 hunks)examples/evaluation.ipynb(6 hunks)examples/training.ipynb(16 hunks)pyproject.toml(2 hunks)pytest.ini(1 hunks)tests/conftest.py(1 hunks)tests/fixtures/logger.py(1 hunks)tests/fixtures/stream.py(1 hunks)tests/unit/helpers/test_files_manager.py(1 hunks)tests/unit/helpers/test_training_params.py(1 hunks)tests/unit/test_config_env.py(1 hunks)tests/unit/utils/test_logging.py(1 hunks)tests/unit/utils/test_paths.py(1 hunks)tests/unit/utils/test_str_to_bool.py(1 hunks)tests/unit/viewers/test_stream_proxy_handlers.py(1 hunks)tests/unit/viewers/test_stream_proxy_routes.py(1 hunks)tests/unit/viewers/test_stream_proxy_utils.py(1 hunks)
💤 Files with no reviewable changes (6)
- drfc_manager/types/config.py
- example.env
- drfc_manager/helpers/config_envs.py
- drfc_manager/utils/redis/manager.py
- drfc_manager/evaluation/setup_evaluation_env.py
- drfc_manager/evaluation/start_evaluation_stack.py
🧰 Additional context used
🧬 Code Graph Analysis (23)
drfc_manager/models/storage_operations.py (3)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/utils/minio/storage_manager.py (1)
StorageError(31-34)drfc_manager/models/model_operations.py (1)
ModelData(8-15)
tests/unit/test_config_env.py (1)
drfc_manager/config_env.py (1)
MinioConfig(6-29)
drfc_manager/transformers/general.py (1)
drfc_manager/transformers/exceptions/base.py (1)
BaseExceptionTransformers(4-18)
drfc_manager/utils/minio/utilities.py (3)
drfc_manager/utils/minio/exceptions/file_upload_exception.py (1)
FunctionConversionException(23-44)drfc_manager/types/hyperparameters.py (1)
HyperParameters(30-62)drfc_manager/types/model_metadata.py (1)
ModelMetadata(93-115)
tests/unit/helpers/test_files_manager.py (1)
drfc_manager/helpers/files_manager.py (2)
create_folder(6-17)delete_files_on_folder(20-31)
drfc_manager/models/env_operations.py (1)
drfc_manager/types/env_vars.py (3)
EnvVars(14-367)update(199-203)load_to_environment(210-225)
tests/unit/utils/test_str_to_bool.py (1)
drfc_manager/utils/str_to_bool.py (1)
str2bool(1-3)
drfc_manager/models/model_operations.py (3)
drfc_manager/types/hyperparameters.py (1)
HyperParameters(30-62)drfc_manager/types/model_metadata.py (1)
ModelMetadata(93-115)drfc_manager/utils/docker/docker_manager.py (1)
check_logs(387-400)
tests/unit/viewers/test_stream_proxy_routes.py (2)
drfc_manager/viewers/stream_proxy_routes.py (3)
validate_container_id(45-53)check_socket_connection(106-156)check_http_ping(159-205)drfc_manager/viewers/exceptions.py (3)
UnknownContainerError(43-46)StreamProxySocketError(55-58)StreamProxyPingError(61-64)
tests/unit/viewers/test_stream_proxy_handlers.py (2)
drfc_manager/viewers/stream_proxy_handlers.py (1)
create_stream_generator(46-78)tests/fixtures/stream.py (4)
normal_response(34-37)empty_response(41-44)read_error_response(48-53)unexpected_error_response(57-62)
tests/unit/utils/test_logging.py (1)
drfc_manager/utils/logging.py (3)
setup_logging(21-63)get_recent_logs(69-82)log_execution(85-99)
drfc_manager/utils/env_utils.py (2)
drfc_manager/types/env_vars.py (2)
EnvVars(14-367)update(199-203)drfc_manager/utils/logging_config.py (1)
get_logger(85-95)
drfc_manager/models/data_extraction.py (3)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/models/model_operations.py (1)
ModelData(8-15)drfc_manager/utils/minio/storage_manager.py (2)
download_json(232-243)download_py_object(245-255)
drfc_manager/utils/docker/utilities.py (2)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/utils/paths.py (1)
get_docker_compose_path(61-66)
drfc_manager/viewers/stream_proxy_utils.py (3)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/viewers/exceptions.py (1)
StreamResponseError(25-28)drfc_manager/utils/logging_config.py (1)
get_logger(85-95)
tests/unit/utils/test_paths.py (1)
drfc_manager/utils/paths.py (5)
ensure_dir_exists(16-23)get_internal_path(26-48)get_comms_dir(51-53)get_logs_dir(56-58)get_docker_compose_path(61-66)
drfc_manager/helpers/training_params.py (2)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/helpers/files_manager.py (1)
create_folder(6-17)
drfc_manager/viewers/stream_proxy_handlers.py (1)
drfc_manager/utils/logging_config.py (1)
get_logger(85-95)
drfc_manager/viewers/stream_proxy.py (4)
drfc_manager/types/env_vars.py (1)
EnvVars(14-367)drfc_manager/viewers/stream_proxy_routes.py (2)
proxy_stream(208-375)health_check(378-418)drfc_manager/viewers/stream_proxy_utils.py (1)
parse_containers(17-45)drfc_manager/utils/logging_config.py (2)
get_logger(85-95)configure_logging(8-82)
tests/unit/viewers/test_stream_proxy_utils.py (3)
drfc_manager/viewers/stream_proxy_utils.py (6)
parse_containers(17-45)get_target_config(48-56)build_stream_url(59-62)parse_content_type(65-90)format_error_text(93-108)build_health_response(111-143)drfc_manager/viewers/exceptions.py (1)
StreamResponseError(25-28)drfc_manager/utils/logging_config.py (1)
get_logger(85-95)
drfc_manager/pipelines/metrics.py (3)
drfc_manager/utils/docker/utilities.py (1)
adjust_composes_file_names(9-27)drfc_manager/types/docker.py (1)
ComposeFileType(4-26)drfc_manager/utils/logging.py (1)
setup_logging(21-63)
drfc_manager/types/env_vars.py (1)
drfc_manager/utils/str_to_bool.py (1)
str2bool(1-3)
drfc_manager/utils/docker/docker_manager.py (8)
drfc_manager/types/env_vars.py (3)
EnvVars(14-367)update(199-203)load_to_environment(210-225)drfc_manager/utils/docker/exceptions/base.py (1)
DockerError(4-23)drfc_manager/types/docker.py (1)
ComposeFileType(4-26)drfc_manager/utils/minio/storage_manager.py (2)
MinioStorageManager(37-255)upload_local_file(158-172)drfc_manager/utils/paths.py (1)
get_comms_dir(51-53)drfc_manager/utils/env_utils.py (1)
get_subprocess_env(10-31)drfc_manager/utils/docker/utilities.py (1)
adjust_composes_file_names(9-27)drfc_manager/helpers/training_params.py (1)
writing_on_temp_training_yml(77-105)
🪛 Pylint (3.3.7)
drfc_manager/utils/minio/exceptions/file_upload_exception.py
[refactor] 41-44: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
drfc_manager/transformers/general.py
[refactor] 64-74: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
drfc_manager/utils/paths.py
[refactor] 35-35: Consider merging these comparisons with 'in' by using 'parent not in (PACKAGE_ROOT, PACKAGE_ROOT.parent)'. Use a set instead if elements are hashable.
(R1714)
drfc_manager/models/model_operations.py
[refactor] 19-19: Too many instance attributes (9/7)
(R0902)
[refactor] 54-54: Too many arguments (9/5)
(R0913)
[refactor] 54-54: Too many positional arguments (9/5)
(R0917)
drfc_manager/evaluation/stop_evaluation_stack.py
[refactor] 47-59: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
drfc_manager/viewers/stream_proxy_utils.py
[refactor] 59-59: Too many arguments (6/5)
(R0913)
[refactor] 59-59: Too many positional arguments (6/5)
(R0917)
[refactor] 111-111: Too many arguments (8/5)
(R0913)
[refactor] 111-111: Too many positional arguments (8/5)
(R0917)
drfc_manager/config_env.py
[error] 26-26: Method 'ensure_http_scheme' should have "self" as first argument
(E0213)
[refactor] 6-6: Too few public methods (1/2)
(R0903)
[refactor] 32-32: Too few public methods (0/2)
(R0903)
[refactor] 72-72: Too few public methods (0/2)
(R0903)
[refactor] 78-78: Too few public methods (0/2)
(R0903)
drfc_manager/helpers/training_params.py
[refactor] 12-12: Too many statements (52/50)
(R0915)
drfc_manager/viewers/stream_proxy_routes.py
[refactor] 165-174: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 208-208: Too many arguments (7/5)
(R0913)
[refactor] 208-208: Too many positional arguments (7/5)
(R0917)
[refactor] 208-208: Too many local variables (28/15)
(R0914)
[refactor] 253-327: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 208-208: Too many branches (15/12)
(R0912)
[refactor] 208-208: Too many statements (85/50)
(R0915)
drfc_manager/viewers/stream_proxy.py
[refactor] 43-43: Too many arguments (6/5)
(R0913)
[refactor] 43-43: Too many positional arguments (6/5)
(R0917)
drfc_manager/pipelines/training.py
[error] 119-119: No value for argument '_' in function call
(E1120)
[error] 120-120: No value for argument '_' in function call
(E1120)
[error] 121-123: No value for argument '_' in function call
(E1120)
[error] 134-134: No value for argument '_' in function call
(E1120)
[error] 136-139: No value for argument '_' in function call
(E1120)
[error] 142-142: No value for argument '_' in function call
(E1120)
[error] 143-146: No value for argument '_' in function call
(E1120)
[error] 147-150: No value for argument '_' in function call
(E1120)
[error] 151-151: No value for argument '_' in function call
(E1120)
[error] 152-155: No value for argument '_' in function call
(E1120)
[error] 157-159: No value for argument '_' in function call
(E1120)
[error] 160-160: No value for argument '_' in function call
(E1120)
[error] 162-162: No value for argument '_' in function call
(E1120)
[error] 164-164: No value for argument '_' in function call
(E1120)
[error] 165-165: No value for argument '_' in function call
(E1120)
[refactor] 201-201: Too many arguments (10/5)
(R0913)
[refactor] 201-201: Too many positional arguments (10/5)
(R0917)
drfc_manager/pipelines/viewer.py
[refactor] 123-123: Too many branches (13/12)
(R0912)
[refactor] 123-123: Too many statements (65/50)
(R0915)
[refactor] 396-416: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 385-385: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 387-393: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 499-523: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 495-495: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 496-496: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 566-577: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
drfc_manager/types/env_vars.py
[refactor] 14-14: Too many instance attributes (121/7)
(R0902)
drfc_manager/viewers/streamlit_viewer.py
[refactor] 91-91: Too many return statements (7/6)
(R0911)
[refactor] 91-91: Too many branches (13/12)
(R0912)
[refactor] 91-91: Too many statements (64/50)
(R0915)
[refactor] 145-154: Unnecessary "else" after "continue", remove the "else" and de-indent the code inside it
(R1724)
[refactor] 207-207: Too many arguments (6/5)
(R0913)
[refactor] 207-207: Too many positional arguments (6/5)
(R0917)
[refactor] 219-219: Too many arguments (8/5)
(R0913)
[refactor] 219-219: Too many positional arguments (8/5)
(R0917)
[refactor] 219-219: Too many local variables (19/15)
(R0914)
🪛 YAMLlint (1.37.1)
.pre-commit-config.yaml
[warning] 5-5: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/mypy.yml
[error] 44-44: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/ruff.yml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 38-38: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 44-44: no new line character at the end of file
(new-line-at-end-of-file)
[error] 44-44: trailing spaces
(trailing-spaces)
.github/workflows/unit-tests.yml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
[error] 24-24: trailing spaces
(trailing-spaces)
🪛 LanguageTool
README.md
[style] ~168-~168: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2862 characters long)
Context: ...# Contributing We welcome contributions! Please see our [CONTRIBUTING.md](CONTRI...
(EN_EXCESSIVE_EXCLAMATION)
CONTRIBUTING.md
[style] ~75-~75: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1389 characters long)
Context: ...for helping make drfc-manager better! 🚗💨
(EN_EXCESSIVE_EXCLAMATION)
🪛 Ruff (0.11.9)
drfc_manager/models/storage_operations.py
16-16: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
30-30: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
44-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/transformers/general.py
52-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/utils/minio/utilities.py
30-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/utils/paths.py
40-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
44-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/helpers/files_manager.py
13-15: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
27-29: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/viewers/stream_proxy_utils.py
88-88: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/viewers/stream_proxy_routes.py
123-123: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
135-135: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
143-143: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
156-156: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
182-182: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
186-186: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
205-205: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/pipelines/evaluation.py
262-262: Yoda condition detected
Rewrite as cloned_prefix == env_vars.DR_LOCAL_S3_MODEL_PREFIX
(SIM300)
299-300: Use a single if statement instead of nested if statements
(SIM102)
drfc_manager/pipelines/viewer.py
385-385: Use a context manager for opening files
(SIM115)
495-495: Use a context manager for opening files
(SIM115)
drfc_manager/transformers/training.py
39-41: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
43-43: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
56-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
64-64: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
179-181: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/utils/minio/storage_manager.py
239-239: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
251-251: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
drfc_manager/utils/docker/docker_manager.py
116-117: Use a single if statement instead of nested if statements
(SIM102)
416-416: Use cmd[-1] != "-f" instead of not cmd[-1] == "-f"
Replace with != operator
(SIM201)
442-442: Use cmd[-1] != "-f" instead of not cmd[-1] == "-f"
Replace with != operator
(SIM201)
467-467: Use cmd[-1] != "-c" instead of not cmd[-1] == "-c"
Replace with != operator
(SIM201)
🪛 Flake8 (7.2.0)
drfc_manager/models/storage_operations.py
[error] 8-8: expected 2 blank lines, found 1
(E302)
drfc_manager/models/env_operations.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
drfc_manager/evaluation/get_compose_files.py
[error] 79-79: too many blank lines (2)
(E303)
drfc_manager/utils/logging_config.py
[error] 29-29: too many blank lines (2)
(E303)
[error] 29-29: missing whitespace after ','
(E231)
[error] 29-29: missing whitespace after ','
(E231)
[error] 33-33: too many blank lines (2)
(E303)
[error] 37-37: too many blank lines (2)
(E303)
drfc_manager/helpers/training_params.py
[error] 12-12: expected 2 blank lines, found 1
(E302)
drfc_manager/pipelines/training.py
[error] 45-45: expected 2 blank lines, found 1
(E302)
[error] 65-65: too many blank lines (2)
(E303)
drfc_manager/viewers/streamlit_viewer.py
[error] 45-45: expected 2 blank lines, found 1
(E302)
[error] 334-334: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 actionlint (1.7.7)
.github/workflows/mypy.yml
31-31: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/ruff.yml
34-34: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/unit-tests.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (81)
.bumpversion.cfg (1)
2-2: Version bump looks correct, but double-check tag/commit automationBumping to
0.1.4is consistent with the other files.
Please make sure your release workflow (GitHub Actions or similar) actually pushes the tag and publishes the artefact, otherwise the new version will never reach PyPI.drfc_manager/__init__.py (1)
3-3: OK – version string updatedNo functional changes here.
drfc_manager/transformers/helpers.py (1)
6-7: Pure style tweak – nothing to actionSwitching to double quotes & whitespace is fine.
drfc_manager/models/model_operations.py (1)
57-64: Changing the default delimiter may cause silent naming drift
create_clone_config()now defaults to"-"instead of the previous value (it used to be"_"). Anything that relied on the old default will quietly start producing different model names.Double-check all call sites where
delimiteris omitted to ensure this behavioural change is intentional and documented.drfc_manager/pipelines/__init__.py (1)
1-19: LGTM – style improvements onlyMulti-line imports and trailing comma in
__all__improve readability without altering behaviour.tests/conftest.py (1)
1-4: LGTMCentralising the plugins via
pytest_pluginsis clean and idiomatic.drfc_manager/types/hyperparameters.py (1)
13-26: LGTM – purely stylisticSwitching to double quotes keeps the file consistent with the rest of the codebase.
drfc_manager/config/drfc-images/docker-compose-training.yml (1)
55-55: Verify thatDEEPRACER_JOB_TYPE_ENVis defined at runtimeAdding an undefined env var causes Compose to error with “variable is not set”. Ensure it is present in the
.envfile, CI secrets, or has a default (${VAR:-default}) before merging.drfc_manager/types/docker.py (1)
26-26: LGTM! Clean enum extension for CloudWatch logging.The addition of the
CWLOGenum member follows the established pattern and naming convention. The formatting improvements with blank lines also enhance readability.README.md (1)
3-12: Excellent README improvements with professional presentation.The centered introduction section with logo and subtitle significantly enhances the project's visual appeal and professional presentation.
pytest.ini (1)
1-12: Well-structured pytest configuration following best practices.The configuration provides:
- Clear test discovery rules
- Useful output formatting with verbose mode and short tracebacks
- Custom markers for test categorization (unit, integration, slow)
- Sensible warning filtering
This setup will support the new testing infrastructure effectively.
tests/unit/utils/test_str_to_bool.py (1)
5-21: Comprehensive test coverage for str2bool function.The parameterized test effectively covers:
- All truthy string values supported by the function
- Various falsy string values
- Edge cases like
None- Boolean input types
The test cases align correctly with the function's implementation that checks for truthy values in
("yes", "true", "t", "1").drfc_manager/transformers/exceptions/base.py (3)
1-1: Good addition of type imports for improved type safety.Adding the
Optionalimport enables proper type annotations for the constructor parameters.
4-9: Excellent type hint additions for constructor parameters.The explicit
Optionaltype hints formsgandexceptionparameters improve code clarity and type safety, making it clear that both parameters can beNone.
11-18: Well-formatted string representation with clear error messaging.The
__str__method formatting enhances readability and provides clear error messages with appropriate emoji indicators for visual distinction.drfc_manager/utils/docker/exceptions/base.py (2)
1-1: Good addition of explicit Optional import.The explicit
Optionalimport improves type clarity and follows modern Python typing conventions.
7-12: Improved constructor signature with explicit typing.The multi-line formatting and explicit
Optionaltype hints enhance readability and type safety. This is a good improvement over implicit optional parameters.drfc_manager/models/storage_operations.py (2)
4-6: Good centralization of environment variable management.The introduction of
EnvVarssingleton improves configuration consistency across the codebase. This aligns well with the broader architectural refactoring mentioned in the AI summary.
12-12: Verify bucket name consistency.The replacement of
storage_client.config.bucket_namewithenv_vars.DR_LOCAL_S3_BUCKETlooks correct based on the relevant code snippets showingDR_LOCAL_S3_BUCKET: str = "tcc-experiments"in the EnvVars class.drfc_manager/models/env_operations.py (2)
2-4: Excellent centralization of environment variable management.The replacement of direct
os.environmanipulation with theEnvVarssingleton is a significant architectural improvement that enhances maintainability and consistency.
24-29: Proper use of EnvVars update and load methods.The implementation correctly uses
env_vars.update()to set multiple environment variables andenv_vars.load_to_environment()to propagate changes to the process environment. This follows the pattern established in the EnvVars class design.tests/unit/test_config_env.py (2)
5-15: Comprehensive test coverage for URL validation.The parametrized test effectively covers the key scenarios for URL scheme validation. The test logic correctly verifies that URLs without schemes get prefixed with "http://", while existing schemes are preserved.
18-22: Good validation of default configuration.The test ensures that the default server URL always has a proper scheme, which is important for configuration reliability.
tests/unit/viewers/test_stream_proxy_handlers.py (1)
5-33: Excellent async generator testing coverage.The test comprehensively covers all scenarios for the stream generator:
- Normal streaming with expected chunks
- Empty response handling
- Read error graceful handling
- Unexpected error handling
The test structure correctly uses async/await patterns and validates that errors are properly handled without propagating exceptions.
drfc_manager/helpers/files_manager.py (1)
6-6: Good addition of type hints.The explicit type hints for
Optional[int]andNonereturn type improve code clarity and type safety.tests/unit/helpers/test_files_manager.py (3)
5-8: Well-structured test with proper isolation.The test correctly uses
tmp_pathfixture for isolation and properly tests folder creation functionality.
11-20: Comprehensive file deletion test.The test properly sets up the scenario with files, executes the deletion, and verifies the expected outcome. Good use of Path operations for file creation and verification.
23-25: Good edge case testing.Testing the behavior with non-existent folders is important for robustness. The test correctly verifies that no exception is raised.
drfc_manager/utils/env_utils.py (1)
10-31: LGTM! Well-implemented subprocess environment utility.The function correctly handles environment variable preparation for subprocess execution by:
- Safely copying the current environment
- Filtering out private attributes and None values
- Converting all values to strings as required by subprocess APIs
drfc_manager/types/model_metadata.py (2)
12-14: Good consistency improvement with string formatting.Standardizing all enum values to use double quotes improves code consistency across the project.
Also applies to: 26-28, 38-39, 50-52
84-89: No breaking changes detected for DiscreteActionSpace usage.All references to
DiscreteActionSpaceare confined todrfc_manager/types/model_metadata.py, and we found no enum‐style member access (e.g.,DiscreteActionSpace.FOO) elsewhere in the codebase. The refactor to a dataclass with float fields will not break existing code.tests/fixtures/logger.py (1)
6-25: Excellent test fixture for structured logging.The fixture provides comprehensive logging configuration for tests with:
- Automatic application via
autouse=True- Proper processor chain for structured logging
- DEBUG level for complete log capture
- ConsoleRenderer for readable test output
This will greatly improve the testability of logging functionality across the codebase.
drfc_manager/utils/logging.py (1)
21-23: Good formatting improvements for better readability.The multi-line parameter formatting and consistent spacing make the code more readable and maintainable while preserving all functionality.
Also applies to: 56-58, 74-78
drfc_manager/utils/minio/storage_client.py (2)
22-24: Good formatting improvements for method signatures.The multi-line parameter formatting improves readability and consistency across the codebase.
Also applies to: 29-31, 36-40
8-10: No config property usage in StorageClient implementationsA search for
.configreferences in allStorageClientsubclasses (e.g.MinioStorageManager) returned no hits. Removing the abstractconfigproperty fromStorageClientintroduces no breaking changes in these implementations.tests/unit/utils/test_logging.py (2)
26-39: Well-structured test for log file filtering and sorting.The test correctly verifies that only files with the "drfc_" prefix are returned and that they are sorted by modification time. The use of
time.sleep(0.01)ensures different modification times for proper sorting verification.
42-56: Good coverage of both success and exception scenarios.The tests properly verify that the decorator works for both successful execution and exception propagation scenarios, ensuring the decorator doesn't interfere with normal function behavior.
drfc_manager/types/constants.py (1)
1-24: Well-organized constants with logical grouping.The constants are properly organized into logical groups with clear naming conventions. The values appear reasonable for the application domain:
- Server configuration uses standard ports
- Stream quality/resolution settings are appropriate for video streaming
- HTTP timeouts are reasonable for network operations
- Health check timeouts provide good balance between responsiveness and reliability
tests/unit/helpers/test_training_params.py (2)
19-23: Good test coverage for environment variable handling.The test properly verifies both default behavior and custom model prefix scenarios. The use of
env_vars.update()to force reload of environment variables is a good approach to ensure the singleton reflects the changes.
36-38: Good validation of YAML file structure.The test properly validates that the generated YAML file contains the expected structure and content, ensuring the function behaves correctly.
drfc_manager/evaluation/stop_evaluation_stack.py (4)
13-13: Good adoption of centralized environment variable management.The use of
EnvVars()singleton aligns with the broader refactoring effort to centralize environment variable access across the codebase.
21-21: Consistent use of EnvVars for environment access.The replacement of direct
os.environ.get()calls withenv_vars.DR_RUN_IDandenv_vars.DR_DOCKER_STYLEis consistent with the centralized approach.Also applies to: 29-29
67-72: Enhanced error handling with specific DockerError exception.The addition of specific
DockerErrorexception handling provides better error categorization and logging, which improves debugging and monitoring capabilities.
61-62: Good addition of debug logging for docker output.The conditional logging of docker output when it's not empty provides useful debugging information without cluttering logs with empty outputs.
examples/evaluation.ipynb (1)
69-93: Good addition of result display for better user feedback.The addition of displaying the evaluation result dictionary provides valuable feedback to users about the evaluation status, model name, timestamp, and log file location.
tests/fixtures/stream.py (2)
6-19: LGTM! Clean async iterator implementation.The
AsyncIteratorclass correctly implements the async iterator protocol with proper__aiter__and__anext__methods, including appropriateStopAsyncIterationhandling.
22-30: LGTM! Proper error simulation for testing.The
ErrorAsyncIteratorclass provides a clean way to simulate errors during async iteration, which is essential for testing error handling in streaming scenarios.drfc_manager/utils/docker/utilities.py (1)
3-6: LGTM! Proper integration with centralized environment management.The import of
EnvVarsand instantiation ofenv_varsaligns with the architectural refactoring to centralize environment variable management across the codebase.drfc_manager/models/data_extraction.py (3)
1-9: LGTM! Proper integration with centralized environment management.The addition of
EnvVarsimport and instantiation follows the architectural pattern established across the codebase for centralizing environment variable management.
17-17: LGTM! Improved type hints for better code clarity.The addition of explicit type hints for the
custom_reward_functionparameter and other function parameters improves code readability and IDE support.Also applies to: 42-42, 60-60, 78-78
49-50: Environment variable mapping is correct—no changes required.The default value of
DR_LOCAL_S3_CUSTOM_FILES_PREFIXis"custom_files", which matches the previouscustom_files_folderdefault indrfc_manager/config_env.py. No further updates are needed.tests/unit/viewers/test_stream_proxy_routes.py (3)
1-17: LGTM! Proper test structure and imports.The test module is well-organized with clear imports and appropriate use of pytest fixtures and async testing support.
19-31: LGTM! Comprehensive container ID validation testing.The test covers all important scenarios: valid container ID, invalid container ID, and empty container list. The edge case handling for empty containers list is particularly good.
33-50: LGTM! Proper socket connection testing with realistic scenarios.The tests cover both successful connections and connection failures, using appropriate mocking patterns for network operations.
drfc_manager/evaluation/get_compose_files.py (1)
4-12: LGTM! EnvVars singleton pattern improves maintainability.The refactoring to use the centralized
EnvVarssingleton for environment variable access is a solid architectural improvement that enhances consistency across the codebase.tests/unit/utils/test_paths.py (1)
1-78: Excellent test coverage for path utilities!The test suite is comprehensive and well-structured:
- Proper use of pytest fixtures for isolation
- Tests both success and error scenarios
- Good use of monkeypatch for environment isolation
- Clear test names and assertions
This ensures the robustness of the path utility functions that are critical for the application's directory management.
drfc_manager/utils/logging_config.py (1)
8-26: LGTM! Solid logging configuration with proper validation.The structured logging setup with validation and fallback mechanisms is well-designed. The use of structlog for structured logging is a good choice for maintainability and debugging.
drfc_manager/viewers/stream_proxy_utils.py (1)
17-46: LGTM! Robust container parsing with good error handling.The function properly handles JSON parsing errors and validates the container format. The logging integration is well-done.
drfc_manager/config_env.py (2)
6-30: LGTM! Good refactoring with improved field declarations.The explicit field declarations with defaults and descriptions improve code clarity and maintainability.
25-29: Fix validator method signature - missingselfparameter.The validator method is missing the
selfparameter, which will cause a runtime error.- @validator("server_url", pre=True) - def ensure_http_scheme(cls, v): + @validator("server_url", pre=True) + def ensure_http_scheme(cls, v):Wait, this is a class method validator, so it should use
cls. Let me check the Pydantic documentation pattern. Actually, looking at this more carefully, the validator is correct as-is for Pydantic v1 style. However, the static analysis tool is flagging this. Let me verify the correct pattern.For Pydantic v2, this should be:
- @validator("server_url", pre=True) - def ensure_http_scheme(cls, v): + @field_validator("server_url", mode="before") + @classmethod + def ensure_http_scheme(cls, v):But if using Pydantic v1 style, the current code is correct. Let me flag this for verification.
Likely an incorrect or invalid review comment.
drfc_manager/viewers/stream_proxy_handlers.py (5)
1-14: LGTM!Imports are well-organized and the logger is properly initialized using the centralized logging configuration.
15-44: Well-implemented async context manager!The StreamClient properly handles resource cleanup with separate error handling for response and client closure, ensuring both cleanup attempts are made even if one fails.
46-79: Robust streaming implementation with comprehensive error handling!The generator properly distinguishes between expected read errors (warning level) and unexpected errors (error level with traceback), while ensuring completion logging.
81-105: Clean error response generation!The function provides standardized error responses with comprehensive logging, making debugging easier.
107-140: Efficient dual-purpose logging function!Smart use of the optional status_code parameter to differentiate between start and completion logging.
drfc_manager/viewers/exceptions.py (1)
1-65: Well-structured exception hierarchy!The exception classes follow Python best practices with clear inheritance relationships and descriptive docstrings. The logical grouping makes it easy to handle specific error scenarios.
drfc_manager/helpers/training_params.py (1)
77-106: Good improvements to the function!The addition of type hints and consistent use of EnvVars singleton aligns well with the PR objectives.
tests/unit/viewers/test_stream_proxy_utils.py (1)
1-219: Excellent test coverage!The tests comprehensively cover all utility functions with both happy paths and error scenarios. Good use of pytest fixtures and clear test naming.
drfc_manager/pipelines/metrics.py (4)
10-10: Import naming improved!The removal of the underscore prefix from
adjust_composes_file_namesmakes it a public function, which is appropriate for cross-module usage.
108-121: Cleaner output suppression implementation!Using shell redirection is more efficient than the previous approach.
142-210: Good code style improvements!The formatting changes enhance readability while maintaining the same functionality.
212-238: Clean formatting improvements!The function signature is now more readable with each parameter on its own line.
drfc_manager/pipelines/evaluation.py (3)
1-20: LGTM! Clean refactoring to centralized environment management.The imports are well-organized and the singleton pattern for
EnvVarsaligns with the project-wide improvements.
173-188: LGTM! Clean model cloning implementation.Good use of the new
storage_manager.copy_model_filesmethod for precise file copying.
189-214: LGTM! Robust configuration upload with proper validation.Good validation of required environment variables before proceeding with the upload.
drfc_manager/pipelines/viewer.py (1)
620-628: Good simplification of the pipeline flow.Always stopping existing viewers before starting new ones eliminates race conditions and simplifies the logic.
examples/training.ipynb (3)
51-54: LGTM! Clean integration of new type system.Good use of the new
DiscreteActionSpaceandEnvVarsimports.
127-136: Good example of discrete action space configuration.Clear demonstration of how to configure discrete action spaces with the new API.
146-179: Excellent reward function example with steering penalty.The steering penalty implementation is well-documented and demonstrates best practices for reducing zig-zag behavior.
drfc_manager/types/env_vars.py (1)
181-198: Well-implemented singleton pattern.The singleton implementation correctly uses
__new__and tracks initialization state to ensure proper single-instance behavior.drfc_manager/utils/docker/docker_manager.py (1)
174-182: No separate single-worker RoboMaker command neededI’ve checked the repository and there isn’t a distinct launch file or invocation for single-worker training. The
distributed_training.launchscript is intentionally used in both cases, with the log messages simply distinguishing “distributed” vs. “single worker” mode. You can safely ignore the earlier suggestion to supply a different command.Likely an incorrect or invalid review comment.
| sagemaker_temp_dir = "/tmp/sagemaker" | ||
| work_directory = "/tmp/teste" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coded temp paths – make them configurable
sagemaker_temp_dir and work_directory are hard-wired to /tmp/*.
This will break on Windows and complicates testing. Read them from settings (or EnvVars) instead.
-sagemaker_temp_dir = "/tmp/sagemaker"
-work_directory = "/tmp/teste"
+sagemaker_temp_dir = settings.sagemaker_temp_dir
+work_directory = settings.work_directoryCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drfc_manager/transformers/general.py at lines 9 to 10, the variables
sagemaker_temp_dir and work_directory are hard-coded to use "/tmp/*" paths,
which is not portable and hinders testing. Modify the code to read these
directory paths from configuration settings or environment variables instead of
hardcoding them. This will make the paths configurable and compatible across
different operating systems.
| raise BaseExceptionTransformers( | ||
| f"Failed to copy S3 object from {source_object_name} to {dest_object_name}", | ||
| e, | ||
| ) |
There was a problem hiding this comment.
Preserve the traceback chain with raise … from err
Static analysis (ruff B904) is right: omitting from loses context.
- except Exception as e:
- raise BaseExceptionTransformers(
- f"Failed to copy S3 object from {source_object_name} to {dest_object_name}",
- e,
- )
+ except Exception as err:
+ raise BaseExceptionTransformers(
+ f"Failed to copy S3 object from {source_object_name} to {dest_object_name}"
+ ) from err📝 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.
| raise BaseExceptionTransformers( | |
| f"Failed to copy S3 object from {source_object_name} to {dest_object_name}", | |
| e, | |
| ) | |
| except Exception as err: | |
| raise BaseExceptionTransformers( | |
| f"Failed to copy S3 object from {source_object_name} to {dest_object_name}" | |
| ) from err |
🧰 Tools
🪛 Ruff (0.11.9)
52-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In drfc_manager/transformers/general.py around lines 52 to 55, the exception is
raised without preserving the original traceback context. Modify the raise
statement to use "raise BaseExceptionTransformers(...) from e" to maintain the
exception chaining and preserve the original error context.
| ruff = "^0.11.11" | ||
| pre-commit = "^4.2.0" | ||
| bump2version = "1.0.1" | ||
| structlog = "^25.3.0" |
There was a problem hiding this comment.
Move tooling deps to dev group – avoid shipping them to production
ruff, pre-commit, and bump2version are development-time tools; packaging them as runtime dependencies bloats install size and slows users’ pip/poetry install.
[tool.poetry.dependencies]
…
-ruff = "^0.11.11"
-pre-commit = "^4.2.0"
-bump2version = "1.0.1"
-structlog = "^25.3.0"
+structlog = "^25.3.0"
[tool.poetry.group.dev.dependencies]
…
+ruff = "^0.11.11"
+pre-commit = "^4.2.0"
+# bump2version already exists here; remove the duplicate above📝 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.
| ruff = "^0.11.11" | |
| pre-commit = "^4.2.0" | |
| bump2version = "1.0.1" | |
| structlog = "^25.3.0" | |
| [tool.poetry.dependencies] | |
| # … other runtime dependencies … | |
| structlog = "^25.3.0" | |
| [tool.poetry.group.dev.dependencies] | |
| # … existing dev dependencies … | |
| ruff = "^0.11.11" | |
| pre-commit = "^4.2.0" | |
| # bump2version already exists here; remove the duplicate above |
🤖 Prompt for AI Agents
In pyproject.toml around lines 35 to 38, move the dependencies ruff, pre-commit,
and bump2version from the main dependencies section to the dev-dependencies
group to ensure they are only installed in development environments and not
included in production installs, reducing package size and install time.
| uses: actions/cache@v3 | ||
| with: | ||
| path: .venv | ||
| key: venv-${{ runner.os }}-${{ hashFiles('**/poetry.lock') }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/cache to v4
actionlint flags v3 as outdated. GitHub now recommends actions/cache@v4, which brings performance and security improvements.
- - uses: actions/cache@v3
+ - uses: actions/cache@v4🧰 Tools
🪛 actionlint (1.7.7)
31-31: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/mypy.yml around lines 31 to 35, the actions/cache version
used is v3, which is outdated. Update the version from actions/cache@v3 to
actions/cache@v4 to comply with GitHub's current recommendations and benefit
from performance and security improvements.
| repos: | ||
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||
| rev: v0.11.11 | ||
| hooks: | ||
| - id: ruff-check | ||
| - id: ruff-format No newline at end of file |
There was a problem hiding this comment.
YAML is malformed – fix indentation and add trailing EOL
Pre-commit will ignore the file as-is. Required indentation is two spaces inside repos, and POSIX files should end with a newline.
-repos:
-- repo: https://github.com/astral-sh/ruff-pre-commit
- rev: v0.11.11
- hooks:
- - id: ruff-check
- - id: ruff-format
+repos:
+ - repo: https://github.com/astral-sh/ruff-pre-commit
+ rev: v0.11.11
+ hooks:
+ - id: ruff-check
+ - id: ruff-format
+📝 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.
| repos: | |
| - repo: https://github.com/astral-sh/ruff-pre-commit | |
| rev: v0.11.11 | |
| hooks: | |
| - id: ruff-check | |
| - id: ruff-format | |
| repos: | |
| - repo: https://github.com/astral-sh/ruff-pre-commit | |
| rev: v0.11.11 | |
| hooks: | |
| - id: ruff-check | |
| - id: ruff-format | |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 5-5: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🤖 Prompt for AI Agents
In .pre-commit-config.yaml lines 1 to 6, fix the YAML indentation by ensuring
two spaces are used inside the repos block for the repo, rev, and hooks keys and
their values. Also, add a trailing newline at the end of the file to comply with
POSIX standards and prevent pre-commit from ignoring the file.
| except Exception as e: | ||
| raise StorageError(f"Error downloading object {object_name}: {e}") | ||
| finally: |
There was a problem hiding this comment.
Add exception chaining.
except Exception as e:
- raise StorageError(f"Error downloading object {object_name}: {e}")
+ raise StorageError(f"Error downloading object {object_name}: {e}") from e📝 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.
| except Exception as e: | |
| raise StorageError(f"Error downloading object {object_name}: {e}") | |
| finally: | |
| except Exception as e: | |
| raise StorageError(f"Error downloading object {object_name}: {e}") from e | |
| finally: |
🧰 Tools
🪛 Ruff (0.11.9)
251-251: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In drfc_manager/utils/minio/storage_manager.py around lines 250 to 252, the
exception raised in the except block does not include exception chaining, which
can obscure the original traceback. Modify the raise statement to use "raise
StorageError(...) from e" to preserve the original exception context and improve
error traceability.
| except Exception as e: | ||
| raise StorageError(f"Error downloading object {object_name}: {e}") | ||
| finally: |
There was a problem hiding this comment.
Add exception chaining.
except Exception as e:
- raise StorageError(f"Error downloading object {object_name}: {e}")
+ raise StorageError(f"Error downloading object {object_name}: {e}") from e📝 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.
| except Exception as e: | |
| raise StorageError(f"Error downloading object {object_name}: {e}") | |
| finally: | |
| except Exception as e: | |
| raise StorageError(f"Error downloading object {object_name}: {e}") from e | |
| finally: |
🧰 Tools
🪛 Ruff (0.11.9)
239-239: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In drfc_manager/utils/minio/storage_manager.py around lines 238 to 240, the
exception raised in the except block does not include exception chaining. Modify
the raise statement to use "raise ... from e" syntax to preserve the original
exception context when raising StorageError.
| DR_LOCAL_S3_METRICS_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}/metrics" | ||
| DR_UPLOAD_S3_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}" | ||
| DR_MINIO_URL: str = f"http://{DR_MINIO_HOST}:{DR_MINIO_PORT}" | ||
| DR_MINIO_URL_API: str = f"http://{DR_MINIO_HOST_API}:{DR_MINIO_PORT}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
F-string fields may not update when referenced fields change.
These fields use f-strings that reference other fields, but they're evaluated at class definition time. If the referenced fields (e.g., DR_LOCAL_S3_MODEL_PREFIX, DR_MINIO_HOST) are updated via the update() method, these f-string fields won't reflect the changes.
Consider using properties or methods instead:
@property
def DR_LOCAL_S3_METRICS_PREFIX(self) -> str:
return f"{self.DR_LOCAL_S3_MODEL_PREFIX}/metrics"🏁 Script executed:
#!/bin/bash
# Check if these f-string fields are updated anywhere in the codebase
rg -A 2 -B 2 "DR_LOCAL_S3_METRICS_PREFIX|DR_UPLOAD_S3_PREFIX|DR_MINIO_URL|DR_MINIO_URL_API" --type py | grep -E "(update|setattr)" Length of output: 132
🏁 Script executed:
#!/bin/bash
set -eux
# Show class and method definitions in env_vars.py
grep -n "class " -n drfc_manager/types/env_vars.py
grep -n "def " -n drfc_manager/types/env_vars.py
# Display the first 200 lines for context
sed -n '1,200p' drfc_manager/types/env_vars.pyLength of output: 7812
🏁 Script executed:
#!/bin/bash
set -eux
# Display lines around the update() method to inspect its implementation
sed -n '190,260p' drfc_manager/types/env_vars.pyLength of output: 2952
Convert computed f-string defaults into dynamic properties
The DR_LOCAL_S3_METRICS_PREFIX, DR_UPLOAD_S3_PREFIX, DR_MINIO_URL and DR_MINIO_URL_API fields are defined with f-strings at class definition time (lines 86–89 in drfc_manager/types/env_vars.py). Because those f-strings are evaluated once when the class is created, calling EnvVars.update() to change DR_LOCAL_S3_MODEL_PREFIX, DR_MINIO_HOST, etc., will not recompute the dependent prefixes/URLs.
To ensure these values always reflect the latest settings, convert them to read-only @property methods. For example:
- DR_LOCAL_S3_METRICS_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}/metrics"
- DR_UPLOAD_S3_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}"
- DR_MINIO_URL: str = f"http://{DR_MINIO_HOST}:{DR_MINIO_PORT}"
- DR_MINIO_URL_API: str = f"http://{DR_MINIO_HOST_API}:{DR_MINIO_PORT}"
+ @property
+ def DR_LOCAL_S3_METRICS_PREFIX(self) -> str:
+ return f"{self.DR_LOCAL_S3_MODEL_PREFIX}/metrics"
+
+ @property
+ def DR_UPLOAD_S3_PREFIX(self) -> str:
+ return self.DR_LOCAL_S3_MODEL_PREFIX
+
+ @property
+ def DR_MINIO_URL(self) -> str:
+ return f"http://{self.DR_MINIO_HOST}:{self.DR_MINIO_PORT}"
+
+ @property
+ def DR_MINIO_URL_API(self) -> str:
+ return f"http://{self.DR_MINIO_HOST_API}:{self.DR_MINIO_PORT}"This guarantees that whenever you call env.update(DR_LOCAL_S3_MODEL_PREFIX="new") or change hosts/ports, the dependent values always reflect the current state.
📝 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.
| DR_LOCAL_S3_METRICS_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}/metrics" | |
| DR_UPLOAD_S3_PREFIX: str = f"{DR_LOCAL_S3_MODEL_PREFIX}" | |
| DR_MINIO_URL: str = f"http://{DR_MINIO_HOST}:{DR_MINIO_PORT}" | |
| DR_MINIO_URL_API: str = f"http://{DR_MINIO_HOST_API}:{DR_MINIO_PORT}" | |
| @property | |
| def DR_LOCAL_S3_METRICS_PREFIX(self) -> str: | |
| return f"{self.DR_LOCAL_S3_MODEL_PREFIX}/metrics" | |
| @property | |
| def DR_UPLOAD_S3_PREFIX(self) -> str: | |
| return self.DR_LOCAL_S3_MODEL_PREFIX | |
| @property | |
| def DR_MINIO_URL(self) -> str: | |
| return f"http://{self.DR_MINIO_HOST}:{self.DR_MINIO_PORT}" | |
| @property | |
| def DR_MINIO_URL_API(self) -> str: | |
| return f"http://{self.DR_MINIO_HOST_API}:{self.DR_MINIO_PORT}" |
🤖 Prompt for AI Agents
In drfc_manager/types/env_vars.py around lines 86 to 89, the fields
DR_LOCAL_S3_METRICS_PREFIX, DR_UPLOAD_S3_PREFIX, DR_MINIO_URL, and
DR_MINIO_URL_API are set using f-strings at class definition time, causing them
not to update when dependent variables change. To fix this, convert these fields
into read-only @property methods that return the computed strings dynamically
based on the current values of DR_LOCAL_S3_MODEL_PREFIX, DR_MINIO_HOST,
DR_MINIO_PORT, and DR_MINIO_HOST_API. This ensures the values always reflect the
latest settings after any update calls.
| ) -> subprocess.CompletedProcess: | ||
| logger.debug(f"Executing: {' '.join(command)}") | ||
| try: | ||
| env = get_subprocess_env(self.env_vars) |
There was a problem hiding this comment.
Use the provided env parameter if available
The method accepts an env parameter but always overrides it with get_subprocess_env. This makes the parameter useless.
Apply this fix:
-env = get_subprocess_env(self.env_vars)
+if env is None:
+ env = get_subprocess_env(self.env_vars)📝 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.
| env = get_subprocess_env(self.env_vars) | |
| if env is None: | |
| env = get_subprocess_env(self.env_vars) |
🤖 Prompt for AI Agents
In drfc_manager/utils/docker/docker_manager.py at line 39, the env parameter
passed to the method is ignored and always overridden by
get_subprocess_env(self.env_vars). Modify the code to use the provided env
parameter if it is not None; otherwise, fall back to
get_subprocess_env(self.env_vars). This will make the env parameter effective
and allow callers to specify a custom environment.
|
|
||
| def _run_command(self, command: List[str], check: bool = True, capture: bool = True) -> subprocess.CompletedProcess: | ||
| self.env_vars = EnvVars() | ||
| if env_vars: |
There was a problem hiding this comment.
Potential singleton pattern violation
Creating a new instance of EnvVars() here could break the singleton pattern. Since EnvVars implements a singleton pattern (as seen in the __new__ method in the relevant code snippets), you should use the existing instance rather than creating a new one.
Consider this fix:
-self.env_vars = EnvVars()
+self.env_vars = env_vars if env_vars else EnvVars()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drfc_manager/utils/docker/docker_manager.py at line 24, the code creates a
new instance of EnvVars which violates its singleton pattern. Instead of
instantiating EnvVars directly, retrieve the existing singleton instance by
accessing the class-level instance or using the appropriate method provided by
EnvVars to get the singleton. Replace the direct call to EnvVars() with this
approach to ensure the singleton pattern is respected.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
Documentation