Bacpop 201 stream microreact #83

absternator · 2024-11-08T10:28:52Z

This PR allows streaming of microreact jobs per cluster as they come in... The main changes are as below:

add new microreactClusters status which is a map of clusters and their status.. This is used to track whether a clusters microreact is ready or not.

Testing:

Best to test by running app with samples and seeing that microreact become available as they are finished...

This commit refactors the code in the MicroReactColumn.vue component and the projectStatus.ts utility file. It updates the condition for checking the microreact cluster status in the hasMicroreactClusterFailed function. Instead of using the microreactClusterStatuses object, it now uses the status object and the microreactClusters property. This change ensures that the correct status is checked when determining if the microreact cluster has failed.

…tils - Updated the microreact cluster status check in ProjectPostRun.vue to include the isAnyMicroreactFinished function from projectStatus.ts. - Added the isAnyMicroreactFinished function to projectStatus.ts to check if any microreact clusters have finished.

codecov · 2024-11-11T15:42:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.83%. Comparing base (d56ebbc) to head (d879a6f).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   98.78%   98.83%   +0.05%     
==========================================
  Files          34       34              
  Lines        1971     2060      +89     
  Branches      247      270      +23     
==========================================
+ Hits         1947     2036      +89     
  Misses         24       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

… cluster status management

…lity for improved cluster status management

…mproved readability

…accuracy

…all databases

…accuracy

EmmaLRussell

Looks great!

I can run this with the newer sample strep samples you sent over a while ago, but failing with the old fasta examples. Is that expected?

EmmaLRussell · 2024-11-14T15:24:08Z

app/client-v2/e2e/projectPostRun.spec.ts

@@ -13,7 +13,7 @@ test("can run project and view results", async ({ page }) => {
  await page.getByLabel("Run Analysis").click();

  await expect(page.getByText("Running Analysis...33%")).toBeVisible();
-  await expect(page.getByText("Running Analysis...67%")).toBeVisible();
+  await expect(page.getByText("Running Analysis...75%")).toBeVisible();


This seems a bit fishy, why does it go from 33% to 75%. This suggests it goes from thinking it's done 1 out of 3 jobs to thinking it's done 3 out of 4... Does it think there are 4 jobs rather than 3 once the initial cluster allocation has happened, because the microreactClusters key is added to the results?

(Yes, when I run it manually, and it's done the cluster assignment and network but no microreact results yet, it goes from 33% to 50%.)

I think the progress bar should include the progress of the individual microreact jobs but ignoring the top level task as separate entity e.g. if it's done assign clusters (33%) plus 1 out of 3 microreactClusters (0.33 * 33%) then it could show 44%...

EmmaLRussell · 2024-11-14T15:57:39Z

app/client-v2/src/stores/projectStore.ts

+    numOfStatus(): number {
      return (
-        analysisStatusValues.length > 0 && analysisStatusValues.every((value) => COMPLETE_STATUS_TYPES.includes(value))
+        Object.keys(this.separatedStatuses.fullStatuses).length +
+        Object.keys(this.separatedStatuses.microreactClusters).length
      );
    },


Would this be a little cleaner?

return statusValues.length;

EmmaLRussell · 2024-11-14T15:59:39Z

app/client-v2/src/stores/projectStore.ts

+    analysisProgressPercentage(): number {
+      return this.statusValues.length > 0
+        ? Math.round(
+            (this.statusValues.filter((value) => COMPLETE_STATUS_TYPES.includes(value)).length / this.numOfStatus) * 100
+          )
+        : 0;
    },


Yeah, I think the reason the progress values look a little weird is because microreact is being counted twice, once with its value in "fullStatuses" but also via all the individual cluster values. I think it's confusing to include the former when it's not really a separate task from completing microreact for each cluster.

EmmaLRussell · 2024-11-14T16:07:01Z

app/client-v2/src/utils/projectStatus.ts

+  status: AnalysisStatus | undefined,
+  cluster: string | undefined
+): boolean => {
+  // occurs when another sample has passed but this one fails


Do you mean when it's failed to assign a cluster, hence it being falsy?

Suggested change

// occurs when another sample has passed but this one fails

// occurs when this sample could not be assigned to a cluster but at least one other sample was assigned

Should we show the microreact job as "failed" in that case? It's more like we couldn't run it because a prerequisite failed. How about "cancelled"?

yup cancelled does make sense here.. ive made ticket as this will need a bit more changes https://mrc-ide.myjetbrains.com/youtrack/issue/bacpop-204/When-assign-fails-change-network-microreact-to-cancelled-not-failed

EmmaLRussell · 2024-11-14T16:11:30Z

app/client-v2/src/utils/projectStatus.ts

+  return status?.microreact || "waiting";
+};
+
+export const isAnyMicroreactQueued = (microreactClusterStatuses: Record<string, StatusTypes> | undefined): boolean => {


Slightly misleading method name, since these could all be completed jobs, not currently queued.

Suggested change

export const isAnyMicroreactQueued = (microreactClusterStatuses: Record<string, StatusTypes> | undefined): boolean => {

export const haveAnyMicroreactBeenQueued = (microreactClusterStatuses: Record<string, StatusTypes> | undefined): boolean => {

Something like that?

EmmaLRussell · 2024-11-15T11:18:01Z

OK, this now seems to be running fine for me with the original fasta files. Maybe I just made a project with the wrong species before!

EmmaLRussell

Nice!

absternator added 8 commits November 6, 2024 14:36

feat: get initial working version

a2ae8e7

feat: get working version %

be23fc0

fail network + microreact if cluster asign fails

284db42

fix: issue regarding % mismatch

9c85205

fix: naming + status errors

a488094

feat: add microreact utils

eff76cf

absternator mentioned this pull request Nov 8, 2024

Bacpop-201 Stream microreact per cluster bacpop/beebop_py#47

Merged

test: tests for code

8ce516f

absternator requested a review from EmmaLRussell November 11, 2024 11:20

absternator added 4 commits November 11, 2024 11:33

styke: formats

144a14d

feat: add --refs option to download_databases script

bc38751

fix: update MOCK_PROJECT to include microreactClusters in network status

d89ae52

feat: add --refs option to run_docker script for download_databases

e73f7ec

absternator added 7 commits November 12, 2024 12:08

fix: update MicroReactColumn and project status handling for improved…

45cc324

… cluster status management

style: format MicroReactColumn.vue for improved readability

5d1aedf

fix: update project status handling and add isAnyMicroreactQueued uti…

30ae4ab

…lity for improved cluster status management

style: simplify Tag component rendering in MicroReactColumn.vue for i…

1035ad1

…mproved readability

fix: update expected analysis progress in projectPostRun.spec.ts for …

c429de6

…accuracy

fix: update expected analysis progress in projectPostRun.spec.ts for …

b8e1c2d

…accuracy

fix: update run_dependencies script to remove --refs for downloading …

18bdb50

…all databases

absternator mentioned this pull request Nov 13, 2024

Bacpop-186 v9 db support bacpop/beebop_py#48

Merged

fix: update expected analysis progress in projectPostRun.spec.ts for …

71b29f8

…accuracy

EmmaLRussell reviewed Nov 14, 2024

View reviewed changes

absternator added 3 commits November 15, 2024 15:05

feat: update progress bar logic

95f4fac

refactor: rename status-related methods and variables for clarity

eb6d4cc

test: remove exclusive test markers from projectStore.spec.ts

de97d31

absternator requested a review from EmmaLRussell November 15, 2024 15:11

EmmaLRussell approved these changes Nov 19, 2024

View reviewed changes

absternator added 2 commits November 19, 2024 10:21

fix: update API_BRANCH to main in common script

8878b5c

{commit_message}

d879a6f

absternator merged commit a50f9bb into main Nov 19, 2024
11 checks passed

absternator deleted the bacpop-201-stream-microreact branch November 19, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bacpop 201 stream microreact #83

Bacpop 201 stream microreact #83

absternator commented Nov 8, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading

EmmaLRussell left a comment

EmmaLRussell Nov 14, 2024

EmmaLRussell Nov 14, 2024

EmmaLRussell Nov 14, 2024

EmmaLRussell Nov 14, 2024

absternator Nov 15, 2024

EmmaLRussell Nov 14, 2024

EmmaLRussell commented Nov 15, 2024

EmmaLRussell left a comment

	// occurs when another sample has passed but this one fails
	// occurs when this sample could not be assigned to a cluster but at least one other sample was assigned

	export const isAnyMicroreactQueued = (microreactClusterStatuses: Record<string, StatusTypes> \| undefined): boolean => {
	export const haveAnyMicroreactBeenQueued = (microreactClusterStatuses: Record<string, StatusTypes> \| undefined): boolean => {

Bacpop 201 stream microreact #83

Bacpop 201 stream microreact #83

Conversation

absternator commented Nov 8, 2024

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

EmmaLRussell left a comment

Choose a reason for hiding this comment

EmmaLRussell Nov 14, 2024

Choose a reason for hiding this comment

EmmaLRussell Nov 14, 2024

Choose a reason for hiding this comment

EmmaLRussell Nov 14, 2024

Choose a reason for hiding this comment

EmmaLRussell Nov 14, 2024

Choose a reason for hiding this comment

absternator Nov 15, 2024

Choose a reason for hiding this comment

EmmaLRussell Nov 14, 2024

Choose a reason for hiding this comment

EmmaLRussell commented Nov 15, 2024

EmmaLRussell left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 •

edited

Loading