DATAUP-765: add user and wsid to job output #3386

ialarmedalien · 2023-11-09T22:24:42Z

Description of PR purpose/changes

This PR adds the user and wsid to the job objects returned by the narrative backend.

I added typing to quite a few of the methods so the diff looks bigger than it really is.

There's still some dodgy stuff going on with the job methods, but whatever. That can go in the next PR!

Jira Ticket / Issue

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

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

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
[] (JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
(Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook
[] Any dependent changes have been merged and published in downstream modules

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)

ialarmedalien · 2023-11-09T22:25:18Z

src/biokbase/narrative/jobs/job.py

 # run_id (str): unique run ID for the job
+# status (str): EE2 job status


no idea why this pretty important attribute was omitted!

ialarmedalien · 2023-11-09T22:26:51Z

src/biokbase/narrative/jobs/job.py

+        extra_data: dict[str, Any] | None = None,
+        children: list["Job"] | None = None,
+    ):
+        state = cls.query_ee2_states([job_id], init=True)


Use the plural form, query_ee2_states, so that the singular form can be removed.

ialarmedalien · 2023-11-09T22:27:13Z

src/biokbase/narrative/jobs/job.py

-    @staticmethod
-    def _trim_ee2_state(state: dict, exclude_fields: list) -> None:


moved down the file a bit

ialarmedalien · 2023-11-09T22:27:37Z

src/biokbase/narrative/jobs/job.py

