Skip to content

fix: print secs as numeric in jsonl benchmark format#3814

Merged
johanneskoester merged 1 commit intosnakemake:mainfrom
fgvieira:bench_s_numeric
Oct 29, 2025
Merged

fix: print secs as numeric in jsonl benchmark format#3814
johanneskoester merged 1 commit intosnakemake:mainfrom
fgvieira:bench_s_numeric

Conversation

@fgvieira
Copy link
Copy Markdown
Contributor

@fgvieira fgvieira commented Oct 28, 2025

Currently, benchmarks in jsonl format have the s field as a string:

{
  "cpu_time": 0.0,
  "cpu_usage": 0.0,
  "h:m:s": "0:00:10",
  "input_size_mb": {
    "a.in": 5.834963798522949
  },
  "io_in": 0.0,
  "io_out": 0.0,
  "jobid": 0,
  "max_pss": 0.6533203125,
  "max_rss": 3.7109375,
  "max_uss": 0.46875,
  "max_vms": 19.86328125,
  "mean_load": 0.0,
  "params": {},
  "resources": {
    "_cores": 1,
    "_nodes": 1,
    "tmpdir": "/tmp"
  },
  "rule_name": "all",
  "s": "10.0083",
  "threads": 1,
  "wildcards": {}
}

This PR fixes this by keeping it as a float.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Summary by CodeRabbit

  • Bug Fixes
    • Corrected benchmark export formats: Running time values in TSV output now display with updated precision, and JSON exports render running times as numeric values for proper data type handling.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 28, 2025

📝 Walkthrough

Walkthrough

BenchmarkRecord.get_benchmarks now returns running_time as a numeric float rounded to 4 decimal places instead of a pre-formatted string, altering how the first benchmark field is represented in TSV and JSON output formats.

Changes

Cohort / File(s) Summary
Benchmark return type change
src/snakemake/benchmark.py
Modified BenchmarkRecord.get_benchmarks to return running_time as a rounded float instead of formatted string, affecting downstream TSV and JSON serialization behavior

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that the type change from string to float is intentional and doesn't break existing consumers
  • Check TSV and JSON output formatting behavior with the new float type to ensure decimal precision aligns with expectations
  • Review any documentation or tests that may need updates to reflect the new numeric type

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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
Title Check ✅ Passed The PR title "fix: print secs as numeric in jsonl benchmark format" is directly related to the main change in the changeset. According to the raw summary, the modification changes the first record element (running_time) from a formatted string to a numeric rounded value, which affects JSON output rendering. The title clearly and specifically identifies this core change—converting the seconds field to numeric format in JSON benchmark output—making it fully representative of the primary modification without being vague or overly broad.
Description Check ✅ Passed The PR description follows the required template structure and includes all necessary sections. It provides a clear explanation of the change with a concrete example showing the before state (seconds field as a string "s": "10.0083") and states the fix keeps it as a float. The QC section is present with both required checkboxes marked as completed: the author confirms test case coverage and indicates documentation updates are addressed or not necessary. The description is mostly complete and sufficiently detailed to understand the change's purpose and scope.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8b8620 and fa46475.

