DATAUP-729 - ts/last_checked for STATUS requests/response, unify output state errors #2891
Conversation
|
This pull request introduces 1 alert and fixes 1 when merging fa4ea85 into d6ff0d5 - view on LGTM.com new alerts:
fixed alerts:
|
n1mus
force-pushed
the
DATAUP-729-job-ts
branch
from
fa4ea85 to
fad63e7
Compare
|
This pull request fixes 1 alert when merging fad63e7 into 20428dd - view on LGTM.com fixed alerts:
|
n1mus
force-pushed
the
DATAUP-729-job-ts
branch
2 times, most recently
from
55ed0fb to
2c275a1
Compare
|
This pull request fixes 1 alert when merging 2c275a1 into 20428dd - view on LGTM.com fixed alerts:
|
n1mus
force-pushed
the
DATAUP-729-job-ts
branch
from
2c275a1 to
81dfcb2
Compare
n1mus
changed the title
|
This pull request fixes 1 alert when merging 81dfcb2 into 20428dd - view on LGTM.com fixed alerts:
|
add ts to STATUS requests add last_checked to STATUS responses unify error modes in response JSON
n1mus
force-pushed
the
DATAUP-729-job-ts
branch
from
81dfcb2 to
542e952
Compare
|
This pull request fixes 1 alert when merging 542e952 into 20428dd - view on LGTM.com fixed alerts:
|
|
SonarCloud Quality Gate failed.
|
|
Looked through some of the issues flagged by the bots but I didn't think most of them were really critical. |
| @@ -100,7 +98,7 @@ def __init__(self, ee2_state, extra_data=None, children=None): | |||
| if ee2_state.get("job_id") is None: | |||
| raise ValueError("Cannot create a job without a job ID!") | |||
|
|
|||
| self._acc_state = ee2_state | |||
| self._update_state(ee2_state) | |||
There was a problem hiding this comment.
Trying to route all Job._acc_state updates through job._update_state to get the job.last_updated updated appropriately
| @@ -325,20 +323,30 @@ def _update_state(self, state: dict) -> None: | |||
| """ | |||
| given a state data structure (as emitted by ee2), update the stored state in the job object | |||
| """ | |||
| if state: | |||
| if not isinstance(state, dict): | |||
There was a problem hiding this comment.
Some refactor--being more specific about what this method should accept anyway
| self._acc_state = state | ||
| else: | ||
| self._acc_state.update(state) | ||
| # Check if there would be no change in updating |
There was a problem hiding this comment.
Check if incoming state would cause any changes
| if {**self._acc_state, **state} == self._acc_state: | ||
| return | ||
|
|
||
| state = copy.deepcopy(state) |
There was a problem hiding this comment.
Update and timestamp job.last_updated
|
|
||
| def state(self, force_refresh=False): | ||
| def state(self, force_refresh=False, exclude=JOB_INIT_EXCLUDED_JOB_STATE_FIELDS): |
There was a problem hiding this comment.
Move the default exclude to a more controllable position in the method header
There was a problem hiding this comment.
can you call this variable something more descriptive, like excluded_fields?
There was a problem hiding this comment.
Sure, that is more consistent
|
|
||
| def _internal_state(self, exclude=None): | ||
| """Wrapper for self._acc_state""" | ||
| state = copy.deepcopy(self._acc_state) | ||
| self._trim_ee2_state(state, exclude) | ||
| return state | ||
|
|
||
| def output_state(self, state=None) -> dict: | ||
| def output_state(self, state=None, no_refresh=False) -> dict: |
There was a problem hiding this comment.
Add no_refresh parameter so that jm._construct_job_state_set can ask for the stagnant output state when EE2 won't cooperate
| errormsg: optional - string, | ||
| error (optional): { | ||
| code: int, | ||
| name: string, | ||
| message: string (should be for the user to read), | ||
| error: string, (likely a stacktrace) | ||
| }, | ||
| error_code: optional - int |
There was a problem hiding this comment.
This output state I didn't find particularly helpful. Just looking at the "optional" error stuff seemed like a violation of helpful documentation. I concluded a long time ago that the error output state would probably be never triggered, and it's kind of mutually exclusive with the regular output state
There was a problem hiding this comment.
The error output state is triggered by an app failing somehow, which can happen pretty easily with some apps.
Here's the job state from an App Sleep job that I configured to fail:
{
"status": "error",
"updated": 1650382783214,
"queued": 1650382769657,
"batch_job": false,
"child_jobs": [],
"retry_ids": [],
"retry_count": 0,
"job_id": "625ed7b128c29d4fd84dcf3a",
"batch_id": null,
"created": 1650382769000,
"running": 1650382777753,
"finished": 1650382783129,
"errormsg": "Job output contains an error",
"error": {
"code": -32000,
"name": "Server error",
"message": "'App woke up from its nap very cranky!'",
"error": "Traceback (most recent call last):\n File \"/kb/module/bin/../lib/NarrativeTest/NarrativeTestServer.py\", line 101, in _call_method\n result = method(ctx, *params)\n File \"/kb/module/lib/NarrativeTest/NarrativeTestImpl.py\", line 345, in app_sleep\n raise RuntimeError('App woke up from its nap very cranky!')\nRuntimeError: App woke up from its nap very cranky!\n"
},
"error_code": 1,
"job_output": {}
}There was a problem hiding this comment.
Oh ... I guess that comes from EE2. I had given up on EE2 documentation because I wasn't sure how up-to-date the spec was, which was why I wished for documentation on that, instead of having to elicit output from the narrative in manifold imaginative ways.
| NO_INPUT_TYPE_ERR = "Please provide one of job_id, job_id_list, or batch_id" | ||
| ONE_INPUT_TYPE_ONLY_ERR = "Please provide at most one of job_id, job_id_list, or batch_id" |
There was a problem hiding this comment.
Thought this was a real distinction but now I'm not so sure with the reconfigured request types ...
There was a problem hiding this comment.
The frontend can't do anything useful with this info so really all these errors could be replaced by NO_INPUT_TYPE_ERR.
| def __init__(self, rq: dict): | ||
| self.raw_request = copy.deepcopy(rq) | ||
| rq = copy.deepcopy(rq) |
There was a problem hiding this comment.
Extra protection since the request dict directs all subsequent execution
| if not self.rq_data: | ||
| raise JobRequestException(INVALID_REQUEST_ERR) | ||
| self.request_type = self.rq_data.get("request_type") | ||
| if self.request_type is None: | ||
| if not self.request_type: |
There was a problem hiding this comment.
I think the falsy values, {} or "", should be just as invalid and should be handled instead of just let through
| except Exception as ex: | ||
| raise JobRequestException(ONE_INPUT_TYPE_ONLY_ERR) from ex |
There was a problem hiding this comment.
Not sure what this was for? removed it
There was a problem hiding this comment.
If you look at the implementation of req.job_id_list, it returns req.job_id as a list if there was no job_id_list parameter.
There was a problem hiding this comment.
req.job_id_list doesn't raise any exceptions except for its own JobRequestException?
There was a problem hiding this comment.
Then JOBS_NOT_PROVIDED_ERR should be "job_id or job_id_list not provided"?
| "source": getattr(e, "source", "jc.start_job_status_loop"), | ||
| "request": getattr(e, "request", "jc.start_job_status_loop"), | ||
| "name": getattr(e, "name", type(e).__name__), | ||
| "message": getattr(e, "message", UNKNOWN_REASON), | ||
| "error": "Unable to get initial jobs list", | ||
| "code": getattr(e, "code", -1), |
There was a problem hiding this comment.
Trying to order these job comm error fields consistently and meaningfully
| """ | ||
| job_id_list = self._get_job_ids(req) | ||
| job_info = self._jm.get_job_info(job_id_list) | ||
| self.send_comm_message(MESSAGE_TYPE["INFO"], job_info) | ||
| return job_info | ||
|
|
||
| def __get_job_states(self, job_id_list) -> dict: |
There was a problem hiding this comment.
If being totally pedantic the leading dunderscore is for name mangling in times of subclassing
There was a problem hiding this comment.
can you give this method a better name than _get_send_job_states, in that case? e.g. _get_job_states_from_job_id_list?
There was a problem hiding this comment.
You would invoke it as _get_send_job_states(job_ids) anyway? And the send part is also important to the story
| """ | ||
| output_states = self._jm.get_job_states(job_id_list) | ||
| output_states = self._jm.get_job_states(job_id_list, ts) |
There was a problem hiding this comment.
Add ts in this downstream STATUS-esque method
| job_id_list = self._get_job_ids(req) | ||
| return self.__get_job_states(job_id_list) | ||
| return self._get_send_job_states(job_id_list, req.ts) |
There was a problem hiding this comment.
Add ts for STATUS requests
| """ | ||
| error_content = {} | ||
| if isinstance(req, JobRequest): | ||
| error_content["request"] = req.rq_data | ||
| error_content["source"] = req.request_type |
There was a problem hiding this comment.
Maintain the order of the error fields consistently throughout code
There was a problem hiding this comment.
reverse alphabetical?
| DOES_NOT_EXIST = "does_not_exist" | ||
|
|
||
|
|
||
| class OutputStateErrMsg(Enum): |
There was a problem hiding this comment.
Decided to turn output state errors into an Enum and not sure if I got carried away adding instance methods and coupling that to jm.add_errors_to_results
There was a problem hiding this comment.
This does seem a little like an excuse to use Enums... 😁
There was a problem hiding this comment.
Yes. I think Enum is fine but I may have gone overboard when I found out each enum could act as an instance ...
| QUERY_EE2_STATES = "A Job.query_ee2_states error occurred for job with ID %s: %s" | ||
| CANCEL = "An EE2.cancel_job error occurred for job with ID %s: %s" | ||
|
|
||
| def gen_err_msg(self, its: List[Union[str, int, Iterable]]): |
There was a problem hiding this comment.
To be used with jm.add_errors_to_results. Returns iterable that will return errors formatted with appropriate args (i.e., error_ids, other error msg information)
There was a problem hiding this comment.
How are you handling error messages that come back from ee2? Just passing them through as-is?
There was a problem hiding this comment.
I believe that was the original behavior which I preserved.
|
|
||
| return gen() | ||
|
|
||
| def replace_result(self) -> bool: |
There was a problem hiding this comment.
Tells whether the particular enum error is meant to replace the dict in the results dict, or if if is meant to be added onto the existing dict
| "return_list": 0, | ||
| } | ||
| ) | ||
| job_states = Job.query_ee2_states(job_ids, init=True) |
There was a problem hiding this comment.
Refactoring the ee2.check_jobs mainly because that's another way to increase the psychological chokepoint-ness of updated job._acc_state ... at least in the sense that it DRYs things up and reduces cognitive load
There was a problem hiding this comment.
Sensible. Ideally all of the ee2 methods would be in one place, but that's probably a refactor too far...
| """ | ||
| job_ids, error_ids = self._check_job_list(job_id_list) | ||
| error_states = dict() | ||
| error_states = {} | ||
| for job_id in job_ids: | ||
| if not self.get_job(job_id).was_terminal(): | ||
| error = self._cancel_job(job_id) | ||
| if error: | ||
| error_states[job_id] = error.message | ||
|
|
||
| job_states = self._construct_job_output_state_set(job_ids) |
There was a problem hiding this comment.
So I realized here you can theoretically can an EE2 error AND a cancel job error for the same job ID. So I made sure that errors were recursively concatenateable
| def add_errors_to_results( | ||
| results: dict, error_ids: List[str], error_enum: OutputStateErrMsg, *extra_its: Tuple | ||
| ) -> dict: |
There was a problem hiding this comment.
Takes the results dict, error_ids, the error enum, and any extra err msg formatting args
| with open(os.path.join(*full_path)) as fh: | ||
| config = json.load(fh) |
There was a problem hiding this comment.
I think this is the pythonic way to make sure the resource is closed
| """ | ||
| generate_test_results.py is used to generate the job message data that the narrative backend produces and that the frontend consumes. It uses data from `ee2_job_test_data_file` and `app_specs_file` and provides expected narrative backend message data, as well as a number of mappings that are used in python tests. | ||
| generate_test_results.py is used to generate the job message data that the narrative backend produces and that the frontend consumes. |
There was a problem hiding this comment.
Introduced line breaks since my IDE is not set to line wrap
There was a problem hiding this comment.
What?! Which IDE are you using? Do you just not have line wrapping turned on?
There was a problem hiding this comment.
VSCode. So I can see when I'm being naughty and going too long?
| @@ -108,7 +115,7 @@ | |||
|
|
|||
|
|
|||
| def make_comm_msg( | |||
| msg_type: str, job_id_like, as_job_request: bool, content: dict = None | |||
| msg_type: str, job_id_like, as_job_request: bool, content: dict = {} | |||
| @@ -134,6 +140,20 @@ def get_app_data(*args): | |||
| return {"info": {"name": APP_NAME}} | |||
|
|
|||
|
|
|||
| def ts_are_close(t0: int, t1: int, tol: float = 1) -> bool: | |||
There was a problem hiding this comment.
For when not mocking out biokbase.narrative.jobs.jobcomm.time.time_ns with 42, check that last_checked ts is approximately right
| @@ -366,6 +386,22 @@ def test_req_no_inputs__fail(self): | |||
|
|
|||
| for msg_type in functions: | |||
| req_dict = make_comm_msg(msg_type, None, False) | |||
| err = JobRequestException(NO_INPUT_TYPE_ERR) | |||
There was a problem hiding this comment.
Tease out cases for NO_INPUT_TYPE_ERR and ONE_INPUT_TYPE_ONLY_ERR
| "source": "ee2", | ||
| "request": "jc.start_job_status_loop", |
There was a problem hiding this comment.
Re-order job comm error fields order
| if response_type == STATUS: | ||
| self._check_pop_last_checked(output_states, last_checked) | ||
| else: | ||
| self.assertNotIn("last_checked", output_states) |
There was a problem hiding this comment.
Easiest way to test the last_checked field compatibly with existing test harnessing is to check it then delete it
| # output_states will be partitioned as | ||
| not_found_ids | ||
| terminal_ids = list(set(job_ids) - set(ACTIVE_JOBS)) | ||
| not_updated_active_ids = list(set(not_updated_ids) & set(active_ids)) | ||
| updated_active_ids = list(set(updated_ids) & set(active_ids)) # (yes, redundant) |
There was a problem hiding this comment.
So the whole point of this test is to check that the mechanism for replacing not updated jobs with error stubs works. The most important part is to distinguish between terminal_ids and not_updated_active_ids, which should be given error stubs, from updated_active_ids which should send back actual output states
| rq_msg1 = {"msg_id": "some_id", "content": {}} | ||
| rq_msg2 = {"msg_id": "some_id", "content": {"data": {}}} | ||
| rq_msg3 = {"msg_id": "some_id", "content": {"data": None}} | ||
| rq_msg4 = {"msg_id": "some_id", "content": {"what": "?"}} | ||
| for msg in [rq_msg1, rq_msg2, rq_msg3, rq_msg4]: | ||
| with self.assertRaisesRegex(JobRequestException, INVALID_REQUEST_ERR): | ||
| JobRequest(msg) | ||
|
|
||
| def test_request_no_req(self): | ||
| rq_msg = {"msg_id": "some_id", "content": {"data": {"request_type": None}}} | ||
| rq_msg2 = {"msg_id": "some_other_id", "content": {"data": {}}} | ||
| for msg in [rq_msg, rq_msg2]: | ||
| rq_msg1 = {"msg_id": "some_id", "content": {"data": {"request_type": None}}} | ||
| rq_msg2 = {"msg_id": "some_id", "content": {"data": {"request_type": ""}}} | ||
| rq_msg3 = {"msg_id": "some_id", "content": {"data": {"what": {}}}} | ||
| for msg in [rq_msg1, rq_msg2, rq_msg3]: |
There was a problem hiding this comment.
Testing for other falsy things. Before, only None was considered invalid
| @@ -640,6 +645,67 @@ def test_get_job_states__empty(self): | |||
| ): | |||
| self.jm.get_job_states([]) | |||
|
|
|||
| @mock.patch(CLIENTS, get_mock_client) | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -740,6 +806,219 @@ def test_get_job_info(self): | |||
| infos, {id: ALL_RESPONSE_DATA[MESSAGE_TYPE["INFO"]][id] for id in ALL_JOBS} | |||
| ) | |||
|
|
|||
| @mock.patch(CLIENTS, get_mock_client) | |||
| def test_add_errors_to_results__concat_errs__integrated(self): | |||
There was a problem hiding this comment.
Test concat errors (i.e., in the case that jm.cancel_jobs needs to concat an error from an output state from jm._construct_output_state_set
| output_state["error"] | ||
| ) | ||
|
|
||
| def test_add_errors_to_results__cannot_add_err(self): |
There was a problem hiding this comment.
Test when trying to add error to existing dict in the results dict in jm.add_errors_to_results but there's no corresponding dict there
| ) | ||
|
|
||
|
|
||
| class OutputStateErrMsgTest(unittest.TestCase): |
There was a problem hiding this comment.
Unit tests for the enum class OutputStateErrMsg
| or when it is retrieved from a cache. If not supplied, must be | ||
| queried with self.state() or self._internal_state() | ||
| :return: dict - with structure generally like (not accounting for error modes): | ||
| { |
There was a problem hiding this comment.
The extra padding here is a waste of space and it forces some of the longer lines to wrap, decreasing legibility.
| } | ||
| } | ||
| } | ||
|
|
||
| This little wrapper fills 2 roles: | ||
| This little wrapper fills some roles: |
There was a problem hiding this comment.
"some" sounds strange -- can you change it to "two"?
There was a problem hiding this comment.
I thought "some" was more a scaffold type documentation, since I was just deleting obsolete documentation and smoothing over the difference
| into multiple JobRequests. Likewise, if the job comm channel request data | ||
| is received with a job_id, a JobRequest may be created with a job_id_list | ||
| containing that job_id | ||
| 2. It provides any job ID information with guardrails |
There was a problem hiding this comment.
Just trying to delete obsolete documentation and smooth it over. The guardrails are in the code
| if not req.has_job_ids(): | ||
| raise JobRequestException(NO_INPUT_TYPE_ERR) |
There was a problem hiding this comment.
Unneeded -- this was already handled by the code you deleted.
There isn't any need for distinction between NO_INPUT_TYPE_ERR and ONE_INPUT_TYPE_ONLY_ERR because the job request is sent back with the error and the frontend can't do anything helpful with the error message other than output it to the console.
There was a problem hiding this comment.
Okay, ONE_INPUT_TYPE_ONLY_ERR sounded to me like there were too many input types
| @@ -114,6 +115,11 @@ def cell_id_list(self): | |||
| return self.rq_data[PARAM["CELL_ID_LIST"]] | |||
| raise JobRequestException(CELLS_NOT_PROVIDED_ERR) | |||
|
|
|||
| @property | |||
| def ts(self): | |||
| """This param is completely optional""" | |||
There was a problem hiding this comment.
Can we have a better description than this?
| failed_ids = [job_id for job_id in jobs_to_lookup if job_id not in fetched_states] | ||
| if failed_ids: | ||
| self.add_errors_to_results( | ||
| output_states, failed_ids, OutputStateErrMsg.QUERY_EE2_STATES, error_message | ||
| ) |
There was a problem hiding this comment.
this is now more complex than it was, so I'm not sure it is an improvement...
There was a problem hiding this comment.
I think the Enum with all its glory may not have been great as it did add complexity. But I think here it's streamlined because you have a method that takes the output data structure, the id keys, the error message, and also concatenates errors instead of overwriting them. So, cancel_jobs calls this method, so the output data structure can accrue both a ee2.check_jobs error and a ee2.cancel_jobs error.
Also, if you look at python library packages, the code can be really stupid complex. So I may be erring on the side of complexity.
| self.add_errors_to_results(output_states, not_found_ids, OutputStateErrMsg.NOT_FOUND) | ||
| self.add_errors_to_results(output_states, not_updated_ids, OutputStateErrMsg.NOT_UPDATED, ts) |
There was a problem hiding this comment.
Unfortunately this is not what I asked for in the Jira ticket -- if a job hasn't been updated, the BE is supposed just to omit it from the results it returns.
Once the docs PR is merged and this PR is rebased on develop, I'll set this up locally and do some experimenting.
There was a problem hiding this comment.
So the description was a little more open-ended and inconsistent with the acceptance critera. So I interpreted the entire JIRA ticket as being open-ended.
There was a problem hiding this comment.
Sorry, I think I got completely tripped up by this part of the ticket
don’t include the jobState dict in the job output, just return job_id and last_checked for each job in the request?
Description of PR purpose/changes
Jira Ticket / Issue
Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-X
DATAUP-69 Adds a PR template)Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)