[AGENT-6608][AGENT-6612][AGENT-6613] Add use_datarobot_predict and time series args to server and score command parsers. #1240
Conversation
…me series args to server and score command parsers.
…to pipeline config.
… predict_structured.
|
The Needs Review labels were added based on the following file changes. Team @datarobot/core-modeling (#core-modeling) was assigned because of changes in files:custom_model_runner/datarobot_drum/drum/adapters/cli/drum_score_adapter.py custom_model_runner/datarobot_drum/drum/args_parser.py custom_model_runner/datarobot_drum/drum/drum.py custom_model_runner/datarobot_drum/drum/enum.py custom_model_runner/datarobot_drum/drum/root_predictors/drum_server_utils.py custom_model_runner/datarobot_drum/drum/root_predictors/predict_mixin.py custom_model_runner/datarobot_drum/resource/pipelines/prediction_pipeline.json.j2 custom_model_runner/datarobot_drum/resource/pipelines/prediction_server_pipeline.json.j2 Team @datarobot/custom-models (#custom-models) was assigned because of changes in files:custom_model_runner/datarobot_drum/drum/adapters/cli/drum_score_adapter.py custom_model_runner/datarobot_drum/drum/args_parser.py custom_model_runner/datarobot_drum/drum/drum.py custom_model_runner/datarobot_drum/drum/enum.py custom_model_runner/datarobot_drum/drum/root_predictors/drum_server_utils.py custom_model_runner/datarobot_drum/drum/root_predictors/predict_mixin.py custom_model_runner/datarobot_drum/resource/pipelines/prediction_pipeline.json.j2 custom_model_runner/datarobot_drum/resource/pipelines/prediction_server_pipeline.json.j2 tests/unit/datarobot_drum/drum/test_args_parser.py tests/unit/datarobot_drum/drum/test_drum.py If you think that there are some issues with ownership, please discuss with C&A domain at #sdtk slack channel and create PR to update DRCODEOWNERS\CODEOWNERS file. |
kijimoshib
changed the title
|
CC @elatt for awareness |
|
Label Needs Review: Custom Models was removed because @yakov-g is part of Custom Models domain. |
| def _reg_arg_time_series(*parsers): | ||
| def require_both_dates(arg): | ||
| # First check if this is score command | ||
| if ArgumentsOptions.SCORE not in sys.argv[1]: |
There was a problem hiding this comment.
Is this commonly how we do this type of argument validation in DRUM, e.g. we manually parse through sys.argv looking for other options? I see you doing this below but this will break down if there is a mixture of things set as env vars and things set and CLI arguments right?
If this is the existing pattern then so-be-it but if this is the first time we have such interdependencies may I suggest instead to let the args all get parsed (and only include validation that is local to the specific flag, such as parsing ISO-8601 format) and then verify_options() is the place to make sure the set of options as a whole are logically consistent?
There was a problem hiding this comment.
Also, I think it breaks down if users do stuff like --myflag=12356 vs --myflag 123456 where I think Python's argparse will accept both styles equally? So yeah, I think it is always best to do pure argument parsing in one phase and then logical validation in a second phase.
| # Add validation for time series args requiring use_datarobot_predict | ||
| time_series_args = ["forecast_point", "predictions_start_date", "predictions_end_date"] | ||
| if any(getattr(options, arg, None) is not None for arg in time_series_args): | ||
| if options.subparser_name != ArgumentsOptions.SCORE: | ||
| print("\nError: Time series arguments can only be used with the score command") | ||
| exit(1) | ||
| if not getattr(options, "use_datarobot_predict", False): | ||
| print( | ||
| "\nError: Time series arguments require --use-datarobot-predict to be enabled" | ||
| ) | ||
| exit(1) |
There was a problem hiding this comment.
Can this get moved into a helper function: CMRunnerArgsRegistry.verify_timeseries_args(...)
This repository is public. Do not put here any private DataRobot or customer's data: code, datasets, model artifacts, .etc.
Rationale
The changes ensure that time series parameters:
Summary
Here's a summary of the main changes to implement time series parameters:
args_parser.py:
drum_score_adapter.py:
predict_mixin.py:
request.argsas param to predict