Add submission_script_path to write run_queue.sh script to a different location #364
Conversation
WalkthroughThe pull request introduces modifications to enhance the job submission functionality across several classes in the Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pysqa/queueadapter.py (1)
216-217: Enhance parameter documentationWhile the documentation is clear, it would be helpful to add more details about:
- Expected format for the path (absolute vs relative)
- Behavior when the directory in the path doesn't exist
- Default behavior when
submission_script_pathis NoneConsider updating the docstring to:
- submission_script_path (str/None): path to write the submission script to. + submission_script_path (str/None): Absolute or relative path where the submission script will be written. + If None, uses the default location. Directory must exist.pysqa/base/core.py (1)
Line range hint
356-397: Update docstring to document new parameter and return values.The implementation looks good, but the docstring needs to be updated to:
- Document the new
submission_script_pathparameter- Clarify that the return tuple contains (working_directory, submission_script_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pysqa/base/core.py(7 hunks)pysqa/base/modular.py(2 hunks)pysqa/queueadapter.py(3 hunks)
🔇 Additional comments (7)
pysqa/base/modular.py (3)
Line range hint 76-92: Consider updating tests for the new submission script path functionality
The changes to support custom submission script paths should be covered by tests to ensure proper handling across different cluster configurations.
#!/bin/bash
# Search for relevant test files and test cases
fd "test.*\.py" | xargs rg -l "submit_job|_write_queue_script"90-92: LGTM! Using named parameters improves clarity.
The use of named parameters makes the code more explicit and maintainable. However, let's verify the parent class implementation accepts this parameter.
✅ Verification successful
Parameter usage is correct and consistent with parent class implementation
The verification shows that _list_command_to_be_executed in the parent class (pysqa/base/core.py) accepts submission_script_path as a parameter with the exact same name, making this usage correct and consistent with the parent implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for _list_command_to_be_executed implementation
ast-grep --pattern 'def _list_command_to_be_executed($$$)'
Length of output: 394
Script:
#!/bin/bash
# Search for _list_command_to_be_executed implementation with ripgrep
rg -A 5 "_list_command_to_be_executed" --type py
Length of output: 6189
Line range hint 76-82: Verify parent class compatibility
The code now expects _write_queue_script to return both working_directory and submission_script_path. Let's verify this change is consistent with the parent class implementation.
✅ Verification successful
Parent class implementation is compatible with the changes
The verification shows that both the parent class (pysqa/base/core.py) and the child class (pysqa/base/modular.py) use the same pattern of unpacking two values (working_directory and submission_script_path) from _write_queue_script. The parent class implementation already includes the submission_script_path parameter, making this change consistent with the inheritance hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for _write_queue_script implementation in parent classes
ast-grep --pattern 'def _write_queue_script($$$)'
Length of output: 394
Script:
#!/bin/bash
# Search for _write_queue_script implementation using ripgrep
rg -A 10 "_write_queue_script" --type py
Length of output: 4777
pysqa/queueadapter.py (1)
234-234: Verify adapter implementations
The parameter forwarding looks good, but we should ensure all queue adapter implementations (QueueAdapterCore, ModularQueueAdapter, RemoteQueueAdapter) support the new submission_script_path parameter to prevent runtime errors.
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the parameter support across adapter implementations.
Parameter submission_script_path is properly supported across adapter implementations
The verification shows that the parameter is consistently implemented:
QueueAdapterCore(inbase/core.py) fully supports it in both thesubmit_jobmethod and helper methodsModularQueueAdapter(inbase/modular.py) properly handles it in its implementation- No
RemoteQueueAdapterwas found in the codebase, suggesting it was either removed or renamed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all adapter implementations support the new parameter
# Look for submit_job method implementations in all adapter classes
ast-grep --pattern 'class $ADAPTER:
$$$
def submit_job($$$):
$$$'
# Also check for any existing usages of the new parameter
rg "submission_script_path" --type py
Length of output: 1667
pysqa/base/core.py (3)
149-150: LGTM! The submission_script_path parameter is well integrated.
The changes to submit_job are consistent and well-implemented:
- Parameter is properly typed and optional
- Documentation is clear
- Parameter is correctly propagated to helper methods
Also applies to: 164-165, 176-186, 190-191
304-304: LGTM! Parameter renaming is consistent.
The renaming of queue_script_path to submission_script_path is consistent with the changes throughout the class, and the docstring has been properly updated.
Also applies to: 309-309, 314-314
304-314: Verify parameter name changes across the codebase.
Let's ensure all callers have been updated to use submission_script_path instead of queue_script_path.
✅ Verification successful
Parameter name change is consistent across the codebase
The verification shows that:
- No occurrences of the old parameter name
queue_script_pathwere found - All uses of
submission_script_pathare consistent throughout the codebase - All calls to
_list_command_to_be_executeduse the correct parameter namesubmission_script_path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of queue_script_path
rg "queue_script_path" --type python
# Search for uses of the new parameter name
rg "submission_script_path" --type python
# Search for calls to _list_command_to_be_executed to verify parameter usage
ast-grep --pattern '_list_command_to_be_executed\($$$\)'
Length of output: 199
Script:
#!/bin/bash
# Search for any remaining uses of queue_script_path
rg "queue_script_path"
# Search for uses of the new parameter name
rg "submission_script_path"
# Search for method calls using ast-grep with a more specific pattern
ast-grep --pattern 'self._list_command_to_be_executed($_)'
Length of output: 2057
| @@ -197,6 +197,7 @@ def submit_job( | |||
| dependency_list: Optional[List[str]] = None, | |||
| command: Optional[str] = None, | |||
| submission_template: Optional[Union[str, Template]] = None, | |||
| submission_script_path: Optional[str] = None, | |||
There was a problem hiding this comment.
💡 Codebase verification
Parent class signature needs to be updated
The submission_script_path parameter is not present in the parent class QueueAdapterAbstractClass in pysqa/base/abstract.py. Adding this parameter to the child class without updating the parent class violates the Liskov Substitution Principle. The abstract base class should be updated first to include this parameter.
pysqa/base/abstract.py: UpdateQueueAdapterAbstractClass.submit_job()method signature to include thesubmission_script_pathparameterpysqa/base/core.py: Update the intermediate implementation if present
🔗 Analysis chain
Verify parent class method signature
The addition of the submission_script_path parameter looks good, but we should ensure this change is consistent with the parent class QueueAdapterAbstractClass.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the parent class method signature is compatible
# Look for the abstract class definition and its submit_job method
ast-grep --pattern 'class QueueAdapterAbstractClass:
$$$
def submit_job($$$):
$$$'
Length of output: 92
Script:
#!/bin/bash
# Let's try a different approach to find the parent class and its method signature
# First, let's find all Python files that might contain QueueAdapterAbstractClass
fd '.*\.py$' --exec rg -l 'QueueAdapterAbstractClass'
# Then, let's look for the actual class definition and method signature with context
rg -A 10 'class QueueAdapterAbstractClass' '*.py'
Length of output: 220
Script:
#!/bin/bash
# Now that we found the files, let's check each one for the class definition
echo "=== Checking pysqa/base/abstract.py ==="
rg -A 10 'class QueueAdapterAbstractClass' pysqa/base/abstract.py
echo "=== Checking pysqa/base/core.py ==="
rg -A 10 'class QueueAdapterAbstractClass' pysqa/base/core.py
echo "=== Checking submit_job method signature ==="
rg -B 2 -A 5 'def submit_job' pysqa/base/abstract.py pysqa/base/core.py
Length of output: 1717
Summary by CodeRabbit
New Features
submission_script_pathparameter in the job submission process, allowing users to specify the script's location.Bug Fixes
Refactor