From 1567f93dca92c2faf1514c25539ff01e4de44425 Mon Sep 17 00:00:00 2001 From: Quentin Roy Date: Thu, 7 Dec 2023 19:01:31 +0100 Subject: [PATCH] log-client: fix resuming with no matching logs (#206) * log-client: fix resuming with no matching logs * log-client: add new resume tests --- .changeset/honest-dolls-smash.md | 5 ++ packages/log-client/__tests__/main.test.ts | 67 +++++++++++++++++----- packages/log-client/src/log-client.ts | 9 ++- 3 files changed, 63 insertions(+), 18 deletions(-) create mode 100644 .changeset/honest-dolls-smash.md diff --git a/.changeset/honest-dolls-smash.md b/.changeset/honest-dolls-smash.md new file mode 100644 index 00000000..8e10df54 --- /dev/null +++ b/.changeset/honest-dolls-smash.md @@ -0,0 +1,5 @@ +--- +'@lightmill/log-client': patch +--- + +Fix resuming a log when there are no matching logs found. diff --git a/packages/log-client/__tests__/main.test.ts b/packages/log-client/__tests__/main.test.ts index 6ad11fc8..61a1ee97 100644 --- a/packages/log-client/__tests__/main.test.ts +++ b/packages/log-client/__tests__/main.test.ts @@ -222,7 +222,9 @@ describe('RunLogger#resumeRun', () => { runId: 'test-run', experimentId: 'test-experiment', }); - await logger.resumeRun({ resumeAfterLast: 'test-type' }); + await expect( + logger.resumeRun({ resumeAfterLast: 'test-type' }), + ).resolves.toEqual({ type: 'test-type', number: 4 }); await expect(waitForChangeRequests()).resolves.toEqual([ { url: 'https://server.test/api/experiments/test-experiment/runs/test-run', @@ -250,7 +252,9 @@ describe('RunLogger#resumeRun', () => { runId: 'test-run', experimentId: 'test-experiment', }); - await logger.resumeRun({ resumeAfterLast: ['test-type1', 'test-type2'] }); + await expect( + logger.resumeRun({ resumeAfterLast: ['test-type1', 'test-type2'] }), + ).resolves.toEqual({ type: 'test-type1', number: 3 }); await expect(waitForChangeRequests()).resolves.toEqual([ { url: 'https://server.test/api/experiments/test-experiment/runs/test-run', @@ -260,6 +264,35 @@ describe('RunLogger#resumeRun', () => { ]); }); + it('should resume an existing run even if no matching logs are found', async () => { + setServerRuns([ + { + runId: 'test-run', + experimentId: 'test-experiment', + status: 'running', + logs: [ + { type: 'test-type1', count: 3, lastNumber: 5, pending: 2 }, + { type: 'test-type2', count: 1, lastNumber: 1, pending: 1 }, + ], + }, + ]); + const logger = new LogClient({ + apiRoot: 'https://server.test/api', + runId: 'test-run', + experimentId: 'test-experiment', + }); + await expect( + logger.resumeRun({ resumeAfterLast: ['other-type'] }), + ).resolves.toBeNull(); + await expect(waitForChangeRequests()).resolves.toEqual([ + { + url: 'https://server.test/api/experiments/test-experiment/runs/test-run', + method: 'PATCH', + body: { resumeFrom: 1 }, + }, + ]); + }); + it('should be able to use the runId and experimentId from its parameter', async () => { setServerRuns([ { @@ -274,11 +307,13 @@ describe('RunLogger#resumeRun', () => { }, ]); const logger = new LogClient({ apiRoot: 'https://server.test/api' }); - await logger.resumeRun({ - resumeAfterLast: ['test-type1', 'test-type2'], - experimentId: 'test-experiment', - runId: 'test-run', - }); + await expect( + logger.resumeRun({ + resumeAfterLast: ['test-type1', 'test-type2'], + experimentId: 'test-experiment', + runId: 'test-run', + }), + ).resolves.toEqual({ type: 'test-type1', number: 3 }); await expect(waitForChangeRequests()).resolves.toEqual([ { url: 'https://server.test/api/experiments/test-experiment/runs/test-run', @@ -297,18 +332,20 @@ describe('RunLogger#resumeRun', () => { logs: [ { type: 'test-type1', count: 3, lastNumber: 5, pending: 2 }, { type: 'test-type2', count: 1, lastNumber: 1, pending: 1 }, - { type: 'other-type', count: 3, lastNumber: 7, pending: 0 }, + { type: 'other-type', count: 4, lastNumber: 8, pending: 0 }, ], }, ]); const logger = new LogClient({ apiRoot: 'https://server.test/api', }); - await logger.resumeRun({ - resumeAfterLast: ['test-type1', 'test-type2'], - experimentId: 'test-experiment', - runId: 'test-run', - }); + await expect( + logger.resumeRun({ + resumeAfterLast: ['test-type1', 'test-type2'], + experimentId: 'test-experiment', + runId: 'test-run', + }), + ).resolves.toEqual({ type: 'test-type1', number: 3 }); await expect(waitForChangeRequests()).resolves.toEqual([ { url: 'https://server.test/api/experiments/test-experiment/runs/test-run', @@ -318,7 +355,7 @@ describe('RunLogger#resumeRun', () => { ]); }); - it("should refuse to overwrite the constructor's experimentId", async () => { + it('should refuse to resume a run with an experimentId different than the one provided in the constructor', async () => { const logger = new LogClient({ apiRoot: 'https://server.test/api', experimentId: 'original-experiment', @@ -332,7 +369,7 @@ describe('RunLogger#resumeRun', () => { ).rejects.toThrowError(Error); }); - it("should refuse to overwrite the constructor's runId", async () => { + it('should refuse to resume a run with a runId different than the one provided in the constructor', async () => { const logger = new LogClient({ apiRoot: 'https://server.test/api', runId: 'original-run', diff --git a/packages/log-client/src/log-client.ts b/packages/log-client/src/log-client.ts index f6029e16..a1f99511 100644 --- a/packages/log-client/src/log-client.ts +++ b/packages/log-client/src/log-client.ts @@ -164,8 +164,9 @@ export class LogClient { .filter((l): l is { type: T } & typeof l => resumeAfterLastSet.has(l.type), ) - .reduce((maxLog, log) => - maxLog.lastNumber > log.lastNumber ? maxLog : log, + .reduce( + (maxLog, log) => (maxLog.lastNumber > log.lastNumber ? maxLog : log), + { lastNumber: 0, count: 0, type: null as null | T }, ); await Interface.resumeRun({ apiRoot: this.#apiRoot, @@ -177,7 +178,9 @@ export class LogClient { this.#experimentId = experimentId; this.#runStatus = 'running'; this.#logCount = lastLog.lastNumber; - return { type: lastLog.type, number: lastLog.count }; + return lastLog.type == null + ? null + : { type: lastLog.type, number: lastLog.count }; } catch (err) { this.#runStatus = 'error'; throw err;