DATAUP-729 job ts implementation #2960

n1mus · 2022-05-09T20:31:34Z

Description of PR purpose/changes

Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-X

Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

Updating Version and Release Notes (if applicable)

Version has been bumped for each release
Release notes have been updated for each release (and during the merge of feature branches)

src/biokbase/narrative/jobs/job.py

src/biokbase/narrative/tests/test_jobcomm.py

ialarmedalien · 2022-05-11T15:00:43Z

This is looking good!

Can you add in functionality such that if there are no updated jobs in response to a request, the backend returns an error?

Thanks!

lgtm-com · 2022-05-12T11:12:28Z

This pull request introduces 1 alert when merging cd0194c into c57f696 - view on LGTM.com

new alerts:

1 for Unused import

codecov · 2022-05-12T11:19:46Z

Codecov Report

Merging #2960 (6bbd36d) into develop (86c5af6) will increase coverage by 0.19%.
The diff coverage is 93.75%.

@@             Coverage Diff             @@
##           develop    #2960      +/-   ##
===========================================
+ Coverage    73.25%   73.45%   +0.19%     
===========================================
  Files           36       36              
  Lines         3903     3906       +3     
===========================================
+ Hits          2859     2869      +10     
+ Misses        1044     1037       -7

Impacted Files	Coverage Δ
src/biokbase/narrative/jobs/job.py	`93.11% <86.95%> (+2.60%)`	⬆️
src/biokbase/narrative/jobs/jobcomm.py	`98.96% <100.00%> (+<0.01%)`	⬆️
src/biokbase/narrative/jobs/jobmanager.py	`95.56% <100.00%> (+0.20%)`	⬆️
src/biokbase/narrative/jobs/util.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 014f72a...6bbd36d. Read the comment docs.

src/biokbase/narrative/jobs/job.py

src/biokbase/narrative/tests/test_jobmanager.py

src/biokbase/narrative/jobs/jobmanager.py

src/biokbase/narrative/jobs/util.py

src/biokbase/narrative/jobs/jobcomm.py

ialarmedalien · 2022-05-18T14:16:54Z

src/biokbase/narrative/jobs/jobcomm.py

+        if msg_type == MESSAGE_TYPE["STATUS"]:
+            now = time_ns()
+            for output_state in content.values():
+                output_state["last_checked"] = now


why not add this timestamp when the job manager is putting together the list of jobs, instead of adding an extra iteration through the job state data here?

Wasn't sure since the CANCEL_JOBS request also responds with a STATUS message.

I decided not to filter the STATUS response for CANCEL_JOBS though because I figured in theory they should all get updated, whether successfully or just coming back with an error

Because everything is asynchronous, the FE doesn't have any way of knowing what triggered a job status message -- whether it was a cancel request, a status request, or the BE job loop. That's why I say it's better to put the timestamp on in the job manager, so that all job state objects that the FE receives have a timestamp on them.

I think one allure of putting everything into JobComm is less tests surgery ... But putting it deep into the stack, at the origin of the STATUS response ds, seems less googly-eyed

Okay I tried putting all the filtering/last_checked logic at the source _construct_job_state_set but the tests were complaining so I'm abandoning that effort for the sake of time. Is the current placement of the filtering/last_checked good enough?

src/biokbase/narrative/jobs/jobcomm.py

src/biokbase/narrative/jobs/util.py

sonarqubecloud · 2022-05-19T21:06:08Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

src/biokbase/narrative/jobs/job.py

ialarmedalien · 2022-05-20T17:01:04Z

src/biokbase/narrative/tests/job_test_constants.py

@@ -32,6 +32,13 @@ def generate_error(job_id, err_type):
    return error_strings[err_type]


+def trim_ee2_state(ee2_state, exclude_fields):


don't we have this code somewhere else?

Yep, Job._trim_ee2_state. I just got tired of using that in tests when usually we use independent testing functions

ialarmedalien · 2022-05-20T17:09:06Z

src/biokbase/narrative/tests/narrative_mock/mockclients.py

+        ee2_states = self.job_state_data
+        if params.get("exclude_fields"):
+            for ee2_state in ee2_states.values():
+                trim_ee2_state(ee2_state, params["exclude_fields"])
+        if params.get("return_list"):
+            ee2_states = list(ee2_states.values())


Do any of those params ever change? There's only one place where check_workspace_jobs gets called, and the params are always the same, so...

No, but I thought it might be a good idea to implement the "exclude_fields" param since here I'm paying closer attention to when state updates are triggered

has adding the exclude_fields param changed the output of the function?

Well .... now that you mention it ... probably not

Today's "good idea to implement" is tomorrow's "why on earth did someone write this?". YAGNI. 😄

ialarmedalien · 2022-05-20T17:12:48Z

src/biokbase/narrative/tests/test_job.py

+            job.update_state({})
+            self.assertEqual(last_updated, job.last_updated)
+
+            # job has init ee2 state


This test looks suspiciously spaghetti code-like. Does it need to be done as this long series of transitions or can it be split into separate tests?

I thought it followed a very similar pattern throughout and so could flow in a single function. The punchline is last_updated defined at the top never changes throughout these tests. Is there a benefit to making tests methods small?

It's much easier to read, understand, and update/edit a couple of stanzas of code than it is a long series of stanzas. Unless there is a specific need to test a sequence of modifications (e.g. there's something going on elsewhere that changes state as a result of these mods), it's best to make tests as simple as possible to assist future codebase editors and maintainers.

Ah. But what if it's two long stanzas of highly repetitive code? With a common punchline that is accentuated by more repetition?

If it's highly repetitive, it suggests that the repetition could be abstracted out into a function... or that it could be replaced by individual tests that validate the atomic operations involved.

ialarmedalien · 2022-05-20T17:14:18Z

src/biokbase/narrative/tests/test_job.py

@@ -757,7 +821,7 @@ def test_in_cells__batch__same_cell(self):
        batch_job, child_jobs = batch_fam[0], batch_fam[1:]

        for job in child_jobs:
-            job.cell_id = "hello"
+            job._acc_state["job_input"]["narrative_cell_info"]["cell_id"] = "hello"


Was this the only place you could find where an attribute was changed (other than via the update_state method)?

I checked every field in job.__setattr__ that was from the "job_input". I didn't check anything in the outer level of the ee2 state.

But update_state is the only place _acc_state is mutated

You left a TODO comment about whether the attribute setter was ever used in job.py -- seems as though you've answered it here, so can delete the comment.

n1mus commented May 9, 2022

View reviewed changes