Skip to content

Commit

Permalink
[style]: updates to coding guidelines and fixes for all QA errors aft…
Browse files Browse the repository at this point in the history
…er migrating to ruff
  • Loading branch information
egparedes authored and tehrengruber committed Feb 28, 2024
1 parent 4c8f706 commit 77a205b
Show file tree
Hide file tree
Showing 89 changed files with 363 additions and 364 deletions.
25 changes: 17 additions & 8 deletions CODING_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ We follow the [Google Python Style Guide][google-style-guide] with a few minor c

We deviate from the [Google Python Style Guide][google-style-guide] only in the following points:

- We use [`flake8`][flake8] with some plugins instead of [`pylint`][pylint].
- We use [`black`][black] and [`isort`][isort] for source code and imports formatting, which may work differently than indicated by the guidelines in section [_3. Python Style Rules_](https://google.github.io/styleguide/pyguide.html#3-python-style-rules). For example, maximum line length is set to 100 instead of 79 (although docstring lines should still be limited to 79).
- We use [`ruff-linter`][ruff-linter] instead of [`pylint`][pylint].
- We use [`ruff-formatter`][ruff-formatter] for source code and imports formatting, which may work differently than indicated by the guidelines in section [_3. Python Style Rules_](https://google.github.io/styleguide/pyguide.html#3-python-style-rules). For example, maximum line length is set to 100 instead of 79 (although docstring lines should still be limited to 79).
- According to subsection [_2.19 Power Features_](https://google.github.io/styleguide/pyguide.html#219-power-features), direct use of _power features_ (e.g. custom metaclasses, import hacks, reflection) should be avoided, but standard library classes that internally use these power features are accepted. Following the same spirit, we allow the use of power features in infrastructure code with similar functionality and scope as the Python standard library.
- According to subsection [_3.19.12 Imports For Typing_](https://google.github.io/styleguide/pyguide.html#31912-imports-for-typing), symbols from `typing` and `collections.abc` modules used in type annotations _"can be imported directly to keep common annotations concise and match standard typing practices"_. Following the same spirit, we allow symbols to be imported directly from third-party or internal modules when they only contain a collection of frequently used typying definitions.

Expand Down Expand Up @@ -107,7 +107,7 @@ In general, you should structure new Python modules in the following way:
1. _shebang_ line: `#! /usr/bin/env python3` (only for **executable scripts**!).
2. License header (see `LICENSE_HEADER.txt`).
3. Module docstring.
4. Imports, alphabetically ordered within each block (fixed automatically by `isort`):
4. Imports, alphabetically ordered within each block (fixed automatically by `ruff-formatter`):
1. Block of imports from the standard library.
2. Block of imports from general third party libraries using standard shortcuts when customary (e.g. `numpy as np`).
3. Block of imports from specific modules of the project.
Expand All @@ -126,10 +126,17 @@ Consider configuration files as another type of source code and apply the same c

### Ignoring QA errors

You may occasionally need to disable checks from _quality assurance_ (QA) tools (e.g. linters, type checkers, etc.) on specific lines as some tool might not be able to fully understand why a certain piece of code is needed. This is usually done with special comments, e.g. `# type: ignore`. However, you should **only** ignore QA errors when you fully understand their source and rewriting your code to pass QA checks would make it less readable. Additionally, you should add a brief comment for future reference, e.g.:
You may occasionally need to disable checks from _quality assurance_ (QA) tools (e.g. linters, type checkers, etc.) on specific lines as some tool might not be able to fully understand why a certain piece of code is needed. This is usually done with special comments, e.g. `# noqa: F401`, `# type: ignore`. However, you should **only** ignore QA errors when you fully understand their source and rewriting your code to pass QA checks would make it less readable. Additionally, you should add a short descriptive code if possible (check [ruff rules][ruff-rules] and [mypy error codes][mypy-error-codes] for reference):

```python
f = lambda: 'empty' # noqa: E731 # assign lambda expression for testing
f = lambda: 'empty' # noqa: E731 [lambda-assignment]
```

and, if needed, a brief comment for future reference:

```python
...
return undeclared_symbol # noqa: F821 [undefined-name] on purpose to trigger black-magic
```

## Testing
Expand Down Expand Up @@ -213,13 +220,15 @@ https://testandcode.com/116

<!-- Reference links -->

[black]: https://black.readthedocs.io/en/stable/
[doctest]: https://docs.python.org/3/library/doctest.html
[flake8]: https://flake8.pycqa.org/
[google-style-guide]: https://google.github.io/styleguide/pyguide.html
[isort]: https://pycqa.github.io/isort/
[mypy]: https://mypy.readthedocs.io/
[mypy-error-codes]: https://mypy.readthedocs.io/en/stable/error_code_list.html
[pre-commit]: https://pre-commit.com/
[pylint]: https://pylint.pycqa.org/
[ruff-formatter]: https://docs.astral.sh/ruff/formatter/
[ruff-linter]: https://docs.astral.sh/ruff/linter/
[ruff-rules]: https://docs.astral.sh/ruff/rules/
[sphinx]: https://www.sphinx-doc.org
[sphinx-autodoc]: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html
[sphinx-napoleon]: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html#
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/__gtscript__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import sys

from gt4py.cartesian.gtscript import *
from gt4py.cartesian.gtscript import * # noqa: F403 [undefined-local-with-import-star]


sys.modules["__gtscript__"] = sys.modules["gt4py.cartesian.__gtscript__"]
17 changes: 17 additions & 0 deletions src/gt4py/cartesian/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,20 @@
type_hints,
)
from .stencil_object import StencilObject


__all__ = [
"typing",
"caching",
"cli",
"config",
"definitions",
"frontend",
"gt_cache_manager",
"gtscript",
"loader",
"stencil_builder",
"stencil_object",
"type_hints",
"StencilObject",
]
4 changes: 3 additions & 1 deletion src/gt4py/cartesian/backend/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@ def check_options(self, options: gt_definitions.BuildOptions) -> None:
unknown_options = set(options.backend_opts.keys()) - set(self.options.keys())
if unknown_options:
warnings.warn(
f"Unknown options '{unknown_options}' for backend '{self.name}'", RuntimeWarning
f"Unknown options '{unknown_options}' for backend '{self.name}'",
RuntimeWarning,
stacklevel=2,
)

def make_module(
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/backend/dace_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ def _postprocess_dace_code(code_objects, is_gpu, builder):
break
for i, line in enumerate(lines):
if "#include <dace/dace.h>" in line:
cuda_code = [co.clean_code for co in code_objects if co.title == "CUDA"][0]
cuda_code = next(co.clean_code for co in code_objects if co.title == "CUDA")
lines = lines[0:i] + cuda_code.split("\n") + lines[i + 1 :]
break

Expand Down
7 changes: 3 additions & 4 deletions src/gt4py/cartesian/backend/pyext_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ def build_pybind_ext(
libraries: Optional[List[str]] = None,
extra_compile_args: Optional[Union[List[str], Dict[str, List[str]]]] = None,
extra_link_args: Optional[List[str]] = None,
build_ext_class: Type = None,
build_ext_class: Optional[Type] = None,
verbose: bool = False,
clean: bool = False,
) -> Tuple[str, str]: ...
Expand All @@ -211,7 +211,7 @@ def build_pybind_ext(
libraries: Optional[List[str]] = None,
extra_compile_args: Optional[Union[List[str], Dict[str, List[str]]]] = None,
extra_link_args: Optional[List[str]] = None,
build_ext_class: Type = None,
build_ext_class: Optional[Type] = None,
verbose: bool = False,
clean: bool = False,
) -> Tuple[str, str]:
Expand Down Expand Up @@ -242,7 +242,6 @@ def build_pybind_ext(
ext_modules=[py_extension],
script_args=[
"build_ext",
# "--parallel={}".format(gt_config.build_settings["parallel_jobs"]),
"--build-temp={}".format(build_path),
"--build-lib={}".format(build_path),
"--force",
Expand Down Expand Up @@ -336,7 +335,7 @@ def build_pybind_cuda_ext(

def _clean_build_flags(config_vars: Dict[str, str]) -> None:
for key, value in config_vars.items():
if type(value) == str:
if isinstance(value, str):
value = " " + value + " "
for s in value.split(" "):
if (
Expand Down
4 changes: 2 additions & 2 deletions src/gt4py/cartesian/frontend/defir_to_gtir.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ def visit_StencilDefinition(
def _nested_list_dim(self, a: List) -> List[int]:
if not isinstance(a, list):
return []
return [len(a)] + self._nested_list_dim(a[0])
return [len(a), *self._nested_list_dim(a[0])]

def visit_Assign(
self, node: Assign, *, fields_decls: Dict[str, FieldDecl], **kwargs
Expand Down Expand Up @@ -181,7 +181,7 @@ def apply(cls, root, *, expected_dim: Tuple[int, ...], fields_decls: Dict[str, F
# if the expression is just a scalar broadcast to the expected dimensions
if not isinstance(result, list):
result = functools.reduce(
lambda val, len: [val for _ in range(len)], reversed(expected_dim), result
lambda val, len_: [val for _ in range(len_)], reversed(expected_dim), result
)
return result

Expand Down
26 changes: 12 additions & 14 deletions src/gt4py/cartesian/frontend/gtscript_frontend.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,15 +211,15 @@ def visit_BinOp(self, node: ast.BinOp) -> Union[gtscript.AxisIndex, nodes.AxisBo
right = self.visit(node.right)

if isinstance(node.op, ast.Add):
bin_op = lambda x, y: x + y # noqa: E731
u_op = lambda x: x # noqa: E731
bin_op = lambda x, y: x + y # noqa: E731 [lambda-assignment]
u_op = lambda x: x # noqa: E731 [lambda-assignment]
elif isinstance(node.op, ast.Sub):
bin_op = lambda x, y: x - y # noqa: E731
u_op = lambda x: -x # noqa: E731
bin_op = lambda x, y: x - y # noqa: E731 [lambda-assignment]
u_op = lambda x: -x # noqa: E731 [lambda-assignment]
elif isinstance(node.op, ast.Mult):
if left.level != right.level or not isinstance(left.level, nodes.LevelMarker):
raise self.interval_error
bin_op = lambda x, y: x * y # noqa: E731
bin_op = lambda x, y: x * y # noqa: E731 [lambda-assignment]
u_op = None
else:
raise GTScriptSyntaxError("Unexpected binary operator found in interval expression")
Expand Down Expand Up @@ -249,7 +249,7 @@ def visit_BinOp(self, node: ast.BinOp) -> Union[gtscript.AxisIndex, nodes.AxisBo

def visit_UnaryOp(self, node: ast.UnaryOp) -> nodes.AxisBound:
if isinstance(node.op, ast.USub):
op = lambda x: -x # noqa: E731
op = lambda x: -x # noqa: E731 [lambda-assignment]
else:
raise self.interval_error

Expand Down Expand Up @@ -417,9 +417,7 @@ def visit_Assign(self, node: ast.Assign):
else:
return self.generic_visit(node)

def visit_Call( # Cyclomatic complexity too high
self, node: ast.Call, *, target_node=None
):
def visit_Call(self, node: ast.Call, *, target_node=None): # Cyclomatic complexity too high
call_name = gt_meta.get_qualified_name_from_node(node.func)

if call_name in self.call_stack:
Expand Down Expand Up @@ -461,10 +459,10 @@ def visit_Call( # Cyclomatic complexity too high
if name not in call_args:
assert arg_infos[name] != nodes.Empty
call_args[name] = ast.Constant(value=arg_infos[name])
except Exception:
except Exception as ex:
raise GTScriptSyntaxError(
message="Invalid call signature", loc=nodes.Location.from_ast_node(node)
)
) from ex

# Rename local names in subroutine to avoid conflicts with caller context names
try:
Expand Down Expand Up @@ -601,7 +599,7 @@ def visit_If(self, node: ast.If):
and node.test.func.id == "__INLINED"
and len(node.test.args) == 1
):
warnings.warn(
warnings.warn( # noqa: B028 [no-explicit-stacklevel]
f"stencil {self.stencil_name}, line {node.lineno}, column {node.col_offset}: compile-time if condition via __INLINED deprecated",
category=DeprecationWarning,
)
Expand Down Expand Up @@ -1059,10 +1057,10 @@ def _eval_new_spatial_index(
for index_node in index_nodes:
try:
value = gt_meta.ast_eval(index_node, axis_context)
except Exception:
except Exception as ex:
raise GTScriptSyntaxError(
message="Could not evaluate axis shift expression.", loc=index_node
)
) from ex
if not isinstance(value, (gtscript.ShiftedAxis, gtscript.Axis)):
raise GTScriptSyntaxError(
message=f"Axis shift expression evaluated to unrecognized type {type(value)}.",
Expand Down
4 changes: 2 additions & 2 deletions src/gt4py/cartesian/frontend/node_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def generic_visit(self, node: Node, **kwargs):
else:
pass

for key, value in items:
for _, value in items:
self._visit(value, **kwargs)


Expand Down Expand Up @@ -122,7 +122,7 @@ def iter_nodes_of_type(root_node: Node, node_type: Type) -> Generator[Node, None
"""Yield an iterator over the nodes of node_type inside root_node in DFS order."""

def recurse(node: Node) -> Generator[Node, None, None]:
for key, value in iter_attributes(node):
for _, value in iter_attributes(node):
if isinstance(node, collections.abc.Iterable):
if isinstance(node, collections.abc.Mapping):
children = node.values()
Expand Down
18 changes: 0 additions & 18 deletions src/gt4py/cartesian/frontend/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,15 +322,6 @@ class ScalarLiteral(Literal):
loc = attribute(of=Location, optional=True)


# @attribclass
# class TupleLiteral(Node):
# items = attribute(of=TupleOf[Expr])
#
# @property
# def length(self):
# return len(self.items)


@attribclass
class BuiltinLiteral(Literal):
value = attribute(of=Builtin)
Expand Down Expand Up @@ -593,12 +584,6 @@ class Statement(Node):
pass


# @attribclass
# class ExprStmt(Statement):
# expr = attribute(of=Expr)
# loc = attribute(of=Location, optional=True)


class Decl(Statement):
pass

Expand Down Expand Up @@ -722,9 +707,6 @@ def is_single_index(self) -> bool:
return self.start.level == self.end.level and self.start.offset == self.end.offset - 1

def disjoint_from(self, other: "AxisInterval") -> bool:
# This made-up constant must be larger than any LevelMarker.offset used
DOMAIN_SIZE: int = 1000

def get_offset(bound: AxisBound) -> int:
return (
0 + bound.offset if bound.level == LevelMarker.START else sys.maxsize + bound.offset
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/dace/expansion/tasklet_codegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def _visit_conditional(
indent = " " * 4
body_code = [line for block in self.visit(body, **kwargs) for line in block.split("\n")]
body_code = [indent + b for b in body_code]
return "\n".join([mask_str] + body_code)
return "\n".join([mask_str, *body_code])

def visit_MaskStmt(self, node: dcir.MaskStmt, **kwargs):
return self._visit_conditional(cond=node.mask, body=node.body, keyword="if", **kwargs)
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/daceir.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class Stmt(common.Stmt):


class Axis(eve.StrEnum):
I = "I" # noqa: E741 ambiguous variable name 'I'
I = "I" # noqa: E741 [ambiguous-variable-name]
J = "J"
K = "K"

Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/definitions.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
class CartesianSpace:
@enum.unique
class Axis(enum.Enum):
I = 0 # noqa: E741 # Do not use variables named 'I', 'O', or 'l'
I = 0 # noqa: E741 [ambiguous-variable-name]
J = 1
K = 2

Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/gtcpp/oir_to_gtcpp.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from dataclasses import dataclass, field
from typing import Any, Callable, Dict, List, Set, Union, cast

from devtools import debug # noqa: F401
from devtools import debug # noqa: F401 [unused-import]
from typing_extensions import Protocol

from gt4py import eve
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/numpy/npir.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

# --- Misc ---
class AxisName(eve.StrEnum):
I = "I" # noqa: E741 (ambiguous variable name)
I = "I" # noqa: E741 [ambiguous-variable-name]
J = "J"
K = "K"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,6 @@ def check(gtir_stencil_expr: gtir.Stencil) -> gtir.Stencil:
"""Execute definitive assignment analysis and warn on errors."""
invalid_accesses = analyze(gtir_stencil_expr)
for invalid_access in invalid_accesses:
warnings.warn(f"`{invalid_access.name}` may be uninitialized.")
warnings.warn(f"`{invalid_access.name}` may be uninitialized.", stacklevel=2)

return gtir_stencil_expr
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ def first_has_horizontal_restriction() -> bool:
or first_has_variable_access()
or first_has_horizontal_restriction()
):
return [first] + self._merge(others, symtable, new_symbol_name, protected_fields)
return [first, *self._merge(others, symtable, new_symbol_name, protected_fields)]

first_scalars = {decl.name for decl in first.declarations}
writes = first_accesses.write_fields()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ def _mergeable(a: oir.VerticalLoop, b: oir.VerticalLoop) -> bool:
def _merge(a: oir.VerticalLoop, b: oir.VerticalLoop) -> oir.VerticalLoop:
sections = a.sections + b.sections
if a.caches or b.caches:
warnings.warn("AdjacentLoopMerging pass removed previously declared caches")
warnings.warn(
"AdjacentLoopMerging pass removed previously declared caches", stacklevel=2
)
return oir.VerticalLoop(
loop_order=a.loop_order,
sections=sections,
Expand Down
2 changes: 1 addition & 1 deletion src/gt4py/cartesian/gtc/ufuncs.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
not_equal: np.ufunc = np.not_equal
logical_and: np.ufunc = np.logical_and
logical_or: np.ufunc = np.logical_or
abs: np.ufunc = np.abs # noqa: A001 # shadowing abs builtin
abs: np.ufunc = np.abs # noqa: A001 [builtin-variable-shadowing]
minimum: np.ufunc = np.minimum
maximum: np.ufunc = np.maximum
remainder: np.ufunc = np.remainder
Expand Down
Loading

0 comments on commit 77a205b

Please sign in to comment.