Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Ruff rule B027 and B024 #6950

Merged
merged 8 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tfx/orchestration/portable/base_executor_operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,6 @@ def with_execution_watcher(
self._execution_watcher_address = execution_watcher_address
return self

def handle_stop(self) -> None:
def handle_stop(self) -> None:# noqa: B027
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also a nit comment; as a coding convention, we were using two spaces before any comment #. This was warned when we are on the internal Google land, but I am not sure if it is captured by the linter here.

@smokestacklightnin could you let me know if this comment style is acceptable by the linter? pylint --rcfile ./pylintrc tfx/types/system_executions.py didn't warn me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe the number of spaces matters. I tried with zero, one, and two spaces, and all three options passed ruff check.

"""Executor Operator specific logic to clean up after it is stopped."""
pass
2 changes: 1 addition & 1 deletion tfx/tools/cli/handler/dag_runner_patcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""Base class to patch DagRunner classes in TFX CLI."""

#ruff: noqa: B027
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you prefer global noqa over specific lines? I personally lean toward using # noqa: B027 only for specific lines because new contributors could ignore all B027 rules on the same file, which isn't desirable for the code health.

import abc
import contextlib
import functools
Expand Down
3 changes: 1 addition & 2 deletions tfx/types/system_artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from ml_metadata.metadata_store import mlmd_types


class SystemArtifact(abc.ABC):
class SystemArtifact(abc.ABC):# noqa: B024
"""TFX system artifact base class.

A user may create a subclass of SystemArtifact and override the
Expand All @@ -30,7 +30,6 @@ class SystemArtifact(abc.ABC):
The subclasses, e.g, Dataset, Model, Statistics, e.t.c, match the MLMD types
from third_party/ml_metadata/metadata_store/mlmd_types.py.
"""

# MLMD system base type enum. Override it when creating subclasses.
MLMD_SYSTEM_BASE_TYPE = None

Expand Down
3 changes: 1 addition & 2 deletions tfx/types/system_executions.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from ml_metadata.metadata_store import mlmd_types


class SystemExecution(abc.ABC):
class SystemExecution(abc.ABC):# noqa: B024
"""TFX system execution base class.

A user may create a subclass of SystemExecution and override the
Expand All @@ -30,7 +30,6 @@ class SystemExecution(abc.ABC):
The subclasses, e.g, Train, Transform, Process, e.t.c, match the MLMD types
from third_party/ml_metadata/metadata_store/mlmd_types.py.
"""

# MLMD system base type enum. Override it when creating subclasses.
MLMD_SYSTEM_BASE_TYPE = None

Expand Down
Loading