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

[Refactor]Fix ruff rule B024: abstract-base-class-without-abstract-method(setting) #49917

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

simotw
Copy link

@simotw simotw commented Jan 17, 2025

Why are these changes needed?

according to the abstract-base-class-without-abstract-method rule

Abstract base classes are used to define interfaces. If an abstract base class has no abstract methods or properties, you may have forgotten to add an abstract method or property to the class, or omitted an @AbstractMethod decorator.

If the class is not meant to be used as an interface, consider removing the ABC base class from the class definition.

According to the discussion below.
This PR is design to be the setting to divide the task into several small PRs.

Related issue number

#47991

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@MortalHappiness MortalHappiness added the go add ONLY when ready to merge, run all tests label Jan 17, 2025
@simotw simotw force-pushed the refactor/migrate-to-ruff-linter-fix-b024 branch 2 times, most recently from 0ba9171 to cd42eb8 Compare January 19, 2025 13:09
@simotw simotw marked this pull request as ready for review January 20, 2025 03:27
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

simply removing ABC is unlikely to be the right fix for most of these cases. The right fix is more likely to be adding @abstractmethod for methods that are abstract.

from typing import List, Union

import torch


class TorchDeviceManager(ABC):
class TorchDeviceManager:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not very familiar with this part of the code, but these fixes are unlikely the right fix.

reading the code, it is pretty clear that this class is an interface class. the right fix is more likely to be declaring all the methods as @abstractmethod

Copy link
Author

@simotw simotw Jan 21, 2025

Choose a reason for hiding this comment

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

Hi @aslonnie, I tried adding @abstractmethod, but the tests failed because it raises an error if child classes don't implement the method.

So, simply adding @abstractmethod to methods with NotImplementedError won't work.

That said, I understand your point about maintaining the abstract structure.

Copy link
Member

@MortalHappiness MortalHappiness Jan 21, 2025

Choose a reason for hiding this comment

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

In this case, you should annotate all methods in TorchDeviceManager with @abstractmethod. Then, for all classes inheriting from it, such as CPUTorchDeviceManager, if any methods from TorchDeviceManager are not implemented, provide an implementation with a method body that raises NotImplementedError.

Looks like this:

class TorchDeviceManager(ABC):
    @abstractmethod
    def get_current_stream(self):
        """Get current stream on accelerators like torch.cuda.current_stream"""
        ...
class CPUTorchDeviceManager(TorchDeviceManager):
    def get_current_stream(self):
        raise NotImplementedError

@simotw simotw changed the title [Refactor] Fix ruff rule B024: abstract-base-class-without-abstract-method [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method Jan 21, 2025
@simotw simotw force-pushed the refactor/migrate-to-ruff-linter-fix-b024 branch from e9c3c23 to 3efec3b Compare January 21, 2025 06:45
@simotw simotw marked this pull request as draft January 21, 2025 08:40
@MortalHappiness
Copy link
Member

Let's divide this PR into several smaller PRs for easier review and debugging in CI.

Refer to the Ruff settings for details:

https://docs.astral.sh/ruff/settings/#lint_per-file-ignores

@simotw simotw force-pushed the refactor/migrate-to-ruff-linter-fix-b024 branch from 3efec3b to 72b20be Compare January 22, 2025 06:42
@simotw simotw changed the title [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method(Base setting) Jan 22, 2025
@simotw simotw changed the title [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method(Base setting) [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method(setting) Jan 22, 2025
@simotw simotw changed the title [Refactor][WIP] Fix ruff rule B024: abstract-base-class-without-abstract-method(setting) [Refactor]Fix ruff rule B024: abstract-base-class-without-abstract-method(setting) Jan 22, 2025
@simotw simotw force-pushed the refactor/migrate-to-ruff-linter-fix-b024 branch from 72b20be to 8366487 Compare January 22, 2025 07:44
@simotw simotw marked this pull request as ready for review January 22, 2025 07:46
@simotw
Copy link
Author

simotw commented Jan 22, 2025

Let's divide this PR into several smaller PRs for easier review and debugging in CI.

Refer to the Ruff settings for details:

https://docs.astral.sh/ruff/settings/#lint_per-file-ignores

Good idea.
I have updated this PR to be the setting PR.
PTAL
You can expect smaller PRs for each fix later.
Thank you!

@MortalHappiness MortalHappiness removed the go add ONLY when ready to merge, run all tests label Jan 22, 2025
@simotw simotw marked this pull request as draft January 22, 2025 13:55
@simotw simotw marked this pull request as ready for review January 23, 2025 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants