Extend QueueAdapter to support dynamic configuration #345
Conversation
|
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 10 minutes and 39 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. WalkthroughThe pull request introduces significant updates to the Changes
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 (
|
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: 14
🧹 Outside diff range and nitpick comments (2)
tests/test_basic.py (1)
29-31: LGTM! Consider adding a more specific error message assertion.The new test case
test_no_configis a valuable addition. It ensures that theQueueAdapterclass raises aTypeErrorwhen instantiated without arguments, which aligns with the PR objective to extend QueueAdapter support for dynamic configuration.To make the test even more robust, consider asserting the specific error message. This ensures not only the correct exception type but also the expected error description. Here's a suggested improvement:
def test_no_config(self): with self.assertRaises(TypeError) as context: QueueAdapter() self.assertIn("missing required argument", str(context.exception))This addition would verify both the exception type and its content, providing a more comprehensive test.
tests/test_slurm.py (1)
258-266: Avoid accessing protected members in testsThe test
test_interfacesaccesses protected members likeself.slurm._adapter._commands. Accessing protected attributes (those prefixed with an underscore) breaks encapsulation and is generally discouraged.Consider using public interfaces or refactoring the code to expose necessary properties via public methods or attributes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- pysqa/queueadapter.py (10 hunks)
- tests/test_basic.py (1 hunks)
- tests/test_slurm.py (2 hunks)
🔇 Additional comments (4)
tests/test_basic.py (1)
29-32: Good addition to test coverageThe new
test_no_configmethod complements the existingtest_missing_configmethod, providing more comprehensive coverage of QueueAdapter initialization scenarios. This change aligns well with the PR objective to extend QueueAdapter support for dynamic configuration.The minimal and focused nature of this change is commendable, as it improves test coverage without introducing unnecessary complexity. It follows the existing coding style and integrates seamlessly with the current test suite.
pysqa/queueadapter.py (3)
126-132: Modify the return value ofssh_delete_file_on_remoteto consistently return a boolean.The method returns
Falsewhen_adapteris not an instance ofQueueAdapterWithConfig. Ensure that this behavior is intentional and documented.Confirm that returning
Falseis the desired default behavior when_adapteris of a different type.
373-382: Ensure consistent return types fromcheck_queue_parameters.In the
elseblock, the method returnscores, run_time_max, memory_maxdirectly. Confirm that this return value is compatible with the expected return type and that it doesn't lead to unexpectedNonevalues.Review the return values to ensure they align with the method's intended functionality.
84-86: Initialize_queue_dictconsistently whenqueue_typeis provided.When
queue_typeis provided anddirectoryisNone,_queue_dictis initialized as an empty dictionary. Ensure that this does not cause issues elsewhere in the code where_queue_dictmight be expected to contain specific entries.Consider whether
_queue_dictshould be initialized differently or documented for this case.
| + directory | ||
| ) | ||
| self._adapter = self._queue_dict[primary_queue] | ||
| raise ValueError() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case when both directory and queue_type are provided.
Currently, if both directory and queue_type are provided, the directory takes precedence, and queue_type is ignored. Consider raising a warning or error if both are provided to avoid confusion.
Add a check to alert the user:
if directory is not None:
# Existing logic
+ if queue_type is not None:
+ raise ValueError("Provide only one of 'directory' or 'queue_type', not both.")
elif queue_type is not None:
# Existing logicCommittable suggestion was skipped due to low confidence.
| raise ValueError() | ||
|
|
There was a problem hiding this comment.
Add an informative error message to ValueError.
When neither directory nor queue_type is provided, the constructor raises a ValueError without a message. Including an informative message will help users understand the error.
Apply this diff to add an error message:
else:
- raise ValueError()
+ raise ValueError("Either 'directory' or 'queue_type' must be provided to initialize QueueAdapter.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ValueError() | |
| else: | |
| raise ValueError("Either 'directory' or 'queue_type' must be provided to initialize QueueAdapter.") |
| if isinstance(self._adapter, QueueAdapterWithConfig): | ||
| self._adapter.get_job_from_remote(working_directory=working_directory) | ||
| else: | ||
| raise TypeError() | ||
|
|
There was a problem hiding this comment.
Add an informative error message to TypeError in get_job_from_remote.
Raising a TypeError without a message can make debugging difficult. Include a message to clarify the issue.
Apply this diff to add an error message:
else:
- raise TypeError()
+ raise TypeError("'get_job_from_remote' is not available when '_adapter' is not an instance of 'QueueAdapterWithConfig'.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| self._adapter.get_job_from_remote(working_directory=working_directory) | |
| else: | |
| raise TypeError() | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| self._adapter.get_job_from_remote(working_directory=working_directory) | |
| else: | |
| raise TypeError("'get_job_from_remote' is not available when '_adapter' is not an instance of 'QueueAdapterWithConfig'.") |
| def queue_list(self) -> Union[List[str], None]: | ||
| """ | ||
| Get the list of available queues. | ||
|
|
||
| Returns: | ||
| List[str]: The list of available queues. | ||
| """ | ||
| return self._adapter.queue_list | ||
| if isinstance(self._adapter, QueueAdapterWithConfig): | ||
| return self._adapter.queue_list | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Update the docstring of the queue_list property to reflect the possible None return value.
Since queue_list may return None, the docstring should be updated accordingly.
Apply this diff to update the docstring:
@property
def queue_list(self) -> Union[List[str], None]:
"""
Get the list of available queues.
Returns:
- List[str]: The list of available queues.
+ Union[List[str], None]: The list of available queues, or `None` if not available.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def queue_list(self) -> Union[List[str], None]: | |
| """ | |
| Get the list of available queues. | |
| Returns: | |
| List[str]: The list of available queues. | |
| """ | |
| return self._adapter.queue_list | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.queue_list | |
| else: | |
| return None | |
| def queue_list(self) -> Union[List[str], None]: | |
| """ | |
| Get the list of available queues. | |
| Returns: | |
| Union[List[str], None]: The list of available queues, or `None` if not available. | |
| """ | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.queue_list | |
| else: | |
| return None |
| if isinstance(self._adapter, QueueAdapterWithConfig): | ||
| return self._adapter.convert_path_to_remote(path=path) | ||
| else: | ||
| raise TypeError() |
There was a problem hiding this comment.
Add an informative error message to TypeError in convert_path_to_remote.
Providing an error message will help users understand why the method is not available.
Apply this diff to add an error message:
else:
- raise TypeError()
+ raise TypeError("'convert_path_to_remote' is not available when '_adapter' is not an instance of 'QueueAdapterWithConfig'.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.convert_path_to_remote(path=path) | |
| else: | |
| raise TypeError() | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.convert_path_to_remote(path=path) | |
| else: | |
| raise TypeError("'convert_path_to_remote' is not available when '_adapter' is not an instance of 'QueueAdapterWithConfig'.") |
| def queue_view(self) -> Union[pandas.DataFrame, None]: | ||
| """ | ||
| Get the Pandas DataFrame representation of the available queues. | ||
|
|
||
| Returns: | ||
| pandas.DataFrame: The Pandas DataFrame representation of the available queues. | ||
| """ | ||
| return self._adapter.queue_view | ||
| if isinstance(self._adapter, QueueAdapterWithConfig): | ||
| return self._adapter.queue_view | ||
| else: | ||
| return None |
There was a problem hiding this comment.
Update the docstring of the queue_view property to reflect the possible None return value.
The queue_view property might return None; the docstring should indicate this possibility.
Apply this diff to update the docstring:
@property
def queue_view(self) -> Union[pandas.DataFrame, None]:
"""
Get the Pandas DataFrame representation of the available queues.
Returns:
- pandas.DataFrame: The Pandas DataFrame representation of the available queues.
+ Union[pandas.DataFrame, None]: The DataFrame representation of the available queues, or `None` if not available.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def queue_view(self) -> Union[pandas.DataFrame, None]: | |
| """ | |
| Get the Pandas DataFrame representation of the available queues. | |
| Returns: | |
| pandas.DataFrame: The Pandas DataFrame representation of the available queues. | |
| """ | |
| return self._adapter.queue_view | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.queue_view | |
| else: | |
| return None | |
| def queue_view(self) -> Union[pandas.DataFrame, None]: | |
| """ | |
| Get the Pandas DataFrame representation of the available queues. | |
| Returns: | |
| Union[pandas.DataFrame, None]: The DataFrame representation of the available queues, or `None` if not available. | |
| """ | |
| if isinstance(self._adapter, QueueAdapterWithConfig): | |
| return self._adapter.queue_view | |
| else: | |
| return None |
| if isinstance(self._adapter, QueueAdapterWithConfig): | ||
| return self._adapter.check_queue_parameters( | ||
| queue=queue, | ||
| cores=cores, | ||
| run_time_max=run_time_max, | ||
| memory_max=memory_max, | ||
| active_queue=active_queue, | ||
| ) | ||
| else: | ||
| return cores, run_time_max, memory_max |
There was a problem hiding this comment.
Update the return type in the docstring of check_queue_parameters to match the actual return type.
The method returns a tuple, but the docstring specifies a list. Additionally, the method may return None values. Update the docstring to reflect this accurately.
Apply this diff to correct the docstring:
Returns:
- List: A list containing the checked parameters [cores, run_time_max, memory_max].
+ Tuple[Union[float, int, None], Union[float, int, None], Union[float, int, None]]: A tuple containing the checked parameters (cores, run_time_max, memory_max).Committable suggestion was skipped due to low confidence.
tests/test_slurm.py
Outdated
| def test_write_queue(self): | ||
| self.slurm._adapter._write_queue_script( | ||
| job_name=None, | ||
| working_directory=None, | ||
| cores=10, | ||
| memory_max=None, | ||
| run_time_max=4320 * 60, | ||
| command='echo "hello"', | ||
| partition="slurm", | ||
| ) | ||
| with open("run_queue.sh", "r") as f: | ||
| content = f.read() | ||
| output = """\ | ||
| #!/bin/bash | ||
| #SBATCH --output=time.out | ||
| #SBATCH --job-name=None | ||
| #SBATCH --chdir=. | ||
| #SBATCH --get-user-env=L | ||
| #SBATCH --partition=slurm | ||
| #SBATCH --time=4320 | ||
| #SBATCH --cpus-per-task=10 | ||
|
|
||
| echo \"hello\"""" | ||
| self.assertEqual(content, output) | ||
| os.remove("run_queue.sh") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in test_write_queue methods
The methods test_write_queue and test_write_queue_extra_keywords contain duplicated code for writing the queue script, reading its content, asserting the output, and removing the script file.
Consider extracting the common code into a helper method to improve maintainability.
Example:
def write_and_check_queue_script(self, **kwargs):
self.slurm._adapter._write_queue_script(**kwargs)
with open("run_queue.sh", "r") as f:
content = f.read()
self.assertEqual(content, kwargs.get('expected_output'))
os.remove("run_queue.sh")Then update your tests to use this helper method:
def test_write_queue(self):
output = """\
#!/bin/bash
#SBATCH --output=time.out
...
"""
self.write_and_check_queue_script(
job_name=None,
working_directory=None,
cores=10,
memory_max=None,
run_time_max=4320 * 60,
command='echo "hello"',
partition="slurm",
expected_output=output,
)Also applies to: 318-342
tests/test_slurm.py
Outdated
| def execute_command( | ||
| commands, | ||
| working_directory=None, | ||
| split_output=True, | ||
| shell=False, | ||
| error_filename="pysqa.err", | ||
| ): | ||
| pass | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated execute_command definitions
The execute_command function is defined multiple times in test_no_queue_id_returned, test_queue_status, and test_not_implemented_functions, leading to code duplication.
Extract execute_command into a static method or a helper function at the class level.
Example:
class TestSlurmQueueAdapterDefault(unittest.TestCase):
@staticmethod
def mock_execute_command(
commands,
working_directory=None,
split_output=True,
shell=False,
error_filename="pysqa.err",
):
# Provide appropriate implementation based on the test case
pass
def test_no_queue_id_returned(self):
slurm_tmp = QueueAdapter(
directory=os.path.join(self.path, "config/slurm"),
execute_command=self.mock_execute_command,
)
# Rest of the test code...Adjust each test method to use self.mock_execute_command and modify the implementation as needed for the specific test.
Also applies to: 369-377, 397-405
tests/test_slurm.py
Outdated
| def test_write_queue_extra_keywords(self): | ||
| self.slurm._adapter._write_queue_script( | ||
| job_name=None, | ||
| working_directory=None, | ||
| cores=10, | ||
| memory_max=None, | ||
| run_time_max=4320 * 60, | ||
| command='echo "hello"', | ||
| partition="slurm", | ||
| ) | ||
| with open("run_queue.sh", "r") as f: | ||
| content = f.read() | ||
| output = """\ | ||
| #!/bin/bash | ||
| #SBATCH --output=time.out | ||
| #SBATCH --job-name=None | ||
| #SBATCH --chdir=. | ||
| #SBATCH --get-user-env=L | ||
| #SBATCH --partition=slurm | ||
| #SBATCH --time=4320 | ||
| #SBATCH --cpus-per-task=10 | ||
|
|
||
| echo \"hello\"""" | ||
| self.assertEqual(content, output) | ||
| os.remove("run_queue.sh") |
There was a problem hiding this comment.
Missing extra keyword parameter in test_write_queue_extra_keywords
The test test_write_queue_extra_keywords is intended to test the handling of extra keyword arguments when writing the queue script. However, it doesn't pass any additional parameters compared to test_write_queue.
Include an extra keyword argument, such as account="123456", to validate that extra parameters are correctly handled.
Apply this diff to fix the issue:
def test_write_queue_extra_keywords(self):
self.slurm._adapter._write_queue_script(
job_name=None,
working_directory=None,
cores=10,
memory_max=None,
run_time_max=4320 * 60,
command='echo "hello"',
partition="slurm",
+ account="123456",
)Also, update the expected output to include the new parameter:
#SBATCH --partition=slurm
+#SBATCH --account=123456
#SBATCH --time=4320
#SBATCH --cpus-per-task=10📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_write_queue_extra_keywords(self): | |
| self.slurm._adapter._write_queue_script( | |
| job_name=None, | |
| working_directory=None, | |
| cores=10, | |
| memory_max=None, | |
| run_time_max=4320 * 60, | |
| command='echo "hello"', | |
| partition="slurm", | |
| ) | |
| with open("run_queue.sh", "r") as f: | |
| content = f.read() | |
| output = """\ | |
| #!/bin/bash | |
| #SBATCH --output=time.out | |
| #SBATCH --job-name=None | |
| #SBATCH --chdir=. | |
| #SBATCH --get-user-env=L | |
| #SBATCH --partition=slurm | |
| #SBATCH --time=4320 | |
| #SBATCH --cpus-per-task=10 | |
| echo \"hello\"""" | |
| self.assertEqual(content, output) | |
| os.remove("run_queue.sh") | |
| def test_write_queue_extra_keywords(self): | |
| self.slurm._adapter._write_queue_script( | |
| job_name=None, | |
| working_directory=None, | |
| cores=10, | |
| memory_max=None, | |
| run_time_max=4320 * 60, | |
| command='echo "hello"', | |
| partition="slurm", | |
| account="123456", | |
| ) | |
| with open("run_queue.sh", "r") as f: | |
| content = f.read() | |
| output = """\ | |
| #!/bin/bash | |
| #SBATCH --output=time.out | |
| #SBATCH --job-name=None | |
| #SBATCH --chdir=. | |
| #SBATCH --get-user-env=L | |
| #SBATCH --partition=slurm | |
| #SBATCH --account=123456 | |
| #SBATCH --time=4320 | |
| #SBATCH --cpus-per-task=10 | |
| echo \"hello\"""" | |
| self.assertEqual(content, output) | |
| os.remove("run_queue.sh") |
Summary by CodeRabbit
New Features
Bug Fixes
Tests