[rllib] bugfix: DO check if the owner is a subclass of the expected base class

[AYou0207]

Description

The override of ray.rllib.utils.annotations does not check whether the owner is a subclass of the expected base class.
This PR fix it.

Related issues

none.

Additional information

The minimul test code:

from ray.rllib.utils.annotations import override

class A:
  def should_be_overrided(self):
    pass

class B(A):
  @override
  def should_be_override(self):
    pass

class C:
  @override
  def should_be_override(self):
    pass

# this line should be ok
B()
# this line is expected to raise errors because the C is not the subclass of A
C()
# but now it will pass

The PR can make the code work as expected.

Additional info: PEP-487

[gemini-code-assist]

Code Review

This pull request addresses a bug in the @override decorator, which was not correctly verifying if a class is a subclass of the expected base class. The fix involves returning the OverrideCheck descriptor instance from the decorator, which ensures its __set_name__ method is invoked during class creation to perform the subclass validation. This change is correct and effectively resolves the issue. The implementation is clean, and I have no further suggestions.

[cursor]

Bug: Decorator stacking loses __is_overridden__ attribute

When @OverrideToImplementCustomLogic_CallToSuperRecommended or @OverrideToImplementCustomLogic is stacked above @override, the outer decorator sets __is_overridden__ = False on the OverrideCheck descriptor instance. However, __set_name__ replaces the descriptor with self.func (the original function) via setattr(owner, name, self.func), discarding the attribute. The is_overridden() function then returns True (the default) instead of False. This breaks existing code patterns like @OverrideToImplementCustomLogic_CallToSuperRecommended @override(Trainable) def setup(...) in algorithm.py.

Additional Locations (1)

• rllib/utils/annotations.py#L41-L42

 

[AYou0207]

I checked the concern. The Cursor is right.

For now, the PR failed the following checks:

from ray.rllib.utils.annotations import override, OverrideToImplementCustomLogic

class A:
  def should_be_overrided(self):
    pass

class B(A):
   @OverrideToImplementCustomLogic
   @override(A)
  def should_be_overrided(self):
    pass

b = B()
# we cannot pass the following assert
# because we do not set __is_overridden__ anymore
# (__set_name__ is called after OverrideToImplementCustomLogic)
assert b.should_be_overrided.__is_overridden__ is False

The fix is coming.

[AYou0207]

@cursoragent check the new PR again, thanks

[cursor]

Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor

[cursor]

Bug: @classmethod @override pattern makes method non-callable

When @classmethod (or @staticmethod) is applied after @override, the OverrideCheck instance is wrapped by classmethod instead of the actual function. Since classmethod.__set_name__ doesn't propagate to OverrideCheck.__set_name__, the descriptor is never replaced with the actual function. When the method is accessed and called, it attempts to invoke the OverrideCheck instance directly, which lacks __call__, raising a TypeError. This pattern exists in the codebase, e.g., @classmethod @override(Checkpointable) def from_checkpoint in algorithm.py.

 

[AYou0207]

Yes, the Cursor is correct. And I am working on it.

[AYou0207]

@override should not take with @classmethod or @staticmethod. It is a good choice to raise errors when @classmethod & @staticmethod applied to @override methods.