NgenCalibrationRequest and associated workflow logic #306
Conversation
robertbartel
added
bug
There was a problem hiding this comment.
I left a few minor comments that arent required to merge this. Thanks for splitting this up @robertbartel!
| _DEFAULT_CPU_COUNT = 1 | ||
| """ The default number of CPUs to assume are being requested for the job, when not explicitly provided. """ | ||
|
|
||
| def __init__(self, config_data_id: str, cpu_count: Optional[int] = None, |
There was a problem hiding this comment.
Just marking this, this work will need to be refactored post merge to line up with the pydantic refactor of comm (#253).
python/lib/communication/dmod/communication/maas_request/ngen/abstract_nextgen_request.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Just a TODO, will need to be refactored inline with the pydantic work post merge (related #253).
|
|
||
| try: | ||
| return cls(begin=datetime.strptime(as_string[:dt_str_length], dt_format), | ||
| end=datetime.strptime(as_string[(-1 * dt_str_length):], dt_format)) |
There was a problem hiding this comment.
It might feel a little weird, but you can avoid the multiplication call here and just write -dt_str_length.
| service_port | ||
| service_ssl_dir | ||
| """ | ||
| super(PartitionRequestHandler, self).__init__(*args, **kwargs) |
There was a problem hiding this comment.
This is mainly an aside, ive noticed that often super calls are made this way. To my understanding in python >= 3 you dont have to include the class name and self in super calls. You can just call super().__init__(*args, **kwargs). Is there a reason to prefer the python 2.7 super call style?
There was a problem hiding this comment.
The only reason I've done this is because it's been faster ...
For some reason, this is how my JetBrains IDE auto-completes things for me. There's probably some setting somewhere to fix this (@christophertubbs?), but it isn't terribly obvious. It knows I'm using Python 3.8, my specified virtual environment, etc., and yet still does this.
I've started doing a little clean-up of these and need to finally take the time to sort out my IDE.
There was a problem hiding this comment.
I figured it was not done purposefully, you have a long track record of consistency and sticking to idiomatic python. That's odd that JetBrains is auto-completing it that way, weird.
|
@aaraney, FYI, when you have a moment, this will need approval once more. I tweaked a few things you noted and fixed the failing tests. |
|
@robertbartel 👍🏻, ill have a look right after the standup. |
Enhancing factory_init_from_deserialized_json function by adding and using new _additional_deserialized_args function (empty for this type) to allow separate mechanism for subclasses to use to prepare their own specific init params and still use base deserialization logic of NGENRequest.factory_init_from_deserialized_json().
Adding new NgenCalibrationRequest and NgenCalibrationResponse classes.
Fixing a bug in the ModelExecRequestHandler class's service_client property method, where the order of the positional arguments was incorrect.
Adding a function to ModelExecRequestHandler for any preprocessing that is needed before sending things to the scheduler (with it being an empty implementation for now), to allow subclasses a way to extend behavior.
Updating requestservice service class to account for changes in the names of init params for utilized request handler classes.
this also includes some commented out code that *might* need to be added in the future to support data_requirement dissemination
Fixing imports, class usage, and code style spacing for several things related to evaluation requests within main request-service handler class.
…en calibration jobs
Move up some attributes from ModelExecRequest to DmodJobRequest.
Creating two new abstract types to represent a NextGen-specific DmodJobRequest (the second also being an ExternalRequest) that isn't (necessarily) a ModelExecRequest, which, an argument could be made, is more appropriate for something like a calibration request.
Having NGENRequest extend the newly created ExternalNextGenRequest type, the adjusting NGENRequest's implementation as needed to utilize and fulfill its new superclass.
Update NgenCalibrationRequest to extend ExternalNextGenRequest, rather than NGENRequest, since NGENRequest is a ModelExecRequest but NgenCalibrationRequest has an event type of CALIBRATION_REQUEST.
Fixing NgenCalRequestClient for the removal of the (duplicate) evaluation_time_range in NgenCalibrationRequest init, as the underlying property is represented already via "time_range".
Ensure certain important message types - the new AbstractNextGenRequest type and the more utilized DmodJobRequest type - are set up properly in (sub)package __init__.py file(s) so that they can be imported directly from the package when needed.
Refactoring Launcher class in dmod.scheduler to improve code for how Docker CMD args are generated for Nextgen-based jobs, and account for Nextgen calibration job CMD args in addition to those for model exec.
Fixing bug in deserialize_for_init() function, where tuples were accidentally added to a built dict of kwargs because of an accidental trailing comma.
Reworking the __hash__ implementations in several classes within the meta_data.py module, thereby fixing bugs in a few of them.
Fixing scheduler request message tests after adjusting NGENRequest to use "session_secret" rather than "session-secret" for the attribute's key in serialized form.
Adding parser function to parse instance from string of two datetime substrings.
Parameterizing init value for cached session file, maintaining previous hard-coded value as default, for ngen and ngen cal request clients.
Fixing tests for service manager to use correct new serialized syntax of session secret property (underscore rather than dash in JSON key).
Fixing unit tests for Job to use correct new serialized syntax of session secret property (underscore rather than dash in JSON key).
Fixing bug with AbstractNgenRequest and this being non-None when job is not parallel.
robertbartel
force-pushed
the
i/old240/ngen-cal-requests
branch
from
0565c2e to
ad97a24
Compare
|
@aaraney, ok, had to do some rebasing and reworking of a few things. Assuming this last check passes (I'm fairly confident), this will need one last review. |
|
|
||
| # TODO: account for differences between regular ngen execution and calibration job | ||
|
|
||
| # $10 is the name of the calibration config dataset (which will imply a directory location) |
There was a problem hiding this comment.
Just so this is captured somewhere, at this point, this method is expecting that a calibration configuration file is provided as part of a DataFormat.NGEN_REALIZATION_CONFIG, right?
__init__syntax in certain request typesNote that this PR partially replaces #240, in that it is composed of changes extracted from that PR for easier reviewing and merging.