Conversation
📝 WalkthroughWalkthroughThe pull request updates resource specifications within the Snakefile located in the test suite. In the rule named "a", the resource key Changes
🪧 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: 0
🧹 Nitpick comments (2)
test-environment.yml (2)
7-7: New Dependency Addition: setuptools
The dependencysetuptoolshas been added directly under the maindependenciessection. This change helps simplify dependency management by ensuring that setuptools is installed from the primary channel list rather than being nested elsewhere. Consider pinning its version if there are concerns about future compatibility.
61-62: Relocating Dependencies: cwltool and cwl-utils
The packagescwltoolandcwl-utilshave been moved to the maindependenciessection, ensuring unified management and installation via the specified channels. This shift removes them from the nestedpipsection as intended. Verify that these packages are available from the channels in use and consider adding version constraints if necessary for stability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (1)
test-environment.yml(2 hunks)
This centralizes the code for resource handling into the `resource.py` file under a new `Resource` class. Notably, `DefaultResources` has been removed, its functionality rolled into the new class. The same class is now used for handling global resource limits, default resources, and rule resource constraints As a result of this standarization, the following bugs/inconsistencies are fixed: 1) humanreadable resources can now be specified in a group job in cluster submit without crashing the program (#3367) 2) `mem`, `mem_mb`, and `mem_mib` can now be used interchangeably across `--resources`, `--set-resource-scopes`, `--default-resources`, `--set-resources`, and the `resource` keyword within a `Snakefile`, as the names are internally standardized. 4) humanreadable resources can now be used in all resource contexts (e.g. `--resources`) 6) Providing the same effective resource twice (e.g. mem, mem_mb and/or mem_mib) in the same rule or in the global resource specification raises an error. 7) **BREAKING** `mem_mb` and `mem_mib` can no longer be provided a string or a function that returns a string within a rule. 8) The `{resources}` object available within a shell command or parameter context now has access to all versions of a sized resource, e.g. `mem`, `mem_mb`, `mem_mib`. `mem` is now always a string with a unit. A few additional changes: - The old `Resource` class in snakemake/io.py (that subclasses `Namedlist` was renamed to `ResourceList` to prevent confusion. This class is still used within the `Job` class and is ultimately passed onto the executor. This ensures compatibility with the existing plugin interface. - I added the ability to watch for a specific Exception class within the tests to help increase the resolution of some of my "failure" tests. This is completely backwards-compatible - Additional typing annotations were added throughout the code I touched. - A small rearrangement in the code within snakemake/workflow.py to group the resource-related code together. ### Outstanding issues (not to be resolved here) 1. It was, and remains, possible to sneak a string value as a thread using `--set-threads` with a python expression 1. --resources cannot be given an expression, per design. Currently, though, nothing stops one from from providing an expression directly via the api. This problem preexists this PR. 3. There is currently no testing of the cli parsing functions. This should be written in a new PR. 4. The check for positive integers is only done for the CLI. Resources requested within rules will not be so validated. This is unchanged from previous ### Gotchas 1. This does represent a breaking change for API use, as the dataclasses will not perform any automatic conversion. Specifically, ResourceSettings.resources, .overwrite_threads, .overwrite_resources, and .default_resources will be broken in dependent apps. <!--Add a description of your PR here--> ### 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 # Release Notes * **New Features** * Support for human-readable resource specifications (e.g., "1 GB", "1 GiB") * New resource units: mem_mib, disk_mib alongside existing mem_mb, disk_mb * **Bug Fixes** * Improved resource handling and validation with better error messages * Enhanced resource constraint detection and reporting * **Refactor** * Unified resource management system for simplified configuration * Streamlined CLI resource parsing and argument handling <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Closing, as no longer relevant. |



Resources on groups are gathered but, currently, it only works if they are all numeric. If some resources are human-readable strings (e.g. "10 m" or "5 GiB"), it fails.
Should pass all tests with #3421.
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
setuptools,cwltool, andcwl-utilsas top-level dependencies in the test environment configuration.