Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 15 additions & 0 deletions python/ray/tune/tests/test_tuner.py
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,21 @@ def to_dict(self) -> dict:
tune.run(trainable, config=CustomConfig())


def test_tuner_restore_classmethod():
tuner = Tuner("PPO")

# Calling `tuner.restore()` on an instance should raise an AttributeError
with pytest.raises(AttributeError):
tuner.restore("/", "PPO")

# Calling `Tuner.restore()` on the class should work. This will throw a
# FileNotFoundError because no checkpoint exists at that location. Since
# this happens in the downstream restoration code, this means that the
# classmethod check successfully passed.
with pytest.raises(FileNotFoundError):
tuner = Tuner.restore("/invalid", "PPO")


if __name__ == "__main__":
import sys

Expand Down
9 changes: 9 additions & 0 deletions python/ray/tune/tuner.py
Original file line number Diff line number Diff line change
Expand Up @@ -432,3 +432,12 @@ def get_results(self) -> ResultGrid:
string_queue,
)
return ray.get(get_results_future)

def __getattribute__(self, item):
if item == "restore":
Comment on lines +436 to +437
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was a cleaner way to do this directly in the definition of restore, but I'm not sure if there is one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, at least I can't think of one right now. The @classmethod decorator passes the class in both cases...

raise AttributeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be TypeError instead?

Copy link
Contributor Author

@krfricke krfricke Sep 15, 2023

Choose a reason for hiding this comment

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

From https://docs.python.org/3/library/exceptions.html#AttributeError

exception AttributeError
Raised when an attribute reference (see Attribute references) or assignment fails. (When an object does not support attribute references or attribute assignments at all, TypeError is raised.)

And

https://docs.python.org/3/reference/expressions.html#attribute-references

If this attribute is not available, the exception AttributeError is raised.

I think AttributeError is correct. Tuner generally supports attribute references, but not this one. It's supposed to be like "this attribute does not exist for an instance (only for the class)"

"`Tuner.restore()` is a classmethod and cannot be called on an "
"instance. Use `tuner = Tuner.restore(...)` to instantiate the "
"Tuner instead."
)
return super().__getattribute__(item)