Skip to content

Conversation

gibber9809
Copy link
Contributor

@gibber9809 gibber9809 commented Sep 23, 2025

Description

This PR follows up on #1176 by making lossless float retention the default behaviour for clp-s. This should be a fairly easy win, since our benchmarks on that PR showed that the compression ratio and performance overhead were minimal.

We may also want to change #1298 at the same time (or this PR if #1298 gets merged first) since the --retain-float-format flag will no longer be required.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Manually validated that compression behaviour with and without the --no-retain-float-format flag work as expected.

Summary by CodeRabbit

  • Refactor

    • Renamed the float-retention CLI flag to --no-retain-float-format, inverting its behaviour; the public getter still indicates whether float format is retained but now derives from the negated flag.
    • Updated option description to: “Do not store extra information to losslessly decompress floats.”
  • Chores

    • Updated job orchestration to remove the old flag and use the new CLI option.

@gibber9809 gibber9809 requested review from a team and wraymo as code owners September 23, 2025 15:07
Copy link
Contributor

coderabbitai bot commented Sep 23, 2025

Walkthrough

Rename clp-s CLI option from --retain-float-format to --no-retain-float-format, invert the internal boolean to m_no_retain_float_format and have the getter return its negation; remove the old flag from the Python command construction. No other control flow changes.

Changes

Cohort / File(s) Summary
clp-s CLI flag inversion (C++)
components/core/src/clp_s/CommandLineArguments.cpp, components/core/src/clp_s/CommandLineArguments.hpp
Rename internal flag to m_no_retain_float_format; change option name/description to --no-retain-float-format; get_retain_float_format() now returns !m_no_retain_float_format; default remains false.
Job orchestration command update (Python)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
Remove --retain-float-format from clp-s command invocation; no other command or control flow changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and succinctly states the primary change—making lossless float retention the default for clp-s—and matches the changeset (flag rename and flipped default semantics). It is component-scoped, concise, and readable, so a teammate scanning history will understand the main intent. The phrasing avoids noise and clearly identifies the feature change.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ea660a and a34b2a5.

📒 Files selected for processing (1)
  • components/job-orchestration/job_orchestration/executor/compress/compression_task.py (0 hunks)
💤 Files with no reviewable changes (1)
  • components/job-orchestration/job_orchestration/executor/compress/compression_task.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: manylinux_2_28-x86_64-static-linked-bins
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: package-image
  • GitHub Check: musllinux_1_2-x86_64-static-linked-bins
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: lint-check (ubuntu-24.04)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 774a106 and 1ea660a.

📒 Files selected for processing (3)
  • components/core/src/clp_s/CommandLineArguments.cpp (1 hunks)
  • components/core/src/clp_s/CommandLineArguments.hpp (2 hunks)
  • components/job-orchestration/job_orchestration/executor/compress/compression_task.py (0 hunks)
💤 Files with no reviewable changes (1)
  • components/job-orchestration/job_orchestration/executor/compress/compression_task.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

  • Prefer false == <expression> rather than !<expression>.

Files:

  • components/core/src/clp_s/CommandLineArguments.hpp
  • components/core/src/clp_s/CommandLineArguments.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: package-image
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: musllinux_1_2-x86_64-static-linked-bins
  • GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
  • GitHub Check: manylinux_2_28-x86_64-static-linked-bins
  • GitHub Check: build-macos (macos-15, true)
  • GitHub Check: build-macos (macos-15, false)
  • GitHub Check: build-macos (macos-14, true)
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/core/src/clp_s/CommandLineArguments.hpp (1)

105-107: Getter inversion LGTM.

Public API preserved while default behaviour flips; style aligns with guidelines.

components/core/src/clp_s/CommandLineArguments.cpp (1)

248-251: Docs/help follow‑through — no occurrences of --retain-float-format found.

rg only matched the new flag declaration (components/core/src/clp_s/CommandLineArguments.cpp:248 — "no-retain-float-format") and a design-doc filename reference (components/core/src/clp_s/FloatFormatEncoding.hpp:16 → docs/src/dev-guide/design-retain-float-format.md). No other references to --retain-float-format detected; update any external docs/orchestration scripts outside this repo if needed.

@kirkrodrigues kirkrodrigues merged commit 17ec517 into y-scope:main Sep 23, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants