[RW-8031][risk=no] Dynamically update extraction jobs via SWR #6601

calbach · 2022-04-22T17:00:56Z

Automatically refresh pending genomic extraction jobs, and deduplicate state requests using SWR.

This is a first integration of SWR into the Workbench, specifically it is being used here to manage shared state across different areas of the application (side bar icon, genomic extraction table, genomic extract launch modal / menu). SWR is used to implement refresh polling, as well as handle stale data.

Overall, it's a bit chattier by default than I might like (re-requests data when you return to the page, for example), but it was pretty easy to set up. This is behavior that can be tuned if we want to.

Note: on abort, ideally I would use an optimistic data update. Unfortunately it's not possible here because Cromwell does not function as expected w.r.t. abortJob (the job continues int he running state, and then almost immediately transitions to ABORTED). The optimistic update is immediately reverted on refresh.

General patterns

Define a custom SWR hook

const useGenomicExtractionJobs = (ns, name) =>
  useSWR(
    // The convention for the key is to match the API path, but it's just an arbitrary unique key for deduplication within the app.
    // I omit "v1" here, but we could also omit "${name}", since it doesn't add uniqueness beyond the namespace.
    `/api/workspaces/${ns}/${name}/genomicExtractionJobs`,
    () =>
      dataSetApi()
        .getGenomicExtractionJobs(ns, name)
        .then(({ jobs }) => jobs));

Use the hook

const {data, error} = useGenomicExtractionJobs(...);
if (!data) {
  return <Loading />;
}
...

Signal a mutation to invalidate the cache

const {data, mutate} = useGenomicExtractionJobs(...);

...
  onclick={async () => {
    await datasetsApi.abortJob(...);
    mutate();
  }}

Alternative considered: React Query

https://react-query.tanstack.com/overview . I started to create another prototype PR for this, but it seems to be almost the same library, but in a less elegant package (lots of overlapping boolean return values, lack of simple mutation binding, required provider binding). Possibly something we can revisit when we have a more nuanced understanding of SWR usage / tradeoffs, but at first blush SWR is definitely my preference.

dmohs

I get the impression that people are unfamiliar with SWR as a technique. I judge it would be useful to cover it with a cross-team meeting, discussing the concept, use cases, and hazards. What do you think?

dmohs · 2022-04-28T15:13:45Z

ui/src/app/components/genomics-extraction-menu.tsx

-                  workspace.namespace,
-                  workspaceJobs
-                );
+                onMutate();


It isn't clear to me why SWR is the right technique here. The naive approach is usually to block user input with a spinner, make the request, update the state, then remove the spinner. Why not do that?

I agree we should probably do that, but it's an existing issue which seems orthogonal to the usage of SWR here.

The reason to integrate with SWR is that this state is referenced in multiple places on the page (specifically, the side-nav icon and the job table). By making a change, and then invoking mutate - we are indicating that the data store is stale. Ideally here, we would optimistically update as well, but that doesn't work per my Cromwell aborting status comment (see my other draft PR which shows that).

Makes sense.

dmohs · 2022-04-28T15:14:11Z

ui/package.json

    "rxjs": "^5.5.6",
    "stackdriver-errors-js": "^0.4.0",
    "stacktrace-js": "^2.0.0",
+    "swr": "^1.3.0",


FYI ~1300 lines, no dependencies.

dmohs · 2022-04-28T15:19:39Z

ui/src/app/pages/data/data-set/genomic-extraction-modal.tsx

-                  job
-                )
-              );
+              mutate(fp.concat(jobs, job));


Good usage here. Since you know the job has been added, there is no reason to make the user to wait for the "list jobs" call.

dmohs · 2022-04-28T15:27:52Z

ui/src/app/utils/stores.tsx

+        return 0;
+      },
+    }
+  );


Roughly equivalent implementation without using the library, for comparison:

export const useGenomicExtractionJobsSansLib = ({workspaceNamespace, workspaceId}) => { const fetchJobs = dataSetApi() .getGenomicExtractionJobs(workspaceNamespace, workspaceId).then(({ jobs }) => jobs) const [jobs, setJobs] = useState() setJobs(await fetchJobs()) const maybeReFetch = () => { const nts = [TerraJobStatus.RUNNING, TerraJobStatus.ABORTING] if (jobs?.some(({ status }) => nts.includes(status))) { setJobs(await fetchJobs()) } } const interval = setInterval(maybeReFetch, 10 * 1000) useEffect(() => () => clearInterval(interval)) return { jobs, mutate: f => { setJobs(f(jobs)) } } }

Note: de-duping isn't implemented, so would require more code if needed.

Thanks for writing this up.

Differences, for a fair comparison (perhaps omitted intentionally for brevity):

(as you mentioned) Deduping I'd say is the primary value add I see from SWR. We've used stores to address this in the past, but it's been fairly messy given their static nature and limited custom API.

your mutate function behaves differently from SWR. Specifically, calling mutate in SWR invalidates the store. Calling with a parameter also applies an optimistic update, which is what you've effectively done here.

no surfacing of errors (this is used elsewhere in the PR)

other library features which we're not using yet (see return value of useSWR()), but might use in the future

I realize your comment is not necessarily stating an opinion, just providing the alternate for comparison, but I'll also note an implicit cost of that approach is that we get either bespoke semantics for each hook, or best case, consistent semantics which are bespoke to our codebase.

