fix: do not limit resources (e.g. threads) before job submission in case of remote execution#3204
fix: do not limit resources (e.g. threads) before job submission in case of remote execution#3204johanneskoester wants to merge 4 commits intomainfrom
Conversation
…ase of remote execution
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
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 (5)
tests/test_cores_cluster/qsub (3)
2-3: Add error handling for log file operationsThe logging operations could fail silently if there are permission issues or if the log file becomes too large.
-echo `date` >> qsub.log -tail -n1 $1 >> qsub.log +LOG_FILE="qsub.log" +if ! echo `date` >> "$LOG_FILE"; then + echo "Error: Failed to write to $LOG_FILE" >&2 + exit 1 +fi +if ! tail -n1 "$1" >> "$LOG_FILE"; then + echo "Error: Failed to append input file content to $LOG_FILE" >&2 + exit 1 +fi
6-6: Consider safer script execution methodDirect execution of input files could be a security risk. Since this is a test script, it's acceptable, but consider adding a comment explaining the security implications.
-sh $1 +# Note: Direct script execution is acceptable in test environment +# but should be avoided in production for security reasons +sh "$1"
4-5: Document the random job ID simulationThe random job ID generation should be documented to explain its purpose in the test environment.
-# simulate printing of job id by a random number -echo $RANDOM +# Simulate cluster job ID generation (1-32767) for testing purposes +# Real cluster systems would return their own job ID format +echo $RANDOMtests/test_cores_cluster/Snakefile (1)
5-6: Consider making the shell command more robust and explicit.While the current command works, it could be more explicit about the test's purpose and provide better error reporting.
Consider this improved version:
- shell: - "if [ {threads} -gt 1 ]; then touch {output}; else exit 1; fi" + shell: + """ + if [ {threads} -eq 32 ]; then + echo "Success: Received expected 32 threads" + touch {output} + else + echo "Error: Expected 32 threads, but got {threads}" >&2 + exit 1 + fi + """This version:
- Explicitly checks for exactly 32 threads
- Provides clear success/failure messages
- Properly redirects error message to stderr
snakemake/rules.py (1)
1052-1055: LGTM! The fix correctly handles resource limitations for remote execution.The changes ensure that resource limits are only applied for local execution or resources explicitly marked as local, which fixes the issue of remote jobs being throttled by local core counts.
Consider adding a comment to explain the rationale:
if isinstance(res, int) and ( + # Only apply resource limits for local execution or explicitly local resources self.workflow.is_local(self) or name in self.workflow.resource_scopes.locals ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
snakemake/rules.py(1 hunks)tests/test_cores_cluster/Snakefile(1 hunks)tests/test_cores_cluster/qsub(1 hunks)tests/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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 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.
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 (5)
tests/test_cores_cluster/qsub (2)
1-6: Verify test suite integration
Let's verify how this script integrates with the test workflow and cluster submission.
#!/bin/bash
# Check for related test files and their configuration
echo "Checking related test files:"
echo "1. Snakefile configuration:"
find tests/test_cores_cluster -name "Snakefile" -exec cat {} \;
echo "2. Test implementation:"
rg -A 5 "test_cores_cluster" tests/tests.py1-1: Ensure script has execute permissions
The script needs execute permissions to function in the test environment.
✅ Verification successful
Script has execute permissions
The tests/test_cores_cluster/qsub script has execute permissions set correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the script has execute permissions
ls -l tests/test_cores_cluster/qsub
Length of output: 636
Script:
#!/bin/bash
# Check execute permissions of the qsub script
stat -c '%A' tests/test_cores_cluster/qsub
Length of output: 114
Script:
#!/bin/bash
# Check if the qsub script has execute permissions using find
find tests/test_cores_cluster/qsub -perm /u=x,g=x,o=x -ls
Length of output: 161
tests/test_cores_cluster/Snakefile (1)
1-6: LGTM! Well-designed test case for validating thread allocation.
The test case effectively validates the PR's objective by:
- Requesting 32 threads (likely exceeding local cores)
- Failing if thread count is incorrectly limited
- Providing a clear pass/fail outcome
snakemake/rules.py (1)
1052-1055: Verify test coverage for resource handling in remote execution.
Let's ensure that the test case mentioned in the PR objectives is properly implemented.
tests/tests.py (1)
930-936: LGTM! Test case appropriately validates the fix.
The test case correctly validates that remote job execution is not limited by locally detected cores by:
- Using the cluster-generic executor
- Providing a custom job submission command
- Limiting concurrent jobs to 1 to ensure proper validation
|
|
@johanneskoester there seem to be a few issues to be fixed. Just merged main, so that this is at least done, before fixing the little things. I wonder: Is it related to #2896, too? |
|
@johanneskoester & @fgvieira just screening the hackathon project space and I wonder: does this issue still persist? |
|
As far as I can see, I think this issue is fixed. |
Copied from <snakemake#3204>.
fixes #3191
fixes #2997
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