feat(clp-package): Add option for specifying archive compression level and update max compression level to 19.#774
Conversation
WalkthroughThis pull request adds support for a configurable compression level across multiple components. The Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant YAML as clp-config.yml
participant JobConfig as Job Config Loader
participant AO as ArchiveOutput
participant CT as CompressionTask
User->>YAML: Request configuration (compression_level)
YAML-->>JobConfig: Provide compression_level (default: 3)
JobConfig->>AO: Initialize ArchiveOutput with compression_level
AO->>AO: Validate compression_level (1-19)
JobConfig->>CT: Pass compression_level to command generator
CT->>CT: Include --compression-level argument in CLI command
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/job-orchestration/job_orchestration/scheduler/job_config.py (2)
47-47: Consider adding a validator for the compression_level attributeThe new
compression_levelattribute should have constraints to ensure it's within the valid range of 1 to 9, similar to the validator implemented in theArchiveOutputclass. This would provide consistent validation across the codebase.+ @validator("compression_level") + def validate_compression_level(cls, field): + if field < 1 or field > 9: + raise ValueError("compression_level must be a value from 1 to 9") + return field
47-47: Consider adding a default value to the compression_level attributeTo maintain consistency with the
ArchiveOutputclass which uses a default of 3, consider adding the same default value here.- compression_level: int + compression_level: int = 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(2 hunks)components/job-orchestration/job_orchestration/scheduler/job_config.py(1 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
components/package-template/src/etc/clp-config.yml
[error] 86-86: trailing spaces
(trailing-spaces)
🪛 GitHub Check: lint-check (ubuntu-latest)
components/package-template/src/etc/clp-config.yml
[failure] 86-86:
86:2 [trailing-spaces] trailing spaces
🪛 GitHub Actions: clp-lint
components/package-template/src/etc/clp-config.yml
[warning] 86-86: Trailing spaces found in the file.
🔇 Additional comments (5)
components/package-template/src/etc/clp-config.yml (1)
87-88: The compression level configuration looks goodThe comment clearly describes the valid range and purpose of the compression level setting, and the default value of 3 provides a good balance between compression ratio and speed.
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (2)
143-143: The compression level parameter is properly added to the clp commandThe option to specify compression level has been correctly added to the command generation for the clp utility.
184-184: The compression level parameter is properly added to the clp-s commandThe option to specify compression level has been correctly added to the command generation for the clp-s utility.
components/clp-py-utils/clp_py_utils/clp_config.py (2)
446-446: Default compression level looks goodSetting the default compression level to 3 provides a good balance between compression ratio and speed, matching the YAML configuration.
472-476: Validator for compression level is properly implementedThe validator correctly ensures that the compression level is within the valid range (1-9) as described in the requirements.
kirkrodrigues
left a comment
There was a problem hiding this comment.
Nice work. One minor request.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/core/src/clp/clp/CommandLineArguments.cpp(1 hunks)components/core/src/clp_s/CommandLineArguments.cpp(1 hunks)components/core/src/glt/glt/CommandLineArguments.cpp(1 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/package-template/src/etc/clp-config.yml
- components/clp-py-utils/clp_py_utils/clp_config.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/clp/CommandLineArguments.cppcomponents/core/src/glt/glt/CommandLineArguments.cppcomponents/core/src/clp_s/CommandLineArguments.cpp
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
components/core/src/glt/glt/CommandLineArguments.cpp (1)
281-281: Update compression level range in help textThe help text has been updated to reflect an extended range of compression levels from 1-9 to 1-19, which provides users with more granular control over the compression ratio vs. speed trade-off. This aligns with the broader changes made across the codebase as mentioned in the summary.
Note that there appears to be a slight discrepancy between the PR objectives (which mentions levels 1-9) and this implementation (which supports levels 1-19). Verify that this extended range is intentional and document this capability appropriately in user-facing documentation.
components/core/src/clp/clp/CommandLineArguments.cpp (1)
360-360: Compression level range has been updated.The description for the compression level option has been updated to reflect an expanded range (from 1-9 to 1-19), allowing for more fine-grained control over the compression ratio vs. speed trade-off.
clp-config.ymlclp-config.yml and change max compression level to 19.
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(clp-package): Add option for specifying archive compression level and update max compression level to 19.
clp-config.yml and change max compression level to 19.
Done. |
…l and update max compression level to 19. (y-scope#774) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Description
This PR adds the
compression_leveloption toarchive_outputinclp-config.yml. Compression level can be set to an integer value from 1 (lowest compression ratio, highest speed) to 19 (highest compression ratio, lowest speed).Checklist
breaking change.
Validation performed
Summary by CodeRabbit