fix: convert parameters so they can be serialized#2925
fix: convert parameters so they can be serialized#2925johanneskoester merged 8 commits intosnakemake:mainfrom
Conversation
|
|
I think this has become obselete with #3175. Please reopen if I am wrong. |
|
Yes, it seems it has been fixed, but then we can just merged the PR to include the extra tests, no? |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Outside diff range and nitpick comments (1)
tests/test_nonstr_params/Snakefile (1)
10-12: Consider using a platform-independent temporary pathThe hardcoded path "/tmp" might cause issues on Windows systems. Consider using a more portable solution.
- path=Path("/tmp"), + path=Path(tempfile.gettempdir()),Don't forget to add the import:
import tempfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/test_nonstr_params/expected-results/test.outis excluded by!**/*.out
📒 Files selected for processing (2)
tests/test_nonstr_params/Snakefile(1 hunks)tests/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/tests.py (1)
Pattern **/*.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.
🔇 Additional comments (4)
tests/test_nonstr_params/Snakefile (3)
1-2: LGTM: Required imports added correctly
The imports for numpy and Path are necessary for the test case and are properly defined.
7-8: LGTM: Benchmark configuration added
The benchmark configuration is correctly set up to test the serialization functionality.
1-15: Verify if changes are still needed after PR #3175
Based on the PR comments, there might be an overlap with PR #3175. While these changes look correct, we should verify if they're still necessary.
#!/bin/bash
# Description: Check for related changes in PR #3175
# Look for similar parameter serialization fixes
# Check for related files in the main branch
gh pr view 3175 --json files -q '.files[].path' | grep -i "param\|serial"
# Look for related code changes
gh pr view 3175 --json files -q '.files[].patch' | grep -i "param\|serial\|json"tests/tests.py (1)
464-466: LGTM: Changes align with PR objectives.
The modifications appropriately address the serialization issues:
- The
@skip_on_windowsdecorator is correctly added as path handling differs on Windows. - The
benchmark_extended=Trueparameter enables extended benchmarking, which helps test the serialization of non-string parameters.
|
🤖 I have created a release *beep* *boop* --- ## [8.26.0](v8.25.5...v8.26.0) (2024-12-23) ### Features * add helpers for deferred input/output etc. item access ([#2927](#2927)) ([2cca9bc](2cca9bc)) ### Bug Fixes * convert parameters so they can be serialized ([#2925](#2925)) ([9e653fb](9e653fb)) * correct formatting of R preamble ([#2425](#2425)) ([5380cae](5380cae)) * fix modification checks for scripts and and notebooks containing wildcards or params in their paths ([#2751](#2751)) ([773568d](773568d)) * Improved handling of missing output files in group job postprocessing, accounting for temporary files. ([#1765](#1765)) ([bac06ba](bac06ba)) * mtime of script or notebook not triggering workflow without metadata ([#3148](#3148)) ([e8a0b83](e8a0b83)) * Pass `host` attribute to `GitlabFile` instantiation within class methods ([#3155](#3155)) ([9ef52de](9ef52de)) * problem with spaces in path ([#3236](#3236)) ([2d08c63](2d08c63)) * require current yte release which contains an important bug fix for cases where numpy/pandas data is passed to templates ([#3227](#3227)) ([c3339da](c3339da)) * rerun jobs if previously failed but rule was changed afterwards (thanks to [@laf070810](https://github.com/laf070810) for bringing this up) ([#3237](#3237)) ([1dc0084](1dc0084)) * use relpath for configfiles added to the source archive (thanks to [@sposadac](https://github.com/sposadac) for the initial solution) ([#3240](#3240)) ([bff3844](bff3844)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
<!--Add a description of your PR here--> Some parameter types (e.g. `Path` or `np`) are not serializable when requesting extended benchmarks (and maybe in other other places snakemake#1425): ```bash $ snakemake test.out -j 1 --benchmark-extended -F Assuming unrestricted shared filesystem usage. Building DAG of jobs... Using shell: /usr/bin/bash Provided cores: 1 (use --cores to define parallelism) Rules claiming more threads will be scaled down. Job stats: job count ----- ------- 1 1 total 1 Select jobs to execute... Execute 1 jobs... [Thu Jul 11 11:07:47 2024] localrule 1: output: test.out jobid: 0 benchmark: test.jsonl reason: Forced execution resources: tmpdir=/tmp WorkflowError: TypeError: Object of type int64 is not JSON serializable [Thu Jul 11 11:07:50 2024] Error in rule 1: jobid: 0 output: test.out Exiting because a job execution failed. Look above for error message WorkflowError: At least one job did not complete successfully. [Thu Jul 11 11:07:50 2024] Error in rule 1: jobid: 0 output: test.out Shutting down, this might take some time. Exiting because a job execution failed. Look above for error message Complete log: .snakemake/log/2024-07-11T110746.832532.snakemake.log WorkflowError: At least one job did not complete successfully. ``` Not sure why, but the tests all pass! :confused: ### 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 - **New Features** - Added new parameters for testing: `path` and `np` to enhance test configurations. - Introduced performance benchmarking for the `test_nonstr_params` function. - **Bug Fixes** - Updated the test to skip on Windows platforms, addressing compatibility issues. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Johannes Köster <johannes.koester@tu-dortmund.de>
🤖 I have created a release *beep* *boop* --- ## [8.26.0](snakemake/snakemake@v8.25.5...v8.26.0) (2024-12-23) ### Features * add helpers for deferred input/output etc. item access ([snakemake#2927](snakemake#2927)) ([2cca9bc](snakemake@2cca9bc)) ### Bug Fixes * convert parameters so they can be serialized ([snakemake#2925](snakemake#2925)) ([9e653fb](snakemake@9e653fb)) * correct formatting of R preamble ([snakemake#2425](snakemake#2425)) ([5380cae](snakemake@5380cae)) * fix modification checks for scripts and and notebooks containing wildcards or params in their paths ([snakemake#2751](snakemake#2751)) ([773568d](snakemake@773568d)) * Improved handling of missing output files in group job postprocessing, accounting for temporary files. ([snakemake#1765](snakemake#1765)) ([bac06ba](snakemake@bac06ba)) * mtime of script or notebook not triggering workflow without metadata ([snakemake#3148](snakemake#3148)) ([e8a0b83](snakemake@e8a0b83)) * Pass `host` attribute to `GitlabFile` instantiation within class methods ([snakemake#3155](snakemake#3155)) ([9ef52de](snakemake@9ef52de)) * problem with spaces in path ([snakemake#3236](snakemake#3236)) ([2d08c63](snakemake@2d08c63)) * require current yte release which contains an important bug fix for cases where numpy/pandas data is passed to templates ([snakemake#3227](snakemake#3227)) ([c3339da](snakemake@c3339da)) * rerun jobs if previously failed but rule was changed afterwards (thanks to [@laf070810](https://github.com/laf070810) for bringing this up) ([snakemake#3237](snakemake#3237)) ([1dc0084](snakemake@1dc0084)) * use relpath for configfiles added to the source archive (thanks to [@sposadac](https://github.com/sposadac) for the initial solution) ([snakemake#3240](snakemake#3240)) ([bff3844](snakemake@bff3844)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>



Some parameter types (e.g.
Pathornp) are not serializable when requesting extended benchmarks (and maybe in other other places #1425):Not sure why, but the tests all pass! 😕
QC
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
New Features
pathandnpto enhance test configurations.test_nonstr_paramsfunction.Bug Fixes