Skip to content

Conversation

@zaneenders
Copy link
Contributor

@zaneenders zaneenders commented Sep 23, 2024

This PR switches Odyessy to using the new Async for the API's that are available to be called that way. I don't know if that is what we want to do but it did make testing that the API's were working correctly with this new calling convention when testing locally.

This PR does rely on this Herbie PR to enable CORS for the checkstatus endpoint. This has been merged with main now.

@elmisback
Copy link
Collaborator

Awesome! I think the process for doing this will be

  1. Me: confirm that this is working
  2. Me: Update Herbie binaries
  3. Me: Merge
  4. Me: Field any bug reports, direct to Zane as needed

I'll use this PR as the issue for tracking this work

@zaneenders
Copy link
Contributor Author

Let me know how I can help.

@zaneenders
Copy link
Contributor Author

Just thought of a bug/error in this code.
If for some reason the request times out (10s / counter reaches 100) an error is not thrown in this state and the function proceeds as if the job is ready to be collected. Will update to throw an error in this state.

@elmisback elmisback self-assigned this Sep 30, 2024
@elmisback
Copy link
Collaborator

elmisback commented Sep 30, 2024

Remaining steps:

  • After Thursday, restart the Herbie server on 6490ee3d760ab48b7c801afb0bded6451e27a5a2 (September 23) or later
  • Update Herbie binaries with sufficiently-recent version of Odyssey, or just point to server by default for now
  • Merge

@elmisback
Copy link
Collaborator

I'll test against the updated server today, won't point to the server or merge yet

@elmisback
Copy link
Collaborator

No progress due to OOPSLA

@elmisback
Copy link
Collaborator

No progress this week due to generals, but I should have some time to work on this on Thursday/Friday. I'll test against the server now that it's updated and switch local Odyssey to point to the server until binaries are updated.

@zaneenders
Copy link
Contributor Author

Worked on adding a fallback check if the async API's don't exist. Getting some weird errors with errors not being caught.
Will circle back on.

@elmisback
Copy link
Collaborator

I'm merging this branch now, tested locally and it works.

@elmisback elmisback merged commit 3855e7b into main Nov 4, 2024
1 check passed
@elmisback elmisback deleted the zane-call-async branch May 2, 2025 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants