Add "async" API's for Odyssey to test out. #966

zaneenders · 2024-08-16T21:47:39Z

This PR duplicates the API's Currently used by Odyessey to have a different async calling pattern. Where you start the job and then can use check-status to see if the job has been completed then use a new results API to retrieve the information.

While I'm not sure if the async version of the API is a value add this didn't take long to add and allows for flexibility to experiment with how Odyessy manages jobs sent to Herbie as this has been talked about as one possible advantage of the new server design. However, I do think the results API could be useful for re-retrieving information from a job you know has been completed.

An example of how to call these new API's can be found in the testAsyncAPI function in the updated testApi.mjs file.

Very open to different API names as I just copied the convention that improve and improve-start used.

Let me know your thoughts and if I should make a PR in Odyessy using these new APIs.

elmisback · 2024-08-20T05:24:35Z

Easy starting point for an Odyssey PR would be to create an async calling function in herbiejs.ts that uses the same strategy as testAsyncAPI and try changing one of the endpoints to use that async function as its backend. (You don't have to totally replace the old functions yet, we can just deprecate and then remove them after we've tested this to be working.) Note Parth did some refactoring on this code recently so it might be a good idea to DM Parth about that.

pavpanchekha

I'm willing to merge this but we really need to do some cleanup here—lots of ugly duplicated code.

pavpanchekha · 2024-08-20T21:36:31Z

infra/testApi.mjs

+// --------------------------------------
+
+// Sample
+const sampleStartData = await testAsyncAPI("sample-start", JSON.stringify({ formula: FPCoreFormula2, seed: 5 }))


Why is this body not stored in a variable above like all the others?

pavpanchekha · 2024-08-20T21:37:00Z

infra/testApi.mjs

+
+async function testAsyncAPI(endpointName, fetchBodyString) {
+  const serverURL = `http://127.0.0.1:8000`
+  const sampleStartURL = new URL(`${serverURL}${`/api/${endpointName}`}`)


Suggested change

const sampleStartURL = new URL(`${serverURL}${`/api/${endpointName}`}`)

const sampleStartURL = new URL(`${serverURL}/api/${endpointName}`)

pavpanchekha · 2024-08-20T21:37:25Z

infra/testApi.mjs

+  const rspJSON = await rsp.json()
+  assertIdAndPath(rspJSON)
+
+  const checkStatusURL = new URL(`${serverURL}${"/check-status/"}${rspJSON.job}`)


Suggested change

const checkStatusURL = new URL(`${serverURL}${"/check-status/"}${rspJSON.job}`)

const checkStatusURL = new URL(`${serverURL}/check-status/${rspJSON.job}`)

pavpanchekha · 2024-08-20T21:38:02Z

infra/testApi.mjs

+  let counter = 0
+  let cap = 100
+  let checkStatus = await fetch(checkStatusURL, { method: 'GET' })
+  // Loop to wait for for job to finnish


Suggested change

// Loop to wait for for job to finnish

// Loop to wait for for job to finish

Big fan of the Finns but not like this.

pavpanchekha · 2024-08-20T21:38:15Z

infra/testApi.mjs

+    await new Promise(r => setTimeout(r, 100)); // ms
+  }
+  assert.equal(checkStatus.statusText, 'Job complete')
+  const resultsURL = new URL(`${serverURL}${"/api/results/"}${rspJSON.job}`)


Suggested change

const resultsURL = new URL(`${serverURL}${"/api/results/"}${rspJSON.job}`)

const resultsURL = new URL(`${serverURL}/api/results/${rspJSON.job}`)

pavpanchekha · 2024-08-20T21:39:21Z

src/api/demo.rkt

                  [("api" "sample") #:method "post" sample-endpoint]
+                  [("api" "sample-start") #:method "post" sample-start-endpoint]


I mean, these are all fine, it's not the end of the world or anything, but I think it would be a lot nicer to do /api/start/foo instead of /api/foo-start. We already have a separator character!

pavpanchekha · 2024-08-20T21:40:44Z

src/api/demo.rkt

+(define analyze-start-endpoint
+  (post-with-json-response (lambda (post-data)
+                             (define formula-str (hash-ref post-data 'formula))
+                             (define formula (read-syntax 'web (open-input-string formula-str)))
+                             (define sample (hash-ref post-data 'sample))
+                             (define seed (hash-ref post-data 'seed #f))
+                             (define test (parse-test formula))
+                             (define pcontext (json->pcontext sample (test-context test)))
+                             (define command
+                               (create-job 'errors
+                                           test
+                                           #:seed seed
+                                           #:pcontext pcontext
+                                           #:profile? #f
+                                           #:timeline-disabled? #t))
+                             (define job-id (start-job command))
+                             (hasheq 'job job-id 'path (make-path job-id)))))


I could be convinced to merge this but surely this is extremely duplicative with the existing non-async endpoints? Could we have a single function and then generic wrappers around it to make it sync or async?

zaneenders · 2024-09-01T22:49:41Z

Replaced by PR #983.

zaneenders requested review from pavpanchekha and elmisback August 16, 2024 21:50

zaneenders force-pushed the zane-async-apis branch from 734e1cf to c661108 Compare August 16, 2024 21:58

zaneenders force-pushed the zane-async-apis branch from c661108 to a68914c Compare August 20, 2024 15:50

pavpanchekha approved these changes Aug 20, 2024

View reviewed changes

zaneenders force-pushed the zane-async-apis branch from a68914c to bf8dce5 Compare September 1, 2024 17:16

zaneenders added 8 commits September 1, 2024 11:20

Add sample-start async api.

46a7a23

Add analyze-start async api.

2a95f02

localerror-start

e1ec338

Add alternatives-start async API.

092c9c8

Add exacts-start async api.

8883ef6

Add calculate-start async api.

40b8c16

Add cost-start async api.

0f8db66

Add explanations-start async api.

6bdd26c

zaneenders force-pushed the zane-async-apis branch from bf8dce5 to 6bdd26c Compare September 1, 2024 17:20

zaneenders mentioned this pull request Sep 1, 2024

Add async API's #983

Merged

zaneenders closed this Sep 1, 2024

zaneenders deleted the zane-async-apis branch September 1, 2024 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "async" API's for Odyssey to test out. #966

Add "async" API's for Odyssey to test out. #966

zaneenders commented Aug 16, 2024 •

edited by pavpanchekha

Loading

elmisback commented Aug 20, 2024

pavpanchekha left a comment

pavpanchekha Aug 20, 2024 •

edited

Loading

pavpanchekha Aug 20, 2024

pavpanchekha Aug 20, 2024

pavpanchekha Aug 20, 2024

pavpanchekha Aug 20, 2024

pavpanchekha Aug 20, 2024

pavpanchekha Aug 20, 2024

zaneenders commented Sep 1, 2024

	const sampleStartURL = new URL(`${serverURL}${`/api/${endpointName}`}`)
	const sampleStartURL = new URL(`${serverURL}/api/${endpointName}`)

	const checkStatusURL = new URL(`${serverURL}${"/check-status/"}${rspJSON.job}`)
	const checkStatusURL = new URL(`${serverURL}/check-status/${rspJSON.job}`)

	// Loop to wait for for job to finnish
	// Loop to wait for for job to finish

	const resultsURL = new URL(`${serverURL}${"/api/results/"}${rspJSON.job}`)
	const resultsURL = new URL(`${serverURL}/api/results/${rspJSON.job}`)

		[("api" "sample") #:method "post" sample-endpoint]
		[("api" "sample-start") #:method "post" sample-start-endpoint]

Add "async" API's for Odyssey to test out. #966

Add "async" API's for Odyssey to test out. #966

Conversation

zaneenders commented Aug 16, 2024 • edited by pavpanchekha Loading

elmisback commented Aug 20, 2024

pavpanchekha left a comment

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

pavpanchekha Aug 20, 2024

Choose a reason for hiding this comment

zaneenders commented Sep 1, 2024

zaneenders commented Aug 16, 2024 •

edited by pavpanchekha

Loading

pavpanchekha Aug 20, 2024 •

edited

Loading