+    @staticmethod
+    def _trim_ee2_state(


moved (but not changed)

ialarmedalien · 2023-11-09T22:29:33Z

src/biokbase/narrative/jobs/job.py

                # TODO
                # Only batch container jobs have a child_jobs field
                # and need the state refresh.
                # But KBParallel/KB Batch App jobs may not have the
                # batch_job field
-                self.refresh_state(force_refresh=True).get(
+                self.refresh_state(force_refresh=force_refresh).get(


this is deeply annoying to me. Why have a refresh_state method if you're not going to refresh the state?!

ialarmedalien · 2023-11-09T22:31:02Z

src/biokbase/narrative/jobs/job.py

        """
        Returns the parameters used to start the job. Job tries to use its params field, but
        if that's None, then it makes a call to EE2.

        If no exception is raised, this only returns the list of parameters, NOT the whole
        object fetched from ee2.check_job
        """
-        if self.params is not None:
-            return self.params
+        if self.params is None:


slight rearrangement of the logic

ialarmedalien · 2023-11-09T22:31:42Z

src/biokbase/narrative/jobs/job.py

+            state = self.query_ee2_states([self.job_id], init=False)
+            self.update_state(state[self.job_id])


use plural form, query_ee2_states

ialarmedalien · 2023-11-09T22:32:07Z

src/biokbase/narrative/jobs/job.py

-        if self._acc_state is None:
-            self._acc_state = {}
-


self._acc_state is set during __init__() so will never be None

ialarmedalien · 2023-11-09T22:33:35Z

src/biokbase/narrative/jobs/job.py

        """Wrapper for self._acc_state"""
        state = copy.deepcopy(self._acc_state)
        self._trim_ee2_state(state, exclude_fields)
        return state

-    def output_state(self, state=None, no_refresh=False) -> dict:


change output_state so that it no longer merges in a fetched state; now the state merge is done by running update_state beforehand

ialarmedalien · 2023-11-09T22:34:40Z

src/biokbase/narrative/jobs/job.py

-            self.update_state(state)
-            state = self.cached_state()
-
-        if state is None:


No way that state can be None with the new logic as self.cached_state() fetches self._acc_state, which always has something populated.

ialarmedalien · 2023-11-09T22:35:04Z

src/biokbase/narrative/jobs/job.py

+        # calling child_jobs may force an undesired data refresh even if
+        # refresh is set to False


this is a bug in the current implementation

ialarmedalien · 2023-11-09T22:35:48Z

src/biokbase/narrative/jobs/jobcomm.py

@@ -67,7 +67,7 @@ class JobRequest:

    INPUT_TYPES = [PARAM["JOB_ID"], PARAM["JOB_ID_LIST"], PARAM["BATCH_ID"]]

-    def __init__(self, rq: dict):
+    def __init__(self: "JobRequest", rq: dict):


all edits to this file are adding typing

codecov · 2023-11-09T22:36:39Z

Codecov Report

Merging #3386 (4faae24) into develop (04c80b2) will increase coverage by 0.01%.
Report is 1 commits behind head on develop.
The diff coverage is 92.35%.

❗ Current head 4faae24 differs from pull request most recent head b982e48. Consider uploading reports for the commit b982e48 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3386      +/-   ##
===========================================
+ Coverage    29.34%   29.35%   +0.01%     
===========================================
  Files          497      497              
  Lines        50580    50569      -11     
===========================================
+ Hits         14841    14843       +2     
+ Misses       35739    35726      -13

Files	Coverage Δ
src/biokbase/narrative/jobs/appmanager.py	`91.96% <100.00%> (+0.02%)`	⬆️
src/biokbase/narrative/jobs/jobcomm.py	`98.96% <100.00%> (ø)`
src/biokbase/narrative/jobs/job.py	`92.34% <98.30%> (-0.96%)`	⬇️
src/biokbase/narrative/jobs/jobmanager.py	`95.39% <95.23%> (+0.01%)`	⬆️
...s/widgets/function_output/kbaseGenericSetViewer.js	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

ialarmedalien · 2023-11-09T22:36:52Z

src/biokbase/narrative/jobs/jobmanager.py

+                job.update_state(states[job_id])
+                output_states[job_id] = job.output_state(refresh=False)


update the job state and then retrieve the version suitable for output

ialarmedalien · 2023-11-10T16:46:23Z

src/biokbase/narrative/jobs/appmanager.py

@@ -6,7 +6,8 @@
 import random
 import re
 import traceback
-from typing import Callable, Optional
+from collections.abc import Callable


all changes in this file are adding typing

Why the switch from using Optional?

Ruff likes type | None rather than Optional[type]

ialarmedalien · 2023-11-10T16:48:13Z

src/biokbase/narrative/jobs/job.py

-                    del state[field]
-
-    @staticmethod
-    def query_ee2_state(


delete this method, replace all uses with query_ee2_states

ialarmedalien · 2023-11-10T16:49:01Z

src/biokbase/narrative/jobs/job.py

@@ -193,52 +188,59 @@ def __getattr__(self, name):
            "cell_id": lambda: self._acc_state.get("job_input", {})
            .get("narrative_cell_info", {})
            .get("cell_id", JOB_ATTR_DEFAULTS["cell_id"]),
-            "child_jobs": lambda: copy.deepcopy(


ialarmedalien · 2023-11-10T16:52:33Z

src/biokbase/narrative/jobs/job.py

            "job_id": lambda: self._acc_state.get("job_id"),
            "params": lambda: copy.deepcopy(
                self._acc_state.get("job_input", {}).get(
                    "params", JOB_ATTR_DEFAULTS["params"]
                )
            ),
-            "retry_ids": lambda: copy.deepcopy(


ialarmedalien · 2023-11-10T16:52:54Z

src/biokbase/narrative/jobs/job.py

+                    "retry_ids", JOB_ATTR_DEFAULTS["retry_ids"]
+                )
+            ),
+            "status": lambda force_refresh=True: copy.deepcopy(


ialarmedalien · 2023-11-10T16:53:21Z

src/biokbase/narrative/jobs/job.py

        return "batch" if self.batch_job else self.app_spec()["info"]["name"]

-    def was_terminal(self):
+    def in_terminal_state(self: "Job") -> bool:


renamed method

ialarmedalien · 2023-11-10T16:57:46Z

src/biokbase/narrative/jobs/job.py

+        if "child_jobs" not in state:
+            state["child_jobs"] = JOB_ATTR_DEFAULTS["child_jobs"]


access via JOB_ATTR_DEFAULTS or otherwise there'll be an unnecessary call to ee2

ialarmedalien · 2023-11-10T16:58:45Z

src/biokbase/narrative/jobs/jobcomm.py

-from typing import List, Union
-
-from ipykernel.comm import Comm
+from typing import Union


Nearly all typing additions; there are a few minor edits which I will comment on.

ialarmedalien · 2023-11-10T16:59:32Z

src/biokbase/narrative/jobs/jobcomm.py

@@ -21,6 +20,8 @@

 LOOKUP_TIMER_INTERVAL = 5

+INPUT_TYPES = [PARAM["JOB_ID"], PARAM["JOB_ID_LIST"], PARAM["BATCH_ID"]]


move out of the JobRequest class

ialarmedalien · 2023-11-10T17:00:49Z

src/biokbase/narrative/jobs/jobmanager.py

+            if job.in_terminal_state():
+                # job is already finished, will not change
+                output_states[job_id] = job.output_state()
+            elif job_id in states:
+                job.update_state(states[job_id])


edit this to update the job state and then call output_state()

ialarmedalien · 2023-11-10T17:01:14Z

src/biokbase/narrative/jobs/jobmanager.py

+                    job.update_state(fetched_states[job_id])
+                    output_states[job_id] = job.output_state()


again, update the state and then get the output state

ialarmedalien · 2023-11-10T17:02:22Z

src/biokbase/narrative/jobs/jobmanager.py

+        jobs_to_lookup = [
+            job_id
+            for job_id in self._running_jobs
+            if self._running_jobs[job_id]["refresh"] or ignore_refresh_flag
+        ]


use a list comprehension

ialarmedalien · 2023-11-10T17:03:00Z

src/biokbase/narrative/jobs/jobmanager.py

@@ -518,27 +535,27 @@ def get_job_logs(
                    logs = logs[first_line:]
            else:
                (max_lines, logs) = job.log(first_line=first_line, num_lines=num_lines)
-
+        except Exception as e:


minor reorganisation of logic

ialarmedalien · 2023-11-10T17:03:37Z

src/biokbase/narrative/tests/data/response_data.json

+                },
+                "wsid": 10538


add the WSIDs into these results

ialarmedalien · 2023-11-10T17:04:09Z

src/biokbase/narrative/tests/generate_test_results.py

-        lines.append({"is_error": 0, "line": f"This is line {str(i+1)}"})
-    return lines
+def log_gen(n_lines: int) -> list[dict[str, int | str]]:
+    return [{"is_error": 0, "line": f"This is line {i+1}"} for i in range(n_lines)]


use list comprehension

ialarmedalien · 2023-11-10T17:05:23Z

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

-        job_state = self.job_state_data.get(
-            job_id, {"job_id": job_id, "status": "unmocked"}
-        )
+        job_state = self.job_state_data.get(job_id, {"job_id": job_id})


a default is supplied for "status", so we don't need the "unmocked" status value any more

ialarmedalien · 2023-11-10T17:08:24Z

src/biokbase/narrative/tests/test_job.py

            job.refresh_state()

-    def test_refresh_state__returns_none(self):


this is no longer relevant as there's no way to provoke this error.

ialarmedalien · 2023-11-10T17:09:20Z

src/biokbase/narrative/tests/test_jobmanager.py

@@ -248,6 +242,12 @@ def mock_check_jobs(params):
            # expect there to be an error message added
            expected[job_id]["error"] = exc_msg

+        def mock_check_jobs(params):


moved this code down a bit

briehl

I'm not sure if the GenericSetViewer code changes were intended in this PR, but there's a couple minor things with them.

briehl · 2023-11-15T16:51:57Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseGenericSetViewer.js

            this.genericClient = new GenericClient(Config.url('service_wizard'), {
                token: this.authToken(),
            });

            const bare_type = this.options._obj_info.bare_type[0];
-            const method = this.methodMap[bare_type];
+            const method = this.methodMap[bare_type]


briehl · 2023-11-15T16:54:10Z

src/biokbase/narrative/jobs/appmanager.py

@@ -6,7 +6,8 @@
 import random
 import re
 import traceback
-from typing import Callable, Optional
+from collections.abc import Callable


Why the switch from using Optional?

briehl · 2023-11-15T17:08:53Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseGenericSetViewer.js

+                console.warn('no _obj_info: input was')
+                console.warn(options)
+                return
+                // if (options.obj_ref && options.wsName) {


This can probably be removed, as it's basically there below.

This was an accidental commit from something else I was troubleshooting. Not sure why I didn't see it when I was reviewing the PR! I'll remove it.

Removing some comments and an extraneous method; general tidying

sonarcloud · 2023-11-16T14:39:48Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

briehl

👍

briehl · 2023-11-16T16:09:40Z

Looks like the upload to codecov failed. AGAIN.

ialarmedalien requested a review from briehl November 9, 2023 22:24

ialarmedalien self-assigned this Nov 9, 2023

ialarmedalien commented Nov 9, 2023

View reviewed changes

ialarmedalien force-pushed the DATAUP-765_add_wsid_to_job branch from 9583b66 to 66788fe Compare November 10, 2023 16:23

ialarmedalien commented Nov 10, 2023

View reviewed changes

ialarmedalien force-pushed the DATAUP-765_add_wsid_to_job branch from 8e6aef5 to 4faae24 Compare November 10, 2023 17:12

briehl requested changes Nov 15, 2023

View reviewed changes

add user and wsid to job output

b982e48

Removing some comments and an extraneous method; general tidying

ialarmedalien force-pushed the DATAUP-765_add_wsid_to_job branch from 4faae24 to b982e48 Compare November 16, 2023 14:39

briehl approved these changes Nov 16, 2023

View reviewed changes

ialarmedalien merged commit 16b4c20 into develop Nov 16, 2023
6 of 9 checks passed

ialarmedalien deleted the DATAUP-765_add_wsid_to_job branch November 16, 2023 17:35

		# run_id (str): unique run ID for the job
		# status (str): EE2 job status

		@staticmethod
		def _trim_ee2_state(state: dict, exclude_fields: list) -> None:

		state = self.query_ee2_states([self.job_id], init=False)
		self.update_state(state[self.job_id])

		# calling child_jobs may force an undesired data refresh even if
		# refresh is set to False

		job.update_state(states[job_id])
		output_states[job_id] = job.output_state(refresh=False)

		if "child_jobs" not in state:
		state["child_jobs"] = JOB_ATTR_DEFAULTS["child_jobs"]

		@@ -21,6 +20,8 @@

		LOOKUP_TIMER_INTERVAL = 5

		INPUT_TYPES = [PARAM["JOB_ID"], PARAM["JOB_ID_LIST"], PARAM["BATCH_ID"]]

		job.update_state(fetched_states[job_id])
		output_states[job_id] = job.output_state()

		job.refresh_state()

		def test_refresh_state__returns_none(self):

DATAUP-765: add user and wsid to job output #3386

DATAUP-765: add user and wsid to job output #3386

Conversation

ialarmedalien commented Nov 9, 2023

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

Updating Version and Release Notes (if applicable)

Choose a reason for hiding this comment

ialarmedalien Nov 9, 2023 • edited Loading

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

ialarmedalien Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

ialarmedalien Nov 9, 2023 • edited Loading

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 Nov 9, 2023 • 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

ialarmedalien Nov 16, 2023 • edited Loading

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

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

briehl left a 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

sonarcloud bot commented Nov 16, 2023

briehl left a comment

Choose a reason for hiding this comment

briehl commented Nov 16, 2023

ialarmedalien Nov 9, 2023 •

edited

Loading

ialarmedalien Nov 9, 2023 •

edited

Loading

ialarmedalien Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

ialarmedalien Nov 16, 2023 •

edited

Loading