fix: mem and disk inference fixes#3040
Conversation
Mem/disk mib fix and unit test
WalkthroughThe changes made primarily involve adjustments to the Snakemake framework regarding resource handling and conversions between memory units. Key modifications include correcting the conversion from megabytes to mebibytes for improved accuracy, refining the resource inference logic for memory and disk allocation, and simplifying the control flow in resource application. Additionally, new test rules have been introduced to validate these changes. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
tests/test_inferred_resources_mib/expected-results/test1.outis excluded by!**/*.outtests/test_inferred_resources_mib/expected-results/test2.outis excluded by!**/*.outtests/test_inferred_resources_mib/expected-results/test3.outis excluded by!**/*.out
Files selected for processing (4)
- snakemake/common/init.py (1 hunks)
- snakemake/resources.py (2 hunks)
- snakemake/rules.py (1 hunks)
- tests/test_inferred_resources_mib/Snakefile (1 hunks)
Additional context used
Path-based instructions (3)
snakemake/common/__init__.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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.snakemake/resources.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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.snakemake/rules.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 theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Ruff
snakemake/rules.py
1041-1041: Use
key not in dictinstead ofkey not in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (7)
tests/test_inferred_resources_mib/Snakefile (3)
1-8: LGTM! Ruleais correct.The rule correctly tests the conversion of resources specified in MB to inferred MiB and MB values.
11-18: LGTM! Rulebis correct.The rule correctly tests the conversion of resources specified in GB to inferred MiB and MB values.
21-28: LGTM! Rulecis correct.The rule correctly tests the conversion of resources specified in TB to inferred MiB and MB values.
snakemake/common/__init__.py (1)
60-60: LGTM! Improved accuracy inmb_to_mibconversion.The updated conversion factor enhances precision in converting MB to MiB.
snakemake/resources.py (1)
7-7: LGTM! Enhanced precision in resource inference.Switching to
math.ceilensures that resource values are not underestimated, aligning with the PR objectives.Also applies to: 645-645
snakemake/rules.py (2)
1032-1034: LGTM! Refactored resource inference logic.The refactoring enhances readability and maintainability by consolidating the logic for inferring additional resources. The functionality remains consistent with the PR objectives.
1035-1038: LGTM! Iteration over resource pairs.The iteration over resource pairs for inferring additional resources is clear and aligns with the PR objectives.
|
🤖 I have created a release *beep* *boop* --- ## [8.19.0](v8.18.2...v8.19.0) (2024-08-29) ### Features * check consistency of output file mtimes (must be newer than input files) ([#3050](#3050)) ([666cf62](666cf62)) * print host name when executing workflow ([#3048](#3048)) ([b0ff787](b0ff787)) ### Bug Fixes * `mem` and `disk` inference fixes ([#3040](#3040)) ([7530794](7530794)) * avoid error accessing superclass in ioutils ([#3056](#3056)) ([a66a5f5](a66a5f5)) * disable execute after print compilation ([#3041](#3041)) ([86ed3cd](86ed3cd)) * download report assets upon package build such that reports become possible offline (cont. of [#2904](#2904)) ([#3026](#3026)) ([e8dad4b](e8dad4b)) ### Documentation * Add 'Editor integrations' section to Installation page ([#3045](#3045)) ([9a4006d](9a4006d)) * Fix typo (seesee to see) ([#3037](#3037)) ([de201fb](de201fb)) * various documentation fixes ([#3052](#3052)) ([b11460c](b11460c)) --- 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--> This fixes snakemake#2774. 1. When inferring memory or disk from a string, *_mb now divides the bytes by 1e6 (SI) instead of 2^20 (binary) and takes the ceiling instead of rounding. 2. The inference of *_mib now properly multiplies by 1.048576 instead of 0.95367431640625 (this is also equivalent to dividing by the original value, which used the wrong operation) 3. After inferring resources from string-based resources, Snakemake now updates `resources` so that the inference of mem_mib and disk_mib will use the newly inferred value. 4. mem_mib and disk_mib are now inferred regardless of whether resources are inferred from human readable values. All space requests are now correct, but we may want to consider using `humanfriendly.parse_size(value, binary=True)` instead of `humanfriendly.parse_size(value)` for `mem`, since SI memory units are rarely used, but this would be an enhancement rather than a pure bugfix. Many thanks to @hermidalc for their help. ### 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** - Enhanced precision for converting megabytes to mebibytes, improving accuracy in data handling. - Updated resource inference to ensure memory and disk values are rounded up, increasing the minimum allocations. - **Bug Fixes** - Streamlined resource inference logic for clarity and maintainability, reducing potential errors. - **Tests** - Added new Snakemake rules for testing resource allocation across different capacities, ensuring accurate output in multiple units. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Leandro Hermida <softdev@leandrohermida.com>
🤖 I have created a release *beep* *boop* --- ## [8.19.0](snakemake/snakemake@v8.18.2...v8.19.0) (2024-08-29) ### Features * check consistency of output file mtimes (must be newer than input files) ([snakemake#3050](snakemake#3050)) ([666cf62](snakemake@666cf62)) * print host name when executing workflow ([snakemake#3048](snakemake#3048)) ([b0ff787](snakemake@b0ff787)) ### Bug Fixes * `mem` and `disk` inference fixes ([snakemake#3040](snakemake#3040)) ([7530794](snakemake@7530794)) * avoid error accessing superclass in ioutils ([snakemake#3056](snakemake#3056)) ([a66a5f5](snakemake@a66a5f5)) * disable execute after print compilation ([snakemake#3041](snakemake#3041)) ([86ed3cd](snakemake@86ed3cd)) * download report assets upon package build such that reports become possible offline (cont. of [snakemake#2904](snakemake#2904)) ([snakemake#3026](snakemake#3026)) ([e8dad4b](snakemake@e8dad4b)) ### Documentation * Add 'Editor integrations' section to Installation page ([snakemake#3045](snakemake#3045)) ([9a4006d](snakemake@9a4006d)) * Fix typo (seesee to see) ([snakemake#3037](snakemake#3037)) ([de201fb](snakemake@de201fb)) * various documentation fixes ([snakemake#3052](snakemake#3052)) ([b11460c](snakemake@b11460c)) --- 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>



This fixes #2774.
resourcesso that the inference of mem_mib and disk_mib will use the newly inferred value.All space requests are now correct, but we may want to consider using
humanfriendly.parse_size(value, binary=True)instead ofhumanfriendly.parse_size(value)formem, since SI memory units are rarely used, but this would be an enhancement rather than a pure bugfix.Many thanks to @hermidalc for their help.
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
Bug Fixes
Tests