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.

Sign up for GitHub

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

Jump to bottom

[tune] Raise an error if Tuner.restore() is called on an instance #39676

Merged
merged 1 commit into from
Sep 15, 2023
Merged

[tune] Raise an error if Tuner.restore() is called on an instance #39676

merged 1 commit into from
Sep 15, 2023

Conversation

krfricke
Copy link
Contributor

Why are these changes needed?

Tuner.restore() is used to construct a Tuner object from an existing experiment. However, calling tuner = Tuner(...); tuner.restore() is a common misuse of the API and does not throw an error.

This PR updates the Tuner class to only allow restore() to be called on the class, not on an instance of the class.

Related issue number

Closes #39674

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 :(

[tune] Raise an error if Tuner.restore() is called on an instance
bf46391 
Signed-off-by: Kai Fricke <kai@anyscale.com>
matthewdeng
matthewdeng approved these changes Sep 15, 2023
View reviewed changes
python/ray/tune/tuner.py
Comment on lines +436 to +437
def __getattribute__(self, item):
if item == "restore":
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...

python/ray/tune/tuner.py

def __getattribute__(self, item):
if item == "restore":
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 •
edited
Loading

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)"

@krfricke krfricke merged commit 71f0ae7 into ray-project:master Sep 15, 2023
59 of 62 checks passed
@krfricke krfricke deleted the tune/tuner-restore-classmethod branch September 15, 2023 17:27
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@krfricke
[tune] Raise an error if Tuner.restore() is called on an instance (r…
f5d88de 
…ay-project#39676)

`Tuner.restore()` is used to construct a `Tuner` object from an existing experiment. However, calling `tuner = Tuner(...); tuner.restore()` is a common misuse of the API and does not throw an error.

This PR updates the `Tuner` class to only allow `restore()` to be called on the class, not on an instance of the class.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewdeng matthewdeng matthewdeng approved these changes

@sven1977 sven1977 Awaiting requested review from sven1977

Assignees

@matthewdeng matthewdeng

@sven1977 sven1977

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[tune] Tuner.restore() also works on objects
3 participants
@krfricke @matthewdeng @sven1977