📒 Files selected for processing (1)
  • src/snakemake/benchmark.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • src/snakemake/benchmark.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). (45)
  • GitHub Check: tests (10, macos-latest, py313)
  • GitHub Check: tests (10, windows-2022, py313)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (9, macos-latest, py313)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (9, windows-2022, py313)
  • GitHub Check: tests (8, windows-2022, py313)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (6, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-2022, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (5, macos-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (4, windows-2022, py313)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-2022, py313)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (2, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (1, windows-2022, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
🔇 Additional comments (1)
src/snakemake/benchmark.py (1)

176-176: The review comment is accurate and the concern is valid—confirm whether TSV precision loss is acceptable.

The verification confirms all claims in the review comment:

  • JSON fix (correct): Changing from string "10.0083" to numeric 10.0083 achieves the intended JSONL format goal.
  • TSV side effect (confirmed): The to_tsv_str function now receives a float instead of a string, triggering the isinstance(x, float) check and applying .2f formatting (2 decimals) instead of preserving the original 4 decimals. This reduces precision from "10.0083" to "10.01".
  • Unintended consequence (valid concern): Since the PR title specifies "jsonl benchmark format," this TSV behavior change appears to be a side effect rather than an intended target.

The review comment correctly asks for developer verification of whether the TSV precision loss is acceptable. If 4-decimal precision is required for TSV consumers, the to_tsv_str function or the value type in get_benchmarks needs adjustment.


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.

@johanneskoester johanneskoester merged commit 395a5e6 into snakemake:main Oct 29, 2025
60 checks passed
@fgvieira fgvieira deleted the bench_s_numeric branch October 29, 2025 12:27
johanneskoester pushed a commit that referenced this pull request Nov 4, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.13.5](v9.13.4...v9.13.5)
(2025-11-04)


### Bug Fixes

* cache wrapper files and wait for them in case of shared filesystem for
sources ([#3809](#3809))
([498fff7](498fff7))
* correctly handle meta-wrapper tag replacement depending on the used
snakemake-wrapper release
([#3826](#3826))
([8d46a4a](8d46a4a))
* ensure that flags are properly considered for input files before
applying path modifiers (i.e. default storage providers)
([#3813](#3813))
([d3bfe32](d3bfe32))
* ensure that tokens are not leaked when paths or uris of source files
are logged ([#3821](#3821))
([a217e50](a217e50))
* print secs as numeric in jsonl benchmark format
([#3814](#3814))
([395a5e6](395a5e6))
* revert breaking change in 9.11.9 disallowing empty input files even
when unused
([#3810](#3810))
([83c05cc](83c05cc))
* shorten report ids (thus dir names) in order to avoid issues with path
length on windows
([#3822](#3822))
([b24d971](b24d971))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
<!--Add a description of your PR here-->

Currently, benchmarks in `jsonl` format have the `s` field as a string:
```
{
  "cpu_time": 0.0,
  "cpu_usage": 0.0,
  "h:m:s": "0:00:10",
  "input_size_mb": {
    "a.in": 5.834963798522949
  },
  "io_in": 0.0,
  "io_out": 0.0,
  "jobid": 0,
  "max_pss": 0.6533203125,
  "max_rss": 3.7109375,
  "max_uss": 0.46875,
  "max_vms": 19.86328125,
  "mean_load": 0.0,
  "params": {},
  "resources": {
    "_cores": 1,
    "_nodes": 1,
    "tmpdir": "/tmp"
  },
  "rule_name": "all",
  "s": "10.0083",
  "threads": 1,
  "wildcards": {}
}
```

This PR fixes this by keeping it as a float.

### QC
<!-- Make sure that you can tick the boxes below. -->

* [x] The PR contains a test case for the changes or the changes are
already covered by an existing test case.
* [x] The documentation (`docs/`) is updated to reflect the changes or
this is not necessary (e.g. if the change does neither modify the
language nor the behavior or functionalities of Snakemake).


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Corrected benchmark export formats: Running time values in TSV output
now display with updated precision, and JSON exports render running
times as numeric values for proper data type handling.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.13.5](snakemake/snakemake@v9.13.4...v9.13.5)
(2025-11-04)


### Bug Fixes

* cache wrapper files and wait for them in case of shared filesystem for
sources ([snakemake#3809](snakemake#3809))
([498fff7](snakemake@498fff7))
* correctly handle meta-wrapper tag replacement depending on the used
snakemake-wrapper release
([snakemake#3826](snakemake#3826))
([8d46a4a](snakemake@8d46a4a))
* ensure that flags are properly considered for input files before
applying path modifiers (i.e. default storage providers)
([snakemake#3813](snakemake#3813))
([d3bfe32](snakemake@d3bfe32))
* ensure that tokens are not leaked when paths or uris of source files
are logged ([snakemake#3821](snakemake#3821))
([a217e50](snakemake@a217e50))
* print secs as numeric in jsonl benchmark format
([snakemake#3814](snakemake#3814))
([395a5e6](snakemake@395a5e6))
* revert breaking change in 9.11.9 disallowing empty input files even
when unused
([snakemake#3810](snakemake#3810))
([83c05cc](snakemake@83c05cc))
* shorten report ids (thus dir names) in order to avoid issues with path
length on windows
([snakemake#3822](snakemake#3822))
([b24d971](snakemake@b24d971))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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