Thanks for taking the time to call out additional differences. It's actually helping me learn more about the tool.

Regarding deduping: The library is likely creating a global store referenced by the given string key. I consider this a drawback versus importing a variable. My hope (having limited time to dive in to the documentation) is that the store is weakly referenced, allowing the objects to be garbage collected once all clients have gone away. If we built this, I'd expect something on top of atoms, but I doubt it already exists.

Taking a step back, I prefer small, composable tools. I really liked that you highlighted the narrow scope of this library versus other options and I'm grateful to you for promoting that position. It is likely I'd prefer a version of this PR that achieved the same result by combining several small tools (useState + useEffect + setInterval + atom + some new "reference counted atom" thing), but I'm also open to seeing where this takes us.

calbach

Yes - I agree, there's not much exposure to it and we should continue to cover it and discuss to try and provide that (e.g. at eng weekly). Bumping up a level to the higher level concepts is a good suggestion. That said, in my experience there's only so much folks will grok without actually trying to use it.

calbach · 2022-04-28T18:47:07Z

ui/src/app/components/genomics-extraction-menu.tsx

-                  workspace.namespace,
-                  workspaceJobs
-                );
+                onMutate();


I agree we should probably do that, but it's an existing issue which seems orthogonal to the usage of SWR here.

The reason to integrate with SWR is that this state is referenced in multiple places on the page (specifically, the side-nav icon and the job table). By making a change, and then invoking mutate - we are indicating that the data store is stale. Ideally here, we would optimistically update as well, but that doesn't work per my Cromwell aborting status comment (see my other draft PR which shows that).

calbach · 2022-04-28T19:00:54Z

ui/src/app/utils/stores.tsx

+        return 0;
+      },
+    }
+  );


Thanks for writing this up.

Differences, for a fair comparison (perhaps omitted intentionally for brevity):

(as you mentioned) Deduping I'd say is the primary value add I see from SWR. We've used stores to address this in the past, but it's been fairly messy given their static nature and limited custom API.

your mutate function behaves differently from SWR. Specifically, calling mutate in SWR invalidates the store. Calling with a parameter also applies an optimistic update, which is what you've effectively done here.

no surfacing of errors (this is used elsewhere in the PR)

other library features which we're not using yet (see return value of useSWR()), but might use in the future

I realize your comment is not necessarily stating an opinion, just providing the alternate for comparison, but I'll also note an implicit cost of that approach is that we get either bespoke semantics for each hook, or best case, consistent semantics which are bespoke to our codebase.

calbach · 2022-04-29T01:12:52Z

I'm happy with this approach and there were no major objections or opinions raised, so promoting this for a proper review. I also had a chance to check out React Query for comparison (see PR description).

dmohs

Thanks for the discussion. I feel much more knowledgable about the value this is adding than I had going in.

dmohs · 2022-05-02T16:12:26Z

ui/src/app/utils/stores.tsx

+        return 0;
+      },
+    }
+  );


Thanks for taking the time to call out additional differences. It's actually helping me learn more about the tool.

Regarding deduping: The library is likely creating a global store referenced by the given string key. I consider this a drawback versus importing a variable. My hope (having limited time to dive in to the documentation) is that the store is weakly referenced, allowing the objects to be garbage collected once all clients have gone away. If we built this, I'd expect something on top of atoms, but I doubt it already exists.

Taking a step back, I prefer small, composable tools. I really liked that you highlighted the narrow scope of this library versus other options and I'm grateful to you for promoting that position. It is likely I'd prefer a version of this PR that achieved the same result by combining several small tools (useState + useEffect + setInterval + atom + some new "reference counted atom" thing), but I'm also open to seeing where this takes us.

dmohs · 2022-05-02T16:13:29Z

ui/src/app/components/genomics-extraction-menu.tsx

-                  workspace.namespace,
-                  workspaceJobs
-                );
+                onMutate();


Makes sense.

calbach · 2022-05-02T23:59:23Z

Thanks for the review.

dmohs reviewed Apr 28, 2022

View reviewed changes

calbach commented Apr 28, 2022

View reviewed changes

calbach added 4 commits April 28, 2022 18:10

SWR for genomic extractions, initial approach

ac8ff8c

lint

988aaa7

cleaner mutation pattern

e697e7b

test fixes

578442a

calbach force-pushed the ch/extract-swr-real branch from 921c2ee to 578442a Compare April 29, 2022 01:10

calbach marked this pull request as ready for review April 29, 2022 01:11

calbach requested a review from dmohs April 29, 2022 01:11

dmohs approved these changes May 2, 2022

View reviewed changes

calbach merged commit a7cec48 into main May 2, 2022

calbach deleted the ch/extract-swr-real branch May 2, 2022 23:59

[RW-8031][risk=no] Dynamically update extraction jobs via SWR #6601

[RW-8031][risk=no] Dynamically update extraction jobs via SWR #6601

Uh oh!

Conversation

calbach commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General patterns

Define a custom SWR hook

Use the hook

Signal a mutation to invalidate the cache

Alternative considered: React Query

Uh oh!

dmohs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calbach commented Apr 29, 2022

Uh oh!

dmohs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calbach commented May 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

calbach commented Apr 22, 2022 •

edited

Loading