Fix Ruff rule B027 and B024 #6950
Conversation
| """Executor Operator specific logic to clean up after it is stopped.""" | ||
| pass | ||
|
|
||
| class BaseExecutorOperator(ParentBaseExecutorOperator): |
There was a problem hiding this comment.
Is this really needed? I think it is sufficient to have handle_stop to be an abstractmethod, without needing to introduce ParentBaseExecutorOperator. I guess it is normal to have handle_stop implementation for any class that inherits BaseExecutorOperator.
| def _before_run(self, runner: tfx_runner.TfxRunner, | ||
| pipeline: Union[pipeline_pb2.Pipeline, tfx_pipeline.Pipeline], | ||
| context: MutableMapping[str, Any]) -> None: | ||
| pass | ||
|
|
||
| @abc.abstractmethod |
There was a problem hiding this comment.
Same here (and above _before_run). I believe all we need is @abc.abstractmethod decorators.
|
SG for B024. Please tackle me if anything is wrong with my comments on B027. Thanks! |
|
I see.. Thanks. Indeed those methods are intentionally no-ops unless they are overridden. |
tfx/types/system_artifacts.py
Outdated
| @@ -30,7 +30,7 @@ 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. | |||
| """ | |||
|
|
|||
| # noqa: B024 | |||
There was a problem hiding this comment.
This wasn't effective. Please try it out with
class SystemArtifact(abc.ABC): # noqa: B024
tfx/types/system_executions.py
Outdated
| @@ -30,7 +30,7 @@ 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. | |||
| """ | |||
|
|
|||
| # noqa: B024 | |||
| The output from executor. | ||
| """ | ||
| pass | ||
|
|
||
| def handle_stop(self) -> None: |
There was a problem hiding this comment.
You can suppress B027 check via
def handle_stop(self) -> None: # noqa: B027
|
Please make sure that everything is clear if you run Thanks! |
| @@ -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 | |||
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I don't believe the number of spaces matters. I tried with zero, one, and two spaces, and all three options passed ruff check.
Fix Ruff rules
#6943