Skip to content

fix: add default json function to benchmarks#4128

Merged
johanneskoester merged 7 commits into
snakemake:mainfrom
fgvieira:bench_json_posixpath
May 29, 2026
Merged

fix: add default json function to benchmarks#4128
johanneskoester merged 7 commits into
snakemake:mainfrom
fgvieira:bench_json_posixpath

Conversation

@fgvieira

@fgvieira fgvieira commented Mar 24, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

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).
  • I, as a human being, have checked each line of code in this pull request

AI-assistance disclosure

I used AI assistance for:

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Summary by CodeRabbit

  • Bug Fixes

    • Improved benchmark output serialization so non-standard values are converted to strings instead of causing errors.

  • Tests

    • Updated benchmark tests and expected results to reflect extended parameter contents in benchmark records.

@coderabbitai

coderabbitai Bot commented Mar 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !CHANGELOG.md

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b99575a9-d68e-469b-b119-4e1096b0aea5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds default=str to JSON serialization in BenchmarkRecord.to_json() so non-JSON-serializable benchmark values are stringified. Tests were updated to include additional non-standard params (a Path and a string) and expected JSONL output adjusted accordingly.

Changes

Cohort / File(s) Summary
JSON Serialization Enhancement
src/snakemake/benchmark.py		 Added default=str to the json.dumps() call in BenchmarkRecord.to_json() so non-serializable benchmark values are converted to strings during serialization.
Test Updates
tests/test_benchmark_jsonl/Snakefile, tests/test_benchmark_jsonl/expected-results/test.benchmark_shell.jsonl		 Extended bench_shell rule params with path = Path() and string = "foobar"; updated expected JSONL to include these params serialized as strings/values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change: adding a default JSON function to handle non-JSON-serializable benchmark values.
Description check ✅ Passed The description follows the template with all QC checkboxes properly checked and the AI-assistance section completed, confirming test coverage and manual code review.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@fgvieira fgvieira self-assigned this Mar 24, 2026
@fgvieira fgvieira added the bug Something isn't working label Mar 24, 2026
@fgvieira fgvieira moved this to In review in Snakemake Hackathon 2026 Mar 24, 2026
coderabbitai[bot]
coderabbitai Bot reviewed Mar 24, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_benchmark_jsonl/Snakefile`:
- Around line 19-20: The Snakefile uses Path() but never imports it, causing a
NameError when Snakemake parses the workflow; add an explicit import such as
"from pathlib import Path" at the top of the Snakefile so Path() is defined and
the test remains self-contained (look for the Path() usage in the
test_benchmark_jsonl/Snakefile).
ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d019b40b-2605-4794-9a3f-712c317fa304

📥 Commits

Reviewing files that changed from the base of the PR and between 2432a6c and 35ad879.

📒 Files selected for processing (3)
  • src/snakemake/benchmark.py
  • tests/test_benchmark_jsonl/Snakefile
  • tests/test_benchmark_jsonl/expected-results/test.benchmark_shell.jsonl

Comment thread tests/test_benchmark_jsonl/Snakefile
@cademirch cademirch self-requested a review March 30, 2026 17:56
@johanneskoester johanneskoester merged commit 41fab22 into snakemake:main May 29, 2026
110 of 112 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Snakemake Hackathon 2026 May 29, 2026
@snakemake-bot snakemake-bot mentioned this pull request May 29, 2026
Merged
@fgvieira fgvieira deleted the bench_json_posixpath branch May 29, 2026 12:47
johanneskoester pushed a commit that referenced this pull request May 29, 2026
@snakemake-bot
chore(main): release 9.21.1 (#4200)
5b5482e 
🤖 I have created a release *beep* *boop*
---


##
[9.21.1](v9.21.0...v9.21.1)
(2026-05-29)


### Bug Fixes

* add default json function to benchmarks
([#4128](#4128))
([41fab22](41fab22))
* do not rerun when checkpoint job missing but downstream file exists
([#4124](#4124))
([a060b93](a060b93))
* ensure that error logs contain all available details
([#4183](#4183))
([74a86e9](74a86e9))
* handle missing pss attribute in benchmark on Windows
([#4160](#4160))
([da52080](da52080))
* implement Resources.setdefault
([#3968](#3968))
([2413e99](2413e99))
* reporting remote nodes number
([#3978](#3978))
([8c534f0](8c534f0))
* resolve pathvars before constructing storage queries
([#3969](#3969))
([bd15237](bd15237))
* storage temp() file cleanup with RemoteProvider
([#4189](#4189))
([898bad1](898bad1))
* tolerate FileNotFoundError in drop_iocache
([#4153](#4153))
([#4191](#4191))
([ce26b28](ce26b28))


### Documentation

* Added guide on debugging workflows
([#4029](#4029))
([3d052ae](3d052ae))
* **cli:** Remove broken ref bold markup
([#4204](#4204))
([1200ebf](1200ebf))
* remove duplicated resources attribute in rules.rst
([#4190](#4190))
([6c8ecdd](6c8ecdd))
* **rules:** Update script type hint advice
([#4193](#4193))
([6108712](6108712))

---
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

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@johanneskoester johanneskoester johanneskoester approved these changes

@cademirch cademirch cademirch approved these changes

Assignees

@fgvieira fgvieira

Labels

bug Something isn't working

Projects

Snakemake Hackathon 2026
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fgvieira @johanneskoester @cademirch