Add linting #555
Add linting #555
Conversation
WalkthroughThis pull request makes several broad refactoring improvements across the repository. Most changes update type annotations to use built‐in types (e.g., replacing Changes
Sequence Diagram(s)sequenceDiagram
participant Executor
participant BaseSpawner
participant DerivedSpawner
Executor->>BaseSpawner: bootup(command_lst)
BaseSpawner->>DerivedSpawner: initialize process
DerivedSpawner-->>BaseSpawner: process started
Executor->>BaseSpawner: poll status
BaseSpawner-->>Executor: status running
Executor->>BaseSpawner: shutdown(wait=True)
BaseSpawner->>DerivedSpawner: terminate process
DerivedSpawner-->>BaseSpawner: process terminated
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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
🧹 Nitpick comments (2)
pyproject.toml (1)
66-86: Define Ruff Linting Rules.
The introduced[tool.ruff.lint]block sets up a comprehensive list of rule codes to select—covering pycodestyle, Pyflakes, pyupgrade, bugbear, simplify, isort, comprehensions, eradicate, and pylint. This offers a consistent linting baseline based on your project’s style guidelines.Consider including inline comments or references to documentation for each group of codes to improve future maintainability.
executorlib/cache/queue_spawner.py (1)
13-13: Update docstring to match type annotation.The docstring for
task_dependent_lstshould be updated to reflect that the list contains integers.Apply this diff to update the docstring:
- task_dependent_lst (list): A list of subprocesses that the current subprocess depends on. Defaults to []. + task_dependent_lst (list[int]): A list of subprocess IDs that the current subprocess depends on. Defaults to [].Also applies to: 25-25
🧰 Tools
🪛 Ruff (0.8.2)
13-13: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.pre-commit-config.yaml(1 hunks)executorlib/backend/cache_parallel.py(0 hunks)executorlib/base/executor.py(1 hunks)executorlib/cache/queue_spawner.py(1 hunks)executorlib/cache/shared.py(1 hunks)executorlib/interactive/create.py(2 hunks)executorlib/interactive/shared.py(3 hunks)executorlib/standalone/hdf.py(1 hunks)executorlib/standalone/inputcheck.py(1 hunks)executorlib/standalone/interactive/backend.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- executorlib/backend/cache_parallel.py
✅ Files skipped from review due to trivial changes (3)
- executorlib/standalone/hdf.py
- executorlib/cache/shared.py
- executorlib/interactive/create.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/cache/queue_spawner.py
3-3: typing.List is deprecated, use list instead
(UP035)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build (windows-latest, 3.13)
🔇 Additional comments (8)
.pre-commit-config.yaml (1)
7-7: Simplify Ruff Linting Arguments.
Removing the restrictive--select Iflag in favor of a more general--fixapproach allows Ruff to address all fixable issues. Ensure this broader application aligns with your project's linting strategy.pyproject.toml (2)
63-65: Add Ruff Exclusion Configuration.
The new[tool.ruff]section correctly excludes directories and files (e.g., "docs", "notebooks", "tests", "setup.py", and "_version.py") from linting. Verify these exclusions match your overall linting and project structure strategy.
87-102: Configure Ruff Lint Ignore List.
The ignore list is set to bypass specific rules (such as line-length violations and several pylint warnings). Please verify that excluding these codes aligns with your intended coding standards and does not allow critical issues to slip through.executorlib/standalone/interactive/backend.py (1)
97-97: LGTM! Good improvement in dictionary key testing.The change from using
.keys()to direct dictionary membership testing is a good improvement that:
- Makes the code more pythonic and readable
- Slightly improves performance by avoiding the creation of a view object
- Aligns with the broader linting improvements across the codebase
executorlib/base/executor.py (1)
90-90: LGTM! Simplified dictionary access.The change removes the redundant default value of
Nonefrom thegetmethod, as it's already the default behavior. The functionality remains unchanged.executorlib/standalone/inputcheck.py (1)
188-193: LGTM! Improved error handling and readability.The changes improve the code by:
- Moving the error check outside the
elseblock for better visibility.- Simplifying the return statement for better readability.
executorlib/interactive/shared.py (2)
139-139: LGTM! Consistent parameter handling.The changes align with the parent class by removing redundant default values from the
max_coresparameter in constructor calls.Also applies to: 193-193
296-296: LGTM! Simplified dictionary key check.The change uses direct membership testing instead of the redundant
.keys()method, making the code more concise.
executorlib/cache/queue_spawner.py
Outdated
| @@ -1,6 +1,6 @@ | |||
| import os | |||
| import subprocess | |||
| from typing import List, Optional, Tuple, Union | |||
| from typing import List, Optional, Union | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove deprecated typing.List import.
The typing.List import is deprecated. Since Python 3.9+, you can use the built-in list type with type parameters directly.
Apply this diff to fix the import:
-from typing import List, Optional, Union
+from typing import Optional, Union📝 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.
| from typing import List, Optional, Union | |
| from typing import Optional, Union |
🧰 Tools
🪛 Ruff (0.8.2)
3-3: typing.List is deprecated, use list instead
(UP035)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.ci_support/release.py (1)
1-71: Consider additional improvements for better code quality.While the current changes are good, here are some additional suggestions to improve code quality:
- Add type hints to improve code maintainability
- Replace single-letter variables with descriptive names
- Simplify dictionary membership test
- Use more Pythonic approaches for string manipulation
Here's an example of how these improvements could look:
- def get_setup_version_and_pattern(setup_content): + from typing import List, Dict + + def get_setup_version_and_pattern(setup_content: List[str]) -> Dict[str, str]: depend_lst, version_lst = [], [] - for l in setup_content: + for line in setup_content: if "==" in l: lst = ( l.split("[")[-1] .split("]")[0] .replace(" ", "") .replace('"', "") .replace("'", "") .split(",") ) for dep in lst: if dep != "\n": version_lst.append(dep.split("==")[1]) depend_lst.append(dep.split("==")[0]) version_high_dict = {d: v for d, v in zip(depend_lst, version_lst)} return version_high_dict - def get_env_version(env_content): + def get_env_version(env_content: List[str]) -> Dict[str, str]: read_flag = False depend_lst, version_lst = [], [] - for l in env_content: + for line in env_content: if "dependencies:" in l: read_flag = True elif read_flag: lst = l.replace("-", "").replace(" ", "").replace("\n", "").split("=") if len(lst) == 2: depend_lst.append(lst[0]) version_lst.append(lst[1]) return {d: v for d, v in zip(depend_lst, version_lst)} - def update_dependencies(setup_content, version_low_dict, version_high_dict): + def update_dependencies( + setup_content: List[str], + version_low_dict: Dict[str, str], + version_high_dict: Dict[str, str] + ) -> str: version_combo_dict = {} for dep, ver in version_high_dict.items(): - if dep in version_low_dict.keys() and version_low_dict[dep] != ver: + if dep in version_low_dict and version_low_dict[dep] != ver: version_combo_dict[dep] = f"{dep}>={version_low_dict[dep]}<={ver}" else: version_combo_dict[dep] = f"{dep}=={ver}"🧰 Tools
🪛 Ruff (0.8.2)
3-3: Ambiguous variable name:
l(E741)
18-18: Unnecessary dict comprehension (rewrite using
dict())Rewrite using
dict()(C416)
25-25: Ambiguous variable name:
l(E741)
33-33: Unnecessary dict comprehension (rewrite using
dict())Rewrite using
dict()(C416)
39-39: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
46-46: Ambiguous variable name:
l(E741)
49-49: Ambiguous variable name:
l(E741)
49-49:
forloop variableloverwritten by assignment target(PLW2901)
executorlib/cache/subprocess_spawner.py (1)
51-51: Usedict.get()for cleaner dictionary access.Consider using the more idiomatic
dict.get()method for dictionary access with a default value.- cwd = resource_dict["cwd"] if "cwd" in resource_dict else cache_directory + cwd = resource_dict.get("cwd", cache_directory)🧰 Tools
🪛 Ruff (0.8.2)
51-51: Use
resource_dict.get("cwd", cache_directory)instead of anifblockReplace with
resource_dict.get("cwd", cache_directory)(SIM401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.ci_support/release.py(1 hunks)executorlib/backend/cache_parallel.py(1 hunks)executorlib/backend/interactive_parallel.py(2 hunks)executorlib/backend/interactive_serial.py(4 hunks)executorlib/base/executor.py(6 hunks)executorlib/cache/queue_spawner.py(4 hunks)executorlib/cache/shared.py(4 hunks)executorlib/cache/subprocess_spawner.py(3 hunks)executorlib/interactive/create.py(7 hunks)executorlib/standalone/hdf.py(4 hunks)executorlib/standalone/inputcheck.py(4 hunks)executorlib/standalone/interactive/communication.py(3 hunks)executorlib/standalone/plot.py(1 hunks)executorlib/standalone/queue.py(1 hunks)executorlib/standalone/serialize.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- executorlib/standalone/plot.py
🚧 Files skipped from review as they are similar to previous changes (5)
- executorlib/backend/cache_parallel.py
- executorlib/cache/queue_spawner.py
- executorlib/standalone/inputcheck.py
- executorlib/interactive/create.py
- executorlib/standalone/hdf.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/cache/subprocess_spawner.py
51-51: Use resource_dict.get("cwd", cache_directory) instead of an if block
Replace with resource_dict.get("cwd", cache_directory)
(SIM401)
executorlib/base/executor.py
168-168: Undefined name contextlib
(F821)
executorlib/cache/shared.py
82-82: Undefined name contextlib
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
.ci_support/release.py (2)
55-55: LGTM! Good cleanup.Removing the explicit
"r"mode is a good practice since it's the default mode foropen().
58-58: LGTM! Good cleanup.Removing the explicit
"r"mode is a good practice since it's the default mode foropen().executorlib/standalone/queue.py (1)
15-15: LGTM! Improved dictionary key check.The change to use direct membership test (
in item) instead of.keys()is more idiomatic and efficient.executorlib/cache/subprocess_spawner.py (1)
10-10: LGTM! Fixed mutable default argument.The change from a mutable default argument to
Optional[list]with proper initialization is a good practice to avoid the shared mutable state pitfall.Also applies to: 36-37
executorlib/backend/interactive_serial.py (2)
3-3: LGTM! Updated type hints.The change from
Listtolistaligns with modern Python type hinting practices (Python 3.9+).Also applies to: 14-14
43-43: LGTM! Improved dictionary key checks.The changes to use direct membership tests (
in input_dict) instead of.keys()are more idiomatic and efficient.Also applies to: 48-51, 65-65, 67-68
executorlib/standalone/serialize.py (1)
4-4: LGTM! Updated type hints.The change from
Tupletotuplealigns with modern Python type hinting practices (Python 3.9+).Also applies to: 33-33
executorlib/backend/interactive_parallel.py (1)
60-60: LGTM! Improved dictionary key membership checks.The changes simplify dictionary key membership checks by removing unnecessary
.keys()calls, making the code more concise and readable while maintaining the same functionality.Also applies to: 67-70, 90-93
executorlib/base/executor.py (2)
8-8: LGTM! Updated type hints to use built-in types.The changes modernize type hints by using built-in types instead of importing from typing module, improving code readability.
Also applies to: 31-31
65-67: LGTM! Fixed mutable default argument.Changed the default value of
resource_dictfrom{}toNoneto avoid the mutable default argument pitfall, which is a Python best practice.Also applies to: 92-94
executorlib/standalone/interactive/communication.py (1)
3-3: LGTM! Simplified dictionary key check and modernized type hints.The changes improve code readability by:
- Using built-in
tupletype instead ofTuplefrom typing module- Simplifying dictionary key membership check
Also applies to: 47-47
executorlib/cache/shared.py (1)
199-201: LGTM! Updated return type annotation.Changed return type annotation to use built-in
tupletype instead ofTuplefrom typing module.
executorlib/standalone/serialize.py
Outdated
| def serialize_funct_h5( | ||
| fn: Callable, fn_args: list = [], fn_kwargs: dict = {}, resource_dict: dict = {} | ||
| ) -> Tuple[str, dict]: | ||
| ) -> tuple[str, dict]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix mutable default arguments.
The function uses mutable default arguments which can lead to unexpected behavior.
def serialize_funct_h5(
- fn: Callable, fn_args: list = [], fn_kwargs: dict = {}, resource_dict: dict = {}
+ fn: Callable,
+ fn_args: Optional[list] = None,
+ fn_kwargs: Optional[dict] = None,
+ resource_dict: Optional[dict] = None,
) -> tuple[str, dict]:
+ if fn_args is None:
+ fn_args = []
+ if fn_kwargs is None:
+ fn_kwargs = {}
+ if resource_dict is None:
+ resource_dict = {}📝 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 serialize_funct_h5( | |
| fn: Callable, fn_args: list = [], fn_kwargs: dict = {}, resource_dict: dict = {} | |
| ) -> Tuple[str, dict]: | |
| ) -> tuple[str, dict]: | |
| def serialize_funct_h5( | |
| fn: Callable, | |
| fn_args: Optional[list] = None, | |
| fn_kwargs: Optional[dict] = None, | |
| resource_dict: Optional[dict] = None, | |
| ) -> tuple[str, dict]: | |
| if fn_args is None: | |
| fn_args = [] | |
| if fn_kwargs is None: | |
| fn_kwargs = {} | |
| if resource_dict is None: | |
| resource_dict = {} | |
| # ... rest of the function implementation ... |
🧰 Tools
🪛 Ruff (0.8.2)
32-32: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
32-32: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
32-32: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| with contextlib.suppress(AttributeError, RuntimeError): | ||
| self.shutdown(wait=False) |
There was a problem hiding this comment.
Add missing contextlib import.
The code uses contextlib.suppress but the import statement is missing.
Add this import at the top of the file:
+import contextlib
from typing import Callable, Optional, Union📝 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.
| with contextlib.suppress(AttributeError, RuntimeError): | |
| self.shutdown(wait=False) | |
| import contextlib | |
| from typing import Callable, Optional, Union | |
| # ... (other imports and code) | |
| with contextlib.suppress(AttributeError, RuntimeError): | |
| self.shutdown(wait=False) |
🧰 Tools
🪛 Ruff (0.8.2)
168-168: Undefined name contextlib
(F821)
| @@ -3,7 +3,7 @@ | |||
| import queue | |||
| import sys | |||
| from concurrent.futures import Future | |||
| from typing import Any, Callable, Optional, Tuple | |||
| from typing import Any, Callable, Optional | |||
There was a problem hiding this comment.
Add missing contextlib import and LGTM for other changes.
The code improvements look good:
- Modernized type hints using built-in types
- Simplified dictionary key membership check
- Improved error handling with contextlib.suppress
However, the contextlib import is missing.
Add this import at the top of the file:
+import contextlib
from typing import Any, Callable, OptionalAlso applies to: 82-84, 107-107
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
executorlib/interactive/slurm.py (1)
102-102: Fix string concatenation in command line argument.There's a missing space in the string concatenation for
--cpus-per-task.- command_prepend_lst += ["--cpus-per-task" + str(threads_per_core)] + command_prepend_lst += ["--cpus-per-task=" + str(threads_per_core)]
🧹 Nitpick comments (4)
executorlib/interactive/slurm.py (2)
42-42: Update docstring to reflect new default value.The docstring still mentions the default as
[]but it's nowNone.- slurm_cmd_args (list[str], optional): Additional command line arguments. Defaults to []. + slurm_cmd_args (list[str], optional): Additional command line arguments. Defaults to None.
93-93: Update docstring to reflect new default value.The docstring still mentions the default as
[]but it's nowNone.- slurm_cmd_args (list[str], optional): Additional command line arguments. Defaults to []. + slurm_cmd_args (list[str], optional): Additional command line arguments. Defaults to None.executorlib/interactive/executor.py (1)
76-76: Update docstring to reflect new default value.The docstring still mentions the default as
{}but it's nowNone.- resource_dict (dict, optional): A dictionary of resources required by the task. Defaults to {}. + resource_dict (dict, optional): A dictionary of resources required by the task. Defaults to None.executorlib/standalone/interactive/spawner.py (1)
121-126: Consider adding error handling for subprocess termination.The shutdown process could benefit from additional error handling to catch potential exceptions during termination.
if self._process is not None: - self._process.communicate() - self._process.terminate() - if wait: - self._process.wait() + try: + self._process.communicate() + self._process.terminate() + if wait: + self._process.wait() + except Exception as e: + # Log the error but continue with cleanup + print(f"Error during process shutdown: {e}") self._process = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
executorlib/base/executor.py(6 hunks)executorlib/cache/shared.py(4 hunks)executorlib/cache/subprocess_spawner.py(3 hunks)executorlib/interactive/executor.py(3 hunks)executorlib/interactive/shared.py(14 hunks)executorlib/interactive/slurm.py(3 hunks)executorlib/standalone/interactive/spawner.py(4 hunks)executorlib/standalone/plot.py(2 hunks)executorlib/standalone/serialize.py(3 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- executorlib/standalone/plot.py
- executorlib/standalone/serialize.py
- executorlib/cache/shared.py
- executorlib/cache/subprocess_spawner.py
- executorlib/base/executor.py
- executorlib/interactive/shared.py
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
executorlib/interactive/slurm.py (1)
31-31: LGTM! Good improvement in parameter handling.The change from a mutable default (empty list) to
Noneis a good practice to avoid the shared mutable state pitfall.Also applies to: 82-82
executorlib/interactive/executor.py (1)
67-67: LGTM! Good improvement in parameter handling.The change from a mutable default (empty dict) to
Nonewith proper initialization is a good practice to avoid the shared mutable state pitfall.Also applies to: 83-84
executorlib/standalone/interactive/spawner.py (1)
27-58: LGTM! Good improvement in interface definition.The addition of abstract methods with clear documentation improves the contract for derived classes and enforces proper lifecycle management.
for more information, see https://pre-commit.ci
Summary by CodeRabbit
Chores
Refactor