Extended operator errors and added forecast specific exceptions #360
Conversation
prasankh commented Sep 29, 2023
prasankh commented - Introduced OperatorSchemaYamlError, ForecastSchemaYamlError and ForecastInputDataError.
- Made target_column and target_column mandatory.
- Updated code with relevant errors.
oracle-contributor-agreement bot added the OCA Verified 
| target_column: | ||
| type: string | ||
| required: true | ||
| default: target |
There was a problem hiding this comment.
Why did we remove this? I think it's used in the ads opctl init method
There was a problem hiding this comment.
Correct I use default field when I build forecast specification yaml based by schema. For the required fields it is better to have some default values.
There was a problem hiding this comment.
Okay, reverted this default value.
| target_category_columns: | ||
| type: list | ||
| required: false | ||
| required: true |
There was a problem hiding this comment.
Does the code break without specifying this series column? If so we should open a ticket to change that
There was a problem hiding this comment.
Yes, It breaks and the error message is not meaningful hence made this mandatory.
ads/opctl/operator/cmd.py Outdated
| # validation | ||
| if not name: | ||
| raise ValueError( | ||
| raise OperatorSchemaYamlError( |
There was a problem hiding this comment.
Why do we want to change ValueError on the OperatorSchemaYamlError here? The function gets the name attribute, and if the name attribute not provided we just show a message that name is expected. I think switching to the OperatorSchemaYamlError might be a bit confusing, because we don't expect YAML here as input attribute. The same for the other places in the CMD. There are only two methods that expect a YAML as input attribute - run and verify.
There was a problem hiding this comment.
Reverted these changes in cmd.py
| target_column: | ||
| type: string | ||
| required: true | ||
| default: target |
There was a problem hiding this comment.
Correct I use default field when I build forecast specification yaml based by schema. For the required fields it is better to have some default values.
| from ads.common.serializer import DataClassSerializable | ||
| from ads.opctl.operator.common.utils import _load_yaml_from_uri | ||
| | ||
| from ..common.errors import OperatorSchemaYamlError |
There was a problem hiding this comment.
Let's use absolute imports for the operator level modules and classes.
from ads.opctl.operator.common.errors import OperatorSchemaYamlError We can use relative imports inside the particular operator, but for the core modules it would be better to use absolute imports.
| | ||
| from .common.utils import OperatorValidator | ||
| | ||
| from .common.errors import OperatorSchemaYamlError |
There was a problem hiding this comment.
Let's replace this with absolute imports.
from ads.opctl.operator.common.utils import OperatorValidator from ads.opctl.operator.common.errors import OperatorSchemaYamlError 
| Looks like there are some conflicts with the target branch. We will need to merge the |
prasankh force-pushed the feature/forecasting-exceptions branch from df7b726 to b8911c8 Compare 4 participants
