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

[Streaming] Fix issues when not using try-with-resource #49

Merged
merged 106 commits into from
Oct 30, 2024
Merged

Conversation

newtork
Copy link
Contributor

@newtork newtork commented Sep 4, 2024

Make the streaming API lenient towards try-with-resources.

Before:

  • with try-with-resource: HTTP connections are closed in normal cases, response errors, http errors
  • without try-with-resource: HTTP connections are NOT closed

After:

  • with try-with-resource: HTTP connections are closed in normal cases, response errors, http errors
  • without try-with-resource: HTTP connections are closed in normal cases, and http errors. Errors in response payload does not lead to closing of HTTP connection.
- InputStream <> BufferedReader <> Stream
+ InputStream <> BufferedReader <> [HandledIterator] <> Stream

The suggested HandledIterator understands when BufferedReader#readLine() returns null, then the underlying InputStream shall be closed [left]. Also when the consuming Stream is closed or finished iteration [right].

See JavaDoc of String BufferedReader#readLine() to understand null contract.

image

See JavaDoc and code of Stream BufferedReader#lines() for comparison.

image

Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP left a comment

Choose a reason for hiding this comment

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

I get a timeout on http://localhost:8080/streamChatCompletionDeltas

Comment on lines 65 to 68
} catch (final Exception e) {
isDone = true;
stopHandler.run();
throw new IllegalStateException("Iterator stopped unexpectedly.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

What can be thrown here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exception caught here would be thrown by readHandler.call().
In this case that would be from BufferedReader#readLine.
The readLine may throw IOException – If an I/ O error occurs.
Presumably this would happen when the nested InputStream#read would fail.

One possibility for an InputStream from HttpEntity to fail would be a network interruption.

Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP Oct 29, 2024

Choose a reason for hiding this comment

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

I fear regularly receiving support issues with IllegalStateException: Iterator stopped unexpectedly.
I suggest either do not throw anything or update the exception catching IOException and with a message like IOException, Stream closed unexpectedly. This is not an IllegalStateException if it can realistically happen.

Copy link
Contributor Author

@newtork newtork Oct 29, 2024

Choose a reason for hiding this comment

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

I agree, network issues will occur someday anyway. So let's improve error messages. fb21544

(Unfortunately IOException is not RuntimeException.)

@MatKuhr
Copy link
Member

MatKuhr commented Oct 22, 2024

Let's finish this PR up so that we are free to continue with streaming for orchestration

@newtork
Copy link
Contributor Author

newtork commented Oct 28, 2024

I get a timeout on http://localhost:8080/streamChatCompletionDeltas

I was not able to reproduce the timeout. Please kindly recheck.

@newtork newtork added the please-review Request to review a pull-request label Oct 28, 2024
@newtork newtork merged commit 20cb326 into main Oct 30, 2024
5 checks passed
@newtork newtork deleted the streaming-2 branch October 30, 2024 11:45
MatKuhr added a commit that referenced this pull request Nov 5, 2024
* chore: [DevOps] bump the test group with 2 updates (#121)

Bumps the test group with 2 updates: [org.junit.jupiter:junit-jupiter-api](https://github.com/junit-team/junit5) and [org.junit.jupiter:junit-jupiter-params](https://github.com/junit-team/junit5).


Updates `org.junit.jupiter:junit-jupiter-api` from 5.11.2 to 5.11.3
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.11.2...r5.11.3)

Updates `org.junit.jupiter:junit-jupiter-params` from 5.11.2 to 5.11.3
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.11.2...r5.11.3)

---
updated-dependencies:
- dependency-name: org.junit.jupiter:junit-jupiter-api
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: test
- dependency-name: org.junit.jupiter:junit-jupiter-params
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: test
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: [DevOps] bump the plugins group with 7 updates (#120)

Bumps the plugins group with 7 updates:

| Package | From | To |
| --- | --- | --- |
| [org.apache.maven.plugins:maven-dependency-plugin](https://github.com/apache/maven-dependency-plugin) | `3.8.0` | `3.8.1` |
| [org.apache.maven.plugins:maven-checkstyle-plugin](https://github.com/apache/maven-checkstyle-plugin) | `3.5.0` | `3.6.0` |
| [com.github.spotbugs:spotbugs-maven-plugin](https://github.com/spotbugs/spotbugs-maven-plugin) | `4.8.6.4` | `4.8.6.5` |
| [org.springframework.boot:spring-boot-autoconfigure](https://github.com/spring-projects/spring-boot) | `3.3.4` | `3.3.5` |
| [org.springframework.boot:spring-boot](https://github.com/spring-projects/spring-boot) | `3.3.4` | `3.3.5` |
| [org.springframework.boot:spring-boot-starter-web](https://github.com/spring-projects/spring-boot) | `3.3.4` | `3.3.5` |
| [org.springframework.boot:spring-boot-maven-plugin](https://github.com/spring-projects/spring-boot) | `3.3.4` | `3.3.5` |


Updates `org.apache.maven.plugins:maven-dependency-plugin` from 3.8.0 to 3.8.1
- [Release notes](https://github.com/apache/maven-dependency-plugin/releases)
- [Commits](apache/maven-dependency-plugin@maven-dependency-plugin-3.8.0...maven-dependency-plugin-3.8.1)

Updates `org.apache.maven.plugins:maven-checkstyle-plugin` from 3.5.0 to 3.6.0
- [Commits](apache/maven-checkstyle-plugin@maven-checkstyle-plugin-3.5.0...maven-checkstyle-plugin-3.6.0)

Updates `com.github.spotbugs:spotbugs-maven-plugin` from 4.8.6.4 to 4.8.6.5
- [Release notes](https://github.com/spotbugs/spotbugs-maven-plugin/releases)
- [Commits](spotbugs/spotbugs-maven-plugin@spotbugs-maven-plugin-4.8.6.4...spotbugs-maven-plugin-4.8.6.5)

Updates `org.springframework.boot:spring-boot-autoconfigure` from 3.3.4 to 3.3.5
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.3.4...v3.3.5)

Updates `org.springframework.boot:spring-boot` from 3.3.4 to 3.3.5
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.3.4...v3.3.5)

Updates `org.springframework.boot:spring-boot-starter-web` from 3.3.4 to 3.3.5
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.3.4...v3.3.5)

Updates `org.springframework.boot:spring-boot-maven-plugin` from 3.3.4 to 3.3.5
- [Release notes](https://github.com/spring-projects/spring-boot/releases)
- [Commits](spring-projects/spring-boot@v3.3.4...v3.3.5)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-dependency-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.apache.maven.plugins:maven-checkstyle-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: plugins
- dependency-name: com.github.spotbugs:spotbugs-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.springframework.boot:spring-boot-autoconfigure
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.springframework.boot:spring-boot
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.springframework.boot:spring-boot-starter-web
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.springframework.boot:spring-boot-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: [DevOps] bump the production-minor-patch group across 1 directory with 3 updates (#122)

Bumps the production-minor-patch group with 3 updates in the / directory: [org.wiremock:wiremock](https://github.com/wiremock/wiremock), [com.puppycrawl.tools:checkstyle](https://github.com/checkstyle/checkstyle) and [ch.qos.logback:logback-classic](https://github.com/qos-ch/logback).


Updates `org.wiremock:wiremock` from 3.9.1 to 3.9.2
- [Release notes](https://github.com/wiremock/wiremock/releases)
- [Commits](wiremock/wiremock@3.9.1...3.9.2)

Updates `com.puppycrawl.tools:checkstyle` from 10.18.2 to 10.19.0
- [Release notes](https://github.com/checkstyle/checkstyle/releases)
- [Commits](checkstyle/checkstyle@checkstyle-10.18.2...checkstyle-10.19.0)

Updates `ch.qos.logback:logback-classic` from 1.5.11 to 1.5.12
- [Commits](qos-ch/logback@v_1.5.11...v_1.5.12)

---
updated-dependencies:
- dependency-name: org.wiremock:wiremock
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: production-minor-patch
- dependency-name: com.puppycrawl.tools:checkstyle
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: production-minor-patch
- dependency-name: ch.qos.logback:logback-classic
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: production-minor-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Chore/sample app readme update (#125)

* Update Sample Code Readme

* F

* Fix typo

* Simpler Config Variant

* [Streaming] Fix issues when not using `try-with-resource` (#49)

* OpenAI streaming

* Added homepage and error handling todo

* Renamed vars

* Added todos

* Made stream generic, try-with resources, TEXT_EVENT_STREAM, exception refactored

* Formatting

* close stream correctly

* Formatting

* Created OpenAiStreamOutput

* Formatting

* Renamed stream to streamChatCompletion, Added comments

* Added total output

* Total output is printed

* Formatting

* addDelta is propagated everywhere

* addDelta is propagated everywhere

* forgotten addDeltas

* Added jackson dependencies

* Added Javadoc

* Removed 1 TODO

* PMD

* PMD again

* Added OpenAiClientTest.streamChatCompletion()

* Change return type of stream, added e2e test

* Added documentation

* Added documentation framework-agnostic + throw if finish reason is invalid

* Added error handling test

* Updates from pair review / discussion

* Cleanup + streamChatCompletion doesn't throw

* PMD

* Added errorHandling test

* Apply suggestions from code review

Co-authored-by: Matthias Kuhr <[email protected]>

* Dependency analyze

* Review comments

* Make client static

* Formatting

* PMD

* Fix tests

* Removed exception constructors no args

* Refactor exception message

* Readme sentences

* Remove superfluous call super

* reset httpclient-cache and -factory after each test case

* Very minor code-style improvements in test

* Minor code-style in OpenAIController

* Reduce README sample code

* Update OpenAiStreamingHandler.java (#43)

* Fix import

* Initial

* Format

* Improve type

* Added stream_options to model

* Change Executor#submit() to #execute()

* Change Executor#submit() to #execute()

* Added usage testing

* Added beautiful Javadoc to enableStreaming

* typo

* Fix mistake

* Syntax improvement to improve API stability.

* Syntax improvement to improve API stability.

* Make exception types similar to BufferedReader original logic

* Format

* Add nonnull characteristic to mirror BufferedReader original logic

* Make buffer size accessible

* Add test

* Add assertion on stream count

* Simplify e2e code

* Simplify README

* Partially revert

* Add assertion

* Partially revert

* Minor code adjustments

* Replace unnecessary nested types

* Merge nested type to renamed parent type

* Change code to ensure our lazy `hasNext()` has no unexpected side effect

* Revert removing `emitter#complete()`

* Add JavaDoc; Replace VAVR type

* Address PMD warnings: change exception type

* Add unhappy-path test cases

* Revert code change in test app

* Initial migrate coverage check to pom xml

* Print FULL coverage report with git diff indicator

* Update foundation-models/openai/src/test/java/com/sap/ai/sdk/foundationmodels/openai/IterableStreamConverterTest.java

Co-authored-by: Charles Dubois <[email protected]>

* Update foundation-models/openai/src/test/java/com/sap/ai/sdk/foundationmodels/openai/IterableStreamConverterTest.java

Co-authored-by: Charles Dubois <[email protected]>

* Reduce redundant method

* Add comment

* Fix merge error

* Fix JavaDoc inaccessibility warning

* Improve error message

---------

Co-authored-by: I538344 <[email protected]>
Co-authored-by: SAP Cloud SDK Bot <[email protected]>
Co-authored-by: Matthias Kuhr <[email protected]>
Co-authored-by: Charles Dubois <[email protected]>
Co-authored-by: Matthias Kuhr <[email protected]>
Co-authored-by: Alexander Dümont <[email protected]>

* Orchestration Client (#128)

* Orchestration Client

* Documentation fix

* Added error tests

* Better Javadoc

* Updated AI Core unit tests (#131)

* Added Cloud SDK logo (#132)

* `AICORE_SERVICE_KEY` in `.env` file (#129)

* Allow File Path in AI Core Env Var

* Sonarlint

* Documentation

* Update README.md

Co-authored-by: Alexander Dümont <[email protected]>

* Accept both env var and .env

* Updated doc

* Apply suggestions from code review

Co-authored-by: Alexander Dümont <[email protected]>

* Do not use System.getenv anymore

* Fix links

* ℹ️ The environment variable has priority over the `.env` file.

---------

Co-authored-by: Alexander Dümont <[email protected]>

* Initial (#130)

Co-authored-by: Alexander Dümont <[email protected]>

* Remove redundant property (#126)

* Remove redundant property

* Removed empty inputParams

---------

Co-authored-by: Alexander Dümont <[email protected]>
Co-authored-by: I538344 <[email protected]>

* chore: [DevOps] bump the plugins group with 3 updates (#134)

Bumps the plugins group with 3 updates: [org.apache.maven.plugins:maven-pmd-plugin](https://github.com/apache/maven-pmd-plugin), [org.apache.maven.plugins:maven-surefire-plugin](https://github.com/apache/maven-surefire) and [org.apache.maven.plugins:maven-javadoc-plugin](https://github.com/apache/maven-javadoc-plugin).


Updates `org.apache.maven.plugins:maven-pmd-plugin` from 3.25.0 to 3.26.0
- [Release notes](https://github.com/apache/maven-pmd-plugin/releases)
- [Commits](apache/maven-pmd-plugin@maven-pmd-plugin-3.25.0...maven-pmd-plugin-3.26.0)

Updates `org.apache.maven.plugins:maven-surefire-plugin` from 3.5.1 to 3.5.2
- [Release notes](https://github.com/apache/maven-surefire/releases)
- [Commits](apache/maven-surefire@surefire-3.5.1...surefire-3.5.2)

Updates `org.apache.maven.plugins:maven-javadoc-plugin` from 3.10.1 to 3.11.1
- [Release notes](https://github.com/apache/maven-javadoc-plugin/releases)
- [Commits](apache/maven-javadoc-plugin@maven-javadoc-plugin-3.10.1...maven-javadoc-plugin-3.11.1)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-pmd-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: plugins
- dependency-name: org.apache.maven.plugins:maven-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: plugins
- dependency-name: org.apache.maven.plugins:maven-javadoc-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: plugins
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: [DevOps] bump the production-minor-patch group with 2 updates (#133)

Bumps the production-minor-patch group with 2 updates: [io.github.cdimascio:dotenv-java](https://github.com/cdimascio/dotenv-java) and [com.puppycrawl.tools:checkstyle](https://github.com/checkstyle/checkstyle).


Updates `io.github.cdimascio:dotenv-java` from 3.0.0 to 3.0.2
- [Release notes](https://github.com/cdimascio/dotenv-java/releases)
- [Changelog](https://github.com/cdimascio/dotenv-java/blob/master/CHANGELOG.md)
- [Commits](cdimascio/dotenv-java@v3.0.0...v3.0.2)

Updates `com.puppycrawl.tools:checkstyle` from 10.19.0 to 10.20.0
- [Release notes](https://github.com/checkstyle/checkstyle/releases)
- [Commits](checkstyle/checkstyle@checkstyle-10.19.0...checkstyle-10.20.0)

---
updated-dependencies:
- dependency-name: io.github.cdimascio:dotenv-java
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: production-minor-patch
- dependency-name: com.puppycrawl.tools:checkstyle
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: production-minor-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Dotend load is static (#135)

* [DevOps] Skip Code Generation By Default (#123)

* DevOps Skip Code Generation By Default

* Fix pom file

* Initial migrate coverage check to pom xml (#116)

Co-authored-by: Alexander Dümont <[email protected]>

* Remove unnecessary steps (#115)

Co-authored-by: Alexander Dümont <[email protected]>

* README fix for AICORE_SERVICE_KEY instructions (#137)

* Switch to passing config with method call

* Add Javadoc

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Alexander Dümont <[email protected]>
Co-authored-by: I538344 <[email protected]>
Co-authored-by: SAP Cloud SDK Bot <[email protected]>
Co-authored-by: Charles Dubois <[email protected]>
Co-authored-by: Alexander Dümont <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please-review Request to review a pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants