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

feat: Add streaming API to orchestration #352

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

tomfrenken
Copy link
Member

@tomfrenken tomfrenken commented Dec 6, 2024

Context

SAP/ai-sdk-js-backlog#161.

This PR adds a streaming API to our orchestration client.

Definition of Done

  • Code is tested (Unit, E2E)
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • (Optional) Aligned changes with the Java SDK
  • (Optional) Release notes updated

@tomfrenken tomfrenken changed the title feat: Add streaming API to orchestration WIP: feat: Add streaming API to orchestration Dec 6, 2024
Copy link
Contributor

@ZhongpinWang ZhongpinWang left a comment

Choose a reason for hiding this comment

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

Already looks promising. For code reuse, maybe after more finds are gathered, we can look into it again to see if it is worth the effort.

@tomfrenken tomfrenken changed the title WIP: feat: Add streaming API to orchestration feat: Add streaming API to orchestration Dec 17, 2024
sample-code/src/orchestration.ts Outdated Show resolved Hide resolved
sample-code/src/orchestration.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-client.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ZhongpinWang ZhongpinWang left a comment

Choose a reason for hiding this comment

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

One more round

packages/orchestration/src/orchestration-client.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-types.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-utils.ts Outdated Show resolved Hide resolved
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

Didn't get through everything, but some ideas. Also, let's remember to add E2E tests

packages/orchestration/src/orchestration-client.ts Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/core/src/stream/line-decoder.ts Show resolved Hide resolved
packages/orchestration/README.md Show resolved Hide resolved
packages/orchestration/src/orchestration-client.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-types.ts Outdated Show resolved Hide resolved
packages/orchestration/README.md Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
@tomfrenken tomfrenken dismissed stale reviews from KavithaSiva and deekshas8 January 10, 2025 11:19

Done

@tomfrenken
Copy link
Member Author

Side node while I add streaming to our sample code and e2e tests, we should really refactor our server.ts file, lots of duplication and it should be more modularized ...

packages/orchestration/README.md Show resolved Hide resolved
tests/e2e-tests/src/orchestration.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ZhongpinWang ZhongpinWang left a comment

Choose a reason for hiding this comment

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

After e2e added

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.

6 participants