[air] Better exception handling #23695
Conversation
| """Returns a PlacementGroupFactory to specify resources for Tune.""" | ||
| from ray.tune.trainable import PlacementGroupFactory |
There was a problem hiding this comment.
Avoid circular imports
| from ray.ml.config import RunConfig, ScalingConfig | ||
| from ray.ml.preprocessor import Preprocessor | ||
|
|
||
| __all__ = ["Checkpoint", "Preprocessor", "RunConfig", "ScalingConfig"] |
There was a problem hiding this comment.
Just a minor point, but I personally like having users do fully qualified package imports instead of putting things into the ray.ml parent package.
There was a problem hiding this comment.
I think we may want to discuss this sometime - I think for the main concepts we can move them to the parent package as they will be used often. We shouldn't do this e.g. for trainer implementations
| def _validate_attributes(self): | ||
| super()._validate_attributes() | ||
|
|
There was a problem hiding this comment.
Yes, otherwise the parent classes validation is not executed
There was a problem hiding this comment.
Hmm if we're only calling just the super class method, then it doesn't seem necessary?
There was a problem hiding this comment.
I don't get it - we're doing more validation under the super call?
There was a problem hiding this comment.
Like in lines 218ff
There was a problem hiding this comment.
Ohh I see, sorry that didn't show up in the diff. Yes you're right, there's more validation being done below
|
@matthewdeng I've removed the custom exceptions for now, we should probably have a discussion on how to introduce these, first. |
| def _validate_attributes(self): | ||
| super()._validate_attributes() | ||
|
|
There was a problem hiding this comment.
Ohh I see, sorry that didn't show up in the diff. Yes you're right, there's more validation being done below
What: Raise meaningful exceptions when invalid parameters are passed. Why: We want to catch invalid parameters and guide users to use the API in the correct way.
…ject#23732) This reverts commit fb50e0a.
Why are these changes needed?
What: Raise meaningful exceptions when invalid parameters are passed.
Why: We want to catch invalid parameters and guide users to use the API in the correct way.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.