DATAUP-729: job timestamp easy pickings #2950

ialarmedalien · 2022-05-04T23:13:09Z

Description of PR purpose/changes

This is a set of "easy pickings" from @n1mus (or Summit, as autocorrect calls her)'s DATAUP-729-job-ts branch. The original PR was large and complex so this updates and merges it into develop (see the first commit), and then picks out a small-ish set of changes for the first PR. It's expected that there will be (at least) two more PRs to cover the rest of the changes.

I ran isort, the import sorter, on the altered files.

Jira Ticket / Issue

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

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 seeing that nothing has changed.

Dev Checklist:

My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
(Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook

ialarmedalien · 2022-05-04T23:14:32Z

src/biokbase/narrative/jobs/appmanager.py

@@ -1,30 +1,32 @@
 """
 A module for managing apps, specs, requirements, and for starting jobs.
 """
+import datetime


ialarmedalien · 2022-05-04T23:17:06Z

src/biokbase/narrative/jobs/job.py

@@ -79,8 +82,8 @@
 STATE_ATTRS = list(set(JOB_ATTRS) - set(JOB_INPUT_ATTRS) - set(NARR_CELL_INFO_ATTRS))


-class Job(object):
-    _job_logs = list()
+class Job:


no need for (object) in python 3

ialarmedalien · 2022-05-04T23:18:50Z

src/biokbase/narrative/jobs/job.py

-class Job(object):
-    _job_logs = list()
+class Job:
+    _job_logs = []


from the flake8-comprehensions plugin (installed locally)

ialarmedalien · 2022-05-04T23:19:43Z

src/biokbase/narrative/jobs/job.py

@@ -172,25 +175,25 @@ def __getattr__(self, name):
        """
        Map expected job attributes to paths in stored ee2 state
        """
-        attr = dict(
-            app_id=lambda: self._acc_state.get("job_input", {}).get(
+        attr = {


flake8-comprehensions again

ialarmedalien · 2022-05-04T23:20:34Z

src/biokbase/narrative/jobs/job.py

+        # Check if there would be no change in updating
+        # i.e., if state <= self._acc_state
+        if self._acc_state is not None:
+            if {**self._acc_state, **state} == self._acc_state:
+                return


prep for upcoming timestamps work

lgtm-com · 2022-05-04T23:20:50Z

This pull request introduces 2 alerts when merging 3dc225d into 213fd59 - view on LGTM.com

new alerts:

2 for Property in old-style class

ialarmedalien · 2022-05-04T23:21:53Z

src/biokbase/narrative/jobs/jobcomm.py

@@ -112,6 +114,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"""


@n1mus is going to write a better description for this param in a forthcoming PR 😁

ialarmedalien · 2022-05-04T23:23:50Z

src/biokbase/narrative/jobs/jobcomm.py

+                MESSAGE_TYPE["CANCEL"]: self.cancel_jobs,
+                MESSAGE_TYPE["CELL_JOB_STATUS"]: self.get_job_states_by_cell_id,
+                MESSAGE_TYPE["INFO"]: self.get_job_info,
+                MESSAGE_TYPE["LOGS"]: self.get_job_logs,
+                MESSAGE_TYPE["RETRY"]: self.retry_jobs,
                MESSAGE_TYPE["START_UPDATE"]: self._modify_job_updates,
-                MESSAGE_TYPE["STATUS"]: self._get_job_states,
-                MESSAGE_TYPE["STATUS_ALL"]: self._get_all_job_states,
+                MESSAGE_TYPE["STATUS"]: self.get_job_states,
+                MESSAGE_TYPE["STATUS_ALL"]: self.get_all_job_states,


renamed these methods for clarity and to make a better distinction between top level and internal methods

Nice! This would always throw me for a loop now that I think of it

ialarmedalien · 2022-05-04T23:26:14Z

src/biokbase/narrative/jobs/jobcomm.py

@@ -323,13 +331,12 @@ def _get_job_info(self, req: JobRequest) -> dict:
        self.send_comm_message(MESSAGE_TYPE["INFO"], job_info)
        return job_info

-    def __get_job_states(self, job_id_list) -> dict:
+    def _get_job_states(self, job_id_list: list, ts: int = None) -> dict:


note new ts parameter

codecov · 2022-05-04T23:27:35Z

Codecov Report

Merging #2950 (3dc225d) into develop (66e5c6e) will increase coverage by 0.05%.
The diff coverage is 98.71%.

❗ Current head 3dc225d differs from pull request most recent head 52e379b. Consider uploading reports for the commit 52e379b to get more accurate results

@@             Coverage Diff             @@
##           develop    #2950      +/-   ##
===========================================
+ Coverage    73.19%   73.25%   +0.05%     
===========================================
  Files           36       36              
  Lines         3895     3903       +8     
===========================================
+ Hits          2851     2859       +8     
  Misses        1044     1044

Impacted Files	Coverage Δ
src/biokbase/narrative/jobs/jobmanager.py	`95.35% <90.90%> (-0.02%)`	⬇️
src/biokbase/narrative/jobs/appmanager.py	`91.86% <100.00%> (-0.31%)`	⬇️
src/biokbase/narrative/jobs/job.py	`90.51% <100.00%> (+0.64%)`	⬆️
src/biokbase/narrative/jobs/jobcomm.py	`98.95% <100.00%> (+0.02%)`	⬆️
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 bcbad86...52e379b. Read the comment docs.

ialarmedalien · 2022-05-04T23:28:00Z

src/biokbase/narrative/jobs/jobmanager.py

-                "return_list": 0,
-            }
-        )
+        job_states = Job.query_ee2_states(job_ids, init=True)


@n1mus has consolidated most of the ee2 queries (apart from the workspace job fetch) to use the query_ee2_state(s) functions.

ialarmedalien · 2022-05-04T23:33:53Z

src/biokbase/narrative/tests/test_job.py

@@ -186,15 +183,19 @@ def check_jobs_equal(self, jobl, jobr):
        for attr in JOB_ATTRS:
            self.assertEqual(getattr(jobl, attr), getattr(jobr, attr))

-    def check_job_attrs_custom(self, job, exp_attr={}):
+    def check_job_attrs_custom(self, job, exp_attr=None):


don't use mutable args as defaults

Ha I've learned something

ialarmedalien · 2022-05-04T23:34:50Z

src/biokbase/narrative/tests/test_jobcomm.py

-import unittest
-from unittest import mock
-import os
+import copy


isort got busy on this file!

n1mus · 2022-05-05T02:55:03Z

src/biokbase/narrative/jobs/appmanager.py

@@ -1002,7 +1004,7 @@ def _generate_input(self, generator):
        if "prefix" in generator:
            ret = str(generator["prefix"]) + ret
        if "suffix" in generator:
-            ret = ret + str(generator["suffix"])
+            return ret + str(generator["suffix"])


? No biggie but I thought this was strange

I've got a flake8 plugin installed locally that warns if there's somewhere where a variable can be returned immediately. This is one such case...

n1mus · 2022-05-05T02:58:50Z

src/biokbase/narrative/jobs/jobcomm.py

@@ -112,6 +114,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"""


n1mus · 2022-05-05T02:59:34Z

src/biokbase/narrative/jobs/jobcomm.py

+                MESSAGE_TYPE["CANCEL"]: self.cancel_jobs,
+                MESSAGE_TYPE["CELL_JOB_STATUS"]: self.get_job_states_by_cell_id,
+                MESSAGE_TYPE["INFO"]: self.get_job_info,
+                MESSAGE_TYPE["LOGS"]: self.get_job_logs,
+                MESSAGE_TYPE["RETRY"]: self.retry_jobs,
                MESSAGE_TYPE["START_UPDATE"]: self._modify_job_updates,
-                MESSAGE_TYPE["STATUS"]: self._get_job_states,
-                MESSAGE_TYPE["STATUS_ALL"]: self._get_all_job_states,
+                MESSAGE_TYPE["STATUS"]: self.get_job_states,
+                MESSAGE_TYPE["STATUS_ALL"]: self.get_all_job_states,


Nice! This would always throw me for a loop now that I think of it

n1mus · 2022-05-05T03:03:45Z

src/biokbase/narrative/jobs/jobmanager.py

@@ -770,7 +771,7 @@ def list_jobs(self):
        """
        try:
            all_states = self.get_all_job_states(ignore_refresh_flag=True)
-            state_list = [s["jobState"] for s in all_states.values()]
+            state_list = [copy.deepcopy(s["jobState"]) for s in all_states.values()]


Shouldn't need this anymore since we fixed the double- yet leaky-caching, but no biggie

would you mind fixing it in the next PR?

n1mus · 2022-05-05T03:06:47Z

src/biokbase/narrative/tests/test_job.py

@@ -186,15 +183,19 @@ def check_jobs_equal(self, jobl, jobr):
        for attr in JOB_ATTRS:
            self.assertEqual(getattr(jobl, attr), getattr(jobr, attr))

-    def check_job_attrs_custom(self, job, exp_attr={}):
+    def check_job_attrs_custom(self, job, exp_attr=None):


Ha I've learned something

… branch

lgtm-com · 2022-05-05T14:00:26Z

This pull request introduces 2 alerts when merging 52e379b into bcbad86 - view on LGTM.com

new alerts:

2 for Property in old-style class

sonarqubecloud · 2022-05-05T14:09:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

ialarmedalien requested review from n1mus and briehl May 4, 2022 23:13

ialarmedalien self-assigned this May 4, 2022

ialarmedalien commented May 4, 2022

View reviewed changes

n1mus approved these changes May 5, 2022

View reviewed changes

n1mus and others added 2 commits May 5, 2022 06:53

Merge DATAUP-729-job-ts into develop

07fd057

Cherry-picking out a set of simple changes from the DATAUP-729-job-ts…

52e379b

… branch

ialarmedalien force-pushed the DATAUP-729_job_ts_easy_pickings branch from 3dc225d to 52e379b Compare May 5, 2022 13:54

briehl approved these changes May 6, 2022

View reviewed changes

ialarmedalien merged commit df43735 into develop May 6, 2022

ialarmedalien deleted the DATAUP-729_job_ts_easy_pickings branch May 6, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAUP-729: job timestamp easy pickings #2950

DATAUP-729: job timestamp easy pickings #2950

ialarmedalien commented May 4, 2022 •

edited

Loading

ialarmedalien May 4, 2022

n1mus May 5, 2022

ialarmedalien May 4, 2022

ialarmedalien May 4, 2022

ialarmedalien May 4, 2022

ialarmedalien May 4, 2022

lgtm-com bot commented May 4, 2022

ialarmedalien May 4, 2022

n1mus May 5, 2022

ialarmedalien May 4, 2022

n1mus May 5, 2022

ialarmedalien May 4, 2022

codecov bot commented May 4, 2022 •

edited

Loading

ialarmedalien May 4, 2022

ialarmedalien May 4, 2022

n1mus May 5, 2022

ialarmedalien May 4, 2022

n1mus May 5, 2022

ialarmedalien May 5, 2022

n1mus May 5, 2022

n1mus May 5, 2022

n1mus May 5, 2022

ialarmedalien May 5, 2022

n1mus May 5, 2022

lgtm-com bot commented May 5, 2022

sonarqubecloud bot commented May 5, 2022

DATAUP-729: job timestamp easy pickings #2950

DATAUP-729: job timestamp easy pickings #2950

Conversation

ialarmedalien commented May 4, 2022 • edited Loading

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 5, 2022

sonarqubecloud bot commented May 5, 2022

ialarmedalien commented May 4, 2022 •

edited

Loading

codecov bot commented May 4, 2022 •

edited

Loading