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

Abstract base classes have incorrectly decorated methods #6943

Open
smokestacklightnin opened this issue Oct 27, 2024 · 7 comments
Open

Abstract base classes have incorrectly decorated methods #6943

smokestacklightnin opened this issue Oct 27, 2024 · 7 comments

Comments

@smokestacklightnin
Copy link
Contributor

If the bug is related to a specific library below, please raise an issue in the
respective repo directly: TFX

System information

  • Have I specified the code to reproduce the issue (Yes, No): Yes
  • Environment in which the code is executed (e.g., Local(Linux/MacOS/Windows),
    Interactive Notebook, Google Cloud, etc): All
  • TensorFlow version: All
  • TFX Version: 1.15.1
  • Python version: All
  • Python dependencies (from pip freeze output): All

Describe the current behavior

There are currently violations of Ruff rules B024 and B027, which have to do with incorrectly decorated class methods/properties. Fixing them currently causes errors (See #6942), so that issue should be dealt with before this one.

Describe the expected behavior

See python docs for the correct way to make methods and properties abstract.

Other info / logs

The code violating B024 and B027 is listed below:

tfx/orchestration/portable/base_executor_operator.py:87:3: B027 `BaseExecutorOperator.handle_stop` is an empty method in an abstract base class, but has no abstract decorator
   |
85 |       return self
86 |   
87 |     def handle_stop(self) -> None:
   |  ___^
88 | |     """Executor Operator specific logic to clean up after it is stopped."""
89 | |     pass
   | |________^ B027
   |

tfx/tools/cli/handler/dag_runner_patcher.py:59:3: B027 `DagRunnerPatcher._before_run` is an empty method in an abstract base class, but has no abstract decorator
   |
57 |       self._call_real_run = call_real_run
58 |   
59 |     def _before_run(self, runner: tfx_runner.TfxRunner,
   |  ___^
60 | |                   pipeline: Union[pipeline_pb2.Pipeline, tfx_pipeline.Pipeline],
61 | |                   context: MutableMapping[str, Any]) -> None:
62 | |     pass
   | |________^ B027
63 |   
64 |     def _after_run(self, runner: tfx_runner.TfxRunner,
   |

tfx/tools/cli/handler/dag_runner_patcher.py:64:3: B027 `DagRunnerPatcher._after_run` is an empty method in an abstract base class, but has no abstract decorator
   |
62 |       pass
63 |   
64 |     def _after_run(self, runner: tfx_runner.TfxRunner,
   |  ___^
65 | |                  pipeline: Union[pipeline_pb2.Pipeline, tfx_pipeline.Pipeline],
66 | |                  context: MutableMapping[str, Any]) -> None:
67 | |     pass
   | |________^ B027
68 |   
69 |     @abc.abstractmethod
   |

tfx/types/system_artifacts.py:24:7: B024 `SystemArtifact` is an abstract base class, but it has no abstract methods
   |
24 | class SystemArtifact(abc.ABC):
   |       ^^^^^^^^^^^^^^ B024
25 |   """TFX system artifact base class.
   |

tfx/types/system_executions.py:24:7: B024 `SystemExecution` is an abstract base class, but it has no abstract methods
   |
24 | class SystemExecution(abc.ABC):
   |       ^^^^^^^^^^^^^^^ B024
25 |   """TFX system execution base class.
   |

Found 5 errors.
@janasangeetha
Copy link
Contributor

Hi @smokestacklightnin,
Thank you for reporting. I'll take a look and provide an update here.

@lego0901
Copy link
Member

lego0901 commented Nov 5, 2024

QQ. How could you see those Ruff errors?

@smokestacklightnin
Copy link
Contributor Author

QQ. How could you see those Ruff errors?

Using the ruff check command

@lego0901
Copy link
Member

lego0901 commented Nov 5, 2024

Hmm.. My ruff check shows different errors like F401, and there is no B024 and B027. Could you let me know how to reproduce your output?

@smokestacklightnin
Copy link
Contributor Author

This on example of a command I used on the master branch:

$ ruff check tfx --exclude tfx/examples/airflow_workshop/taxi/notebooks/*.ipynb --select B

from the root project directory

@lego0901
Copy link
Member

lego0901 commented Nov 5, 2024

Great! Now I see the message. Thanks a lot!

@janasangeetha
Copy link
Contributor

Hi @smokestacklightnin
#6950 is merged. Please feel free to validate and close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants