Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor application structure, implement orchestrator with unary handling #7

Merged
merged 8 commits into from
May 3, 2024

Conversation

declark1
Copy link
Collaborator

@declark1 declark1 commented May 1, 2024

This PR refactors the application structure and implements various other changes tested during our initial PoC which we will continue to iterate on, including:

  • Config enhancements
  • Implements Orchestrator with end-to-end unary task handling (and placeholders for streaming)
  • Refactors clients (DetectorClient, NlpClient, TgisClient) to be higher-level with all methods (including streaming) fully implemented
  • Custom error types
  • Type conversions across APIs, e.g. generation parameters
  • Updates dependencies
  • Formatted and linted code (all clippy lints cleared)
  • Added Dockerfile

TODO:

  • Add doc comments to orchestrator task handlers
  • Draft initial docs, including diagram illustrating the unary task processing sequence
  • Improve error handling
    • Propogate errors up from all orchestrator tasks
    • Properly convert Error to appropriate http::StatusCode with context

@declark1 declark1 requested review from gkumbhat and evaline-ju and removed request for gkumbhat and evaline-ju May 1, 2024 19:41
Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dan for all the 🌶️ changes. Loving all the refactors. This is much cleaner. I went through all the files except orchestrator.rs and left some questions and comments!

src/clients.rs Show resolved Hide resolved
src/clients.rs Show resolved Hide resolved
src/clients.rs Show resolved Hide resolved
src/clients.rs Show resolved Hide resolved
src/clients/detector.rs Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/models.rs Show resolved Hide resolved
src/server.rs Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 yay parallel calls! Thanks for all the cleanup here - just some q's

config/config.yaml Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/orchestrator.rs Show resolved Hide resolved
src/orchestrator.rs Show resolved Hide resolved
src/orchestrator.rs Show resolved Hide resolved
src/orchestrator.rs Show resolved Hide resolved
src/orchestrator.rs Show resolved Hide resolved
config/config.yaml Outdated Show resolved Hide resolved
@declark1 declark1 force-pushed the structure-refactor branch from e8fb32c to 0240ff6 Compare May 2, 2024 18:28
declark1 and others added 7 commits May 3, 2024 10:33
Co-authored-by: Evaline Ju <[email protected]>
Signed-off-by: declark1 <[email protected]>
…ty. added rust-toolchain.toml. dropped unused methods.

Signed-off-by: declark1 <[email protected]>
@declark1 declark1 force-pushed the structure-refactor branch from b425b41 to c10a295 Compare May 3, 2024 17:34
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks Dan

@gkumbhat gkumbhat merged commit 23f64b2 into main May 3, 2024
1 check passed
@gkumbhat gkumbhat deleted the structure-refactor branch May 3, 2024 18:03
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