From bc9abe852376dd5841559b53b16faf9bfdc6cd7b Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 16:43:43 -0600 Subject: [PATCH 1/9] parallelize mask path decoding --- app/packages/looker/src/worker/index.ts | 48 ++++++++++++++++--------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/app/packages/looker/src/worker/index.ts b/app/packages/looker/src/worker/index.ts index d10d0625fd..c58075a74b 100644 --- a/app/packages/looker/src/worker/index.ts +++ b/app/packages/looker/src/worker/index.ts @@ -107,11 +107,12 @@ const imputeOverlayFromPath = async ( colorscale: Colorscale, buffers: ArrayBuffer[], sources: { [path: string]: string }, - cls: string + cls: string, + maskPathDecodingPromises?: Promise[] ) => { // handle all list types here if (cls === DETECTIONS) { - const promises = []; + const promises: Promise[] = []; for (const detection of label.detections) { promises.push( imputeOverlayFromPath( @@ -126,10 +127,7 @@ const imputeOverlayFromPath = async ( ) ); } - // if some paths fail to load, it's okay, we can still proceed - // hence we use `allSettled` instead of `all` - await Promise.allSettled(promises); - return; + maskPathDecodingPromises.push(...promises); } // overlay path is in `map_path` property for heatmap, or else, it's in `mask_path` property (for segmentation or detection) @@ -190,8 +188,11 @@ const processLabels = async ( schema: Schema ): Promise => { const buffers: ArrayBuffer[] = []; - const promises = []; + const painterPromises = []; + + const maskPathDecodingPromises = []; + // mask deserialization / mask_path decoding loop for (const field in sample) { let labels = sample[field]; if (!Array.isArray(labels)) { @@ -205,8 +206,8 @@ const processLabels = async ( } if (DENSE_LABELS.has(cls)) { - try { - await imputeOverlayFromPath( + maskPathDecodingPromises.push( + imputeOverlayFromPath( `${prefix || ""}${field}`, label, coloring, @@ -214,11 +215,10 @@ const processLabels = async ( colorscale, buffers, sources, - cls - ); - } catch (e) { - console.error("Couldn't decode overlay image from disk: ", e); - } + cls, + maskPathDecodingPromises + ) + ); } if (cls in DeserializerFactory) { @@ -249,9 +249,25 @@ const processLabels = async ( mapId(label); } } + } + } + + await Promise.allSettled(maskPathDecodingPromises); + + // overlay painting loop + for (const field in sample) { + let labels = sample[field]; + if (!Array.isArray(labels)) { + labels = [labels]; + } + const cls = getCls(`${prefix ? prefix : ""}${field}`, schema); + for (const label of labels) { + if (!label) { + continue; + } if (painterFactory[cls]) { - promises.push( + painterPromises.push( painterFactory[cls]( prefix ? prefix + field : field, label, @@ -266,7 +282,7 @@ const processLabels = async ( } } - return Promise.all(promises).then(() => buffers); + return Promise.all(painterPromises).then(() => buffers); }; /** GLOBALS */ From 209932bbaf67922fb4dfc9c15c3b7bbc3998aba1 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:13:25 -0600 Subject: [PATCH 2/9] set default retry delay to 500ms --- app/packages/utilities/src/fetch.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/packages/utilities/src/fetch.ts b/app/packages/utilities/src/fetch.ts index 71c5366336..260dfac82b 100644 --- a/app/packages/utilities/src/fetch.ts +++ b/app/packages/utilities/src/fetch.ts @@ -18,7 +18,7 @@ export interface FetchFunction { body?: A, result?: "json" | "blob" | "text" | "arrayBuffer" | "json-stream", retries?: number, - retryCodes?: number[] | "arrayBuffer" + retryCodes?: number[] ): Promise; } @@ -110,7 +110,7 @@ export const setFetchFunction = ( const fetchCall = retries ? fetchRetry(fetch, { retries, - retryDelay: 0, + retryDelay: 500, retryOn: (attempt, error, response) => { if ( (error !== null || retryCodes.includes(response.status)) && From c91d265e5b1874dd492633bb2677e3941a382290 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:14:03 -0600 Subject: [PATCH 3/9] use custom fetch with linear backoff for mask path decoding --- .../looker/src/worker/decorated-fetch.test.ts | 80 +++++++++++++++++++ .../looker/src/worker/decorated-fetch.ts | 25 ++++++ app/packages/looker/src/worker/index.ts | 18 +++-- 3 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 app/packages/looker/src/worker/decorated-fetch.test.ts create mode 100644 app/packages/looker/src/worker/decorated-fetch.ts diff --git a/app/packages/looker/src/worker/decorated-fetch.test.ts b/app/packages/looker/src/worker/decorated-fetch.test.ts new file mode 100644 index 0000000000..32c44aa1fe --- /dev/null +++ b/app/packages/looker/src/worker/decorated-fetch.test.ts @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { fetchWithLinearBackoff } from "./decorated-fetch"; + +describe("fetchWithLinearBackoff", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.useRealTimers(); + }); + + it("should return response when fetch succeeds on first try", async () => { + const mockResponse = new Response("Success", { status: 200 }); + global.fetch = vi.fn().mockResolvedValue(mockResponse); + + const response = await fetchWithLinearBackoff("http://fiftyone.ai"); + + expect(response).toBe(mockResponse); + expect(global.fetch).toHaveBeenCalledTimes(1); + expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai"); + }); + + it("should retry when fetch fails and eventually succeed", async () => { + const mockResponse = new Response("Success", { status: 200 }); + global.fetch = vi + .fn() + .mockRejectedValueOnce(new Error("Network Error")) + .mockResolvedValue(mockResponse); + + const response = await fetchWithLinearBackoff("http://fiftyone.ai"); + + expect(response).toBe(mockResponse); + expect(global.fetch).toHaveBeenCalledTimes(2); + }); + + it("should throw an error after max retries when fetch fails every time", async () => { + global.fetch = vi.fn().mockRejectedValue(new Error("Network Error")); + + await expect( + fetchWithLinearBackoff("http://fiftyone.ai", 3) + ).rejects.toThrow("Max retries for fetch reached"); + + expect(global.fetch).toHaveBeenCalledTimes(3); + }); + + it("should throw an error when response is not ok", async () => { + const mockResponse = new Response("Not Found", { status: 404 }); + global.fetch = vi.fn().mockResolvedValue(mockResponse); + + await expect(fetchWithLinearBackoff("http://fiftyone.ai")).rejects.toThrow( + "HTTP error: 404" + ); + + expect(global.fetch).toHaveBeenCalledTimes(5); + }); + + it("should apply linear backoff between retries", async () => { + const mockResponse = new Response("Success", { status: 200 }); + global.fetch = vi + .fn() + .mockRejectedValueOnce(new Error("Network Error")) + .mockRejectedValueOnce(new Error("Network Error")) + .mockResolvedValue(mockResponse); + + vi.useFakeTimers(); + + const fetchPromise = fetchWithLinearBackoff("http://fiftyone.ai", 5, 100); + + // advance timers to simulate delays + // after first delay + await vi.advanceTimersByTimeAsync(100); + // after scond delay + await vi.advanceTimersByTimeAsync(200); + + const response = await fetchPromise; + + expect(response).toBe(mockResponse); + expect(global.fetch).toHaveBeenCalledTimes(3); + + vi.useRealTimers(); + }); +}); diff --git a/app/packages/looker/src/worker/decorated-fetch.ts b/app/packages/looker/src/worker/decorated-fetch.ts new file mode 100644 index 0000000000..19d42cb33a --- /dev/null +++ b/app/packages/looker/src/worker/decorated-fetch.ts @@ -0,0 +1,25 @@ +const DEFAULT_MAX_RETRIES = 10; +const DEFAULT_BASE_DELAY = 200; + +export const fetchWithLinearBackoff = async ( + url: string, + retries = DEFAULT_MAX_RETRIES, + delay = DEFAULT_BASE_DELAY +) => { + for (let i = 0; i < retries; i++) { + try { + const response = await fetch(url); + if (!response.ok) throw new Error(`HTTP error: ${response.status}`); + return response; + } catch (e) { + if (i < retries - 1) { + await new Promise((resolve) => setTimeout(resolve, delay * (i + 1))); + } else { + throw new Error( + "Max retries for fetch reached (linear backoff), error: " + e + ); + } + } + } + return new Promise(() => {}); +}; diff --git a/app/packages/looker/src/worker/index.ts b/app/packages/looker/src/worker/index.ts index c58075a74b..a327d682cd 100644 --- a/app/packages/looker/src/worker/index.ts +++ b/app/packages/looker/src/worker/index.ts @@ -30,6 +30,7 @@ import { Sample, } from "../state"; import { decodeWithCanvas } from "./canvas-decoder"; +import { fetchWithLinearBackoff } from "./decorated-fetch"; import { DeserializerFactory } from "./deserializer"; import { PainterFactory } from "./painter"; import { mapId } from "./shared"; @@ -155,14 +156,17 @@ const imputeOverlayFromPath = async ( baseUrl = overlayImageUrl.split("?")[0]; } - const overlayImageBuffer: Blob = await getFetchFunction()( - "GET", - overlayImageUrl, - null, - "blob" - ); + let overlayImageBlob: Blob; + try { + const overlayImageFetchResponse = await fetchWithLinearBackoff(baseUrl); + overlayImageBlob = await overlayImageFetchResponse.blob(); + } catch (e) { + console.error(e); + // skip decoding if fetch fails altogether + return; + } - const overlayMask = await decodeWithCanvas(overlayImageBuffer); + const overlayMask = await decodeWithCanvas(overlayImageBlob); const [overlayHeight, overlayWidth] = overlayMask.shape; // set the `mask` property for this label From e991eab4337655f232486cf6174263b53c997f3a Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:16:52 -0600 Subject: [PATCH 4/9] retry image looker image.src with linear backoff --- app/packages/looker/src/elements/image.ts | 44 ++++++++++++++++++++--- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/app/packages/looker/src/elements/image.ts b/app/packages/looker/src/elements/image.ts index 2df92eb5fd..ad089c98cf 100644 --- a/app/packages/looker/src/elements/image.ts +++ b/app/packages/looker/src/elements/image.ts @@ -6,13 +6,24 @@ import type { ImageState } from "../state"; import type { Events } from "./base"; import { BaseElement } from "./base"; +const MAX_IMAGE_LOAD_RETRIES = 10; + export class ImageElement extends BaseElement { private src = ""; - private imageSource: HTMLImageElement; + protected imageSource: HTMLImageElement; + + private retryCount = 0; + private timeoutId: number | null = null; getEvents(): Events { return { load: ({ update }) => { + if (this.timeoutId !== null) { + window.clearTimeout(this.timeoutId); + this.timeoutId = null; + } + this.retryCount = 0; + this.imageSource = this.element; update({ @@ -21,7 +32,29 @@ export class ImageElement extends BaseElement { }); }, error: ({ update }) => { - update({ error: true, dimensions: [512, 512], loaded: true }); + // sometimes image loading fails because of insufficient resources + // we'll want to try again in those cases + if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) { + // schedule a retry after a delay + if (this.timeoutId !== null) { + window.clearTimeout(this.timeoutId); + } + console.log( + ">>> retrying image load url", + this.src, + "attempt", + this.retryCount + ); + this.timeoutId = window.setTimeout(() => { + this.retryCount += 1; + const retrySrc = `${this.src}`; + this.element.setAttribute("src", retrySrc); + // linear backoff + }, 1000 * this.retryCount); + } else { + // max retries reached; finally set error to true + update({ error: true, dimensions: [512, 512], loaded: true }); + } }, }; } @@ -36,10 +69,13 @@ export class ImageElement extends BaseElement { renderSelf({ config: { src } }: Readonly) { if (this.src !== src) { this.src = src; - + this.retryCount = 0; + if (this.timeoutId !== null) { + window.clearTimeout(this.timeoutId); + this.timeoutId = null; + } this.element.setAttribute("src", src); } - return null; } } From 11fbcb0b4fb8b9135322e316da99a2bbfd336a6e Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:17:04 -0600 Subject: [PATCH 5/9] remove debug logs --- app/packages/looker/src/elements/image.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/packages/looker/src/elements/image.ts b/app/packages/looker/src/elements/image.ts index ad089c98cf..eabf771d18 100644 --- a/app/packages/looker/src/elements/image.ts +++ b/app/packages/looker/src/elements/image.ts @@ -39,12 +39,6 @@ export class ImageElement extends BaseElement { if (this.timeoutId !== null) { window.clearTimeout(this.timeoutId); } - console.log( - ">>> retrying image load url", - this.src, - "attempt", - this.retryCount - ); this.timeoutId = window.setTimeout(() => { this.retryCount += 1; const retrySrc = `${this.src}`; From e73409b07d830a137440cb278a43a1032f8192b8 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:57:04 -0600 Subject: [PATCH 6/9] fix tests --- .../looker/src/worker/decorated-fetch.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/packages/looker/src/worker/decorated-fetch.test.ts b/app/packages/looker/src/worker/decorated-fetch.test.ts index 32c44aa1fe..bcf2e4f4dd 100644 --- a/app/packages/looker/src/worker/decorated-fetch.test.ts +++ b/app/packages/looker/src/worker/decorated-fetch.test.ts @@ -35,8 +35,8 @@ describe("fetchWithLinearBackoff", () => { global.fetch = vi.fn().mockRejectedValue(new Error("Network Error")); await expect( - fetchWithLinearBackoff("http://fiftyone.ai", 3) - ).rejects.toThrow("Max retries for fetch reached"); + fetchWithLinearBackoff("http://fiftyone.ai", 3, 10) + ).rejects.toThrowError(new RegExp("Max retries for fetch reached")); expect(global.fetch).toHaveBeenCalledTimes(3); }); @@ -45,9 +45,9 @@ describe("fetchWithLinearBackoff", () => { const mockResponse = new Response("Not Found", { status: 404 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); - await expect(fetchWithLinearBackoff("http://fiftyone.ai")).rejects.toThrow( - "HTTP error: 404" - ); + await expect( + fetchWithLinearBackoff("http://fiftyone.ai", 5, 10) + ).rejects.toThrow("HTTP error: 404"); expect(global.fetch).toHaveBeenCalledTimes(5); }); @@ -62,7 +62,7 @@ describe("fetchWithLinearBackoff", () => { vi.useFakeTimers(); - const fetchPromise = fetchWithLinearBackoff("http://fiftyone.ai", 5, 100); + const fetchPromise = fetchWithLinearBackoff("http://fiftyone.ai", 5, 10); // advance timers to simulate delays // after first delay From 3f74b15c48cebd92d50e14de7e39d84c8e6eef11 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 19 Nov 2024 18:59:31 -0600 Subject: [PATCH 7/9] explicit default arg --- app/packages/looker/src/worker/decorated-fetch.ts | 2 +- app/packages/looker/src/worker/index.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/packages/looker/src/worker/decorated-fetch.ts b/app/packages/looker/src/worker/decorated-fetch.ts index 19d42cb33a..dded46c056 100644 --- a/app/packages/looker/src/worker/decorated-fetch.ts +++ b/app/packages/looker/src/worker/decorated-fetch.ts @@ -21,5 +21,5 @@ export const fetchWithLinearBackoff = async ( } } } - return new Promise(() => {}); + return null; }; diff --git a/app/packages/looker/src/worker/index.ts b/app/packages/looker/src/worker/index.ts index a327d682cd..21859407e2 100644 --- a/app/packages/looker/src/worker/index.ts +++ b/app/packages/looker/src/worker/index.ts @@ -109,7 +109,7 @@ const imputeOverlayFromPath = async ( buffers: ArrayBuffer[], sources: { [path: string]: string }, cls: string, - maskPathDecodingPromises?: Promise[] + maskPathDecodingPromises: Promise[] = [] ) => { // handle all list types here if (cls === DETECTIONS) { From dbbd0b61273dc50c70030af8e4c87470f606becc Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Wed, 20 Nov 2024 12:47:21 -0600 Subject: [PATCH 8/9] set error: true preemptively --- app/packages/looker/src/elements/common/error.ts | 11 +++++------ app/packages/looker/src/elements/image.ts | 5 ++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/packages/looker/src/elements/common/error.ts b/app/packages/looker/src/elements/common/error.ts index 5523c06903..eab52a6f4b 100644 --- a/app/packages/looker/src/elements/common/error.ts +++ b/app/packages/looker/src/elements/common/error.ts @@ -103,12 +103,11 @@ export class ErrorElement extends BaseElement { } } + if (!error && this.errorElement) { + this.errorElement.remove(); + this.errorElement = null; + } + return this.errorElement; } } - -const onClick = (href) => { - let openExternal; - - return null; -}; diff --git a/app/packages/looker/src/elements/image.ts b/app/packages/looker/src/elements/image.ts index eabf771d18..5d5d7ecdfb 100644 --- a/app/packages/looker/src/elements/image.ts +++ b/app/packages/looker/src/elements/image.ts @@ -28,10 +28,12 @@ export class ImageElement extends BaseElement { update({ loaded: true, + error: false, dimensions: [this.element.naturalWidth, this.element.naturalHeight], }); }, error: ({ update }) => { + update({ error: true, dimensions: [512, 512], loaded: true }); // sometimes image loading fails because of insufficient resources // we'll want to try again in those cases if (this.retryCount < MAX_IMAGE_LOAD_RETRIES) { @@ -45,9 +47,6 @@ export class ImageElement extends BaseElement { this.element.setAttribute("src", retrySrc); // linear backoff }, 1000 * this.retryCount); - } else { - // max retries reached; finally set error to true - update({ error: true, dimensions: [512, 512], loaded: true }); } }, }; From bdc35703bde6a26bb4904b83576bd8113bf6f78b Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Fri, 22 Nov 2024 12:15:09 -0600 Subject: [PATCH 9/9] don't retry on 4xx error --- .../looker/src/worker/decorated-fetch.test.ts | 15 ++++++++-- .../looker/src/worker/decorated-fetch.ts | 28 +++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/app/packages/looker/src/worker/decorated-fetch.test.ts b/app/packages/looker/src/worker/decorated-fetch.test.ts index bcf2e4f4dd..67ed853200 100644 --- a/app/packages/looker/src/worker/decorated-fetch.test.ts +++ b/app/packages/looker/src/worker/decorated-fetch.test.ts @@ -42,16 +42,27 @@ describe("fetchWithLinearBackoff", () => { }); it("should throw an error when response is not ok", async () => { - const mockResponse = new Response("Not Found", { status: 404 }); + const mockResponse = new Response("Not Found", { status: 500 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); await expect( fetchWithLinearBackoff("http://fiftyone.ai", 5, 10) - ).rejects.toThrow("HTTP error: 404"); + ).rejects.toThrow("HTTP error: 500"); expect(global.fetch).toHaveBeenCalledTimes(5); }); + it("should throw an error when response is a 4xx, like 404", async () => { + const mockResponse = new Response("Not Found", { status: 404 }); + global.fetch = vi.fn().mockResolvedValue(mockResponse); + + await expect( + fetchWithLinearBackoff("http://fiftyone.ai", 5, 10) + ).rejects.toThrow("Non-retryable HTTP error: 404"); + + expect(global.fetch).toHaveBeenCalledTimes(1); + }); + it("should apply linear backoff between retries", async () => { const mockResponse = new Response("Success", { status: 200 }); global.fetch = vi diff --git a/app/packages/looker/src/worker/decorated-fetch.ts b/app/packages/looker/src/worker/decorated-fetch.ts index dded46c056..c77059d551 100644 --- a/app/packages/looker/src/worker/decorated-fetch.ts +++ b/app/packages/looker/src/worker/decorated-fetch.ts @@ -1,5 +1,14 @@ const DEFAULT_MAX_RETRIES = 10; const DEFAULT_BASE_DELAY = 200; +// list of HTTP status codes that are client errors (4xx) and should not be retried +const NON_RETRYABLE_STATUS_CODES = [400, 401, 403, 404, 405, 422]; + +class NonRetryableError extends Error { + constructor(message: string) { + super(message); + this.name = "NonRetryableError"; + } +} export const fetchWithLinearBackoff = async ( url: string, @@ -9,12 +18,27 @@ export const fetchWithLinearBackoff = async ( for (let i = 0; i < retries; i++) { try { const response = await fetch(url); - if (!response.ok) throw new Error(`HTTP error: ${response.status}`); - return response; + if (response.ok) { + return response; + } else { + if (NON_RETRYABLE_STATUS_CODES.includes(response.status)) { + throw new NonRetryableError( + `Non-retryable HTTP error: ${response.status}` + ); + } else { + // retry on other HTTP errors (e.g., 500 Internal Server Error) + throw new Error(`HTTP error: ${response.status}`); + } + } } catch (e) { + if (e instanceof NonRetryableError) { + // immediately throw + throw e; + } if (i < retries - 1) { await new Promise((resolve) => setTimeout(resolve, delay * (i + 1))); } else { + // max retries reached throw new Error( "Max retries for fetch reached (linear backoff), error: " + e );