-
Notifications
You must be signed in to change notification settings - Fork 147
Refactor upstream guardrails #1619
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
Conversation
This is still a WIP. I'll mark it "ready for review" once I'm done. |
6771ee2
to
9f68081
Compare
d670ca9
to
917bb89
Compare
Initial deprecation and implementation of upstream SPIs Closes quarkiverse#1284
564ce1c
to
35a3717
Compare
d9e9897
to
41f41d8
Compare
e75f556
to
7b97e36
Compare
@geoand / @cescoffier / @mariofusco I've finished this. Turns out I didn't need any of the extra hooks in upstream LangChain4j 1.2.0. I did discover a bug though in the upstream implementation (see langchain4j/langchain4j#3470). This bug is critical to making streaming work, so rather than waiting around for a LangChain4j 1.3.0 release, I did a quick hack and brought a single LangChain4j class up here into Quarkus LangChain4j. That class has the fix I need to make streaming work. See https://github.com/quarkiverse/quarkus-langchain4j/blob/7b97e36041270cb67e221a343acb8bc9091364b0/core/runtime/src/main/java/dev/langchain4j/guardrail/OutputGuardrailExecutor.java Its a bonafide bug in upstream affecting how retries work that needed to be fixed anyways. Not ideal, I know. If we don't want to bring that class up into Quarkus LC4j temporarily, then the repercussion is that if there are output guardrails on operations that are streaming (i.e. returning If we'd rather live with that and state it as a "known issue that will be fixed in the next upstream LC4j release", I'm fine with that too. We won't have to do anything here other than just consume the next released version of LC4j. I'll leave it to you to decide what you'd like to do. The fix is very, very small (moving a loop counter incrementer :) ). As far as implementation details go, the current Quarkus LC4j guardrail implementation doesn't really take advantage of the Quarkus build-time process. It uses reflection at runtime to instantiate & find annotations/etc. I've left that implementation alone in the Quarkus-specific implementation, but for the upstream LC4j integration I've fully integrated it with the build-time processing. I'm heading out on PTO and will return on August 11. |
This comment has been minimized.
This comment has been minimized.
If #1679 goes in before this, then this hack won't be needed as the fix for that bug is included there. |
I'll also resolve these conflicts this week too now that I'm back from PTO |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conflicts resolved and |
This comment has been minimized.
This comment has been minimized.
FYI I've removed the hack that I discussed in #1619 (comment) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Any idea on when this might get merged in? |
Please squash and rebase onto |
There have been lots of merges from main, conflicts, etc. To be honest I tried to rebase into a single commit and ran into lots and lots of issues and it made a mess. Why can't we squash the PR when merging? |
Because it won't create the merge commit we need. I can do the squash-rebase if you need me to |
Please |
I just merged in the latest from |
Oh wow, I have no idea how your branch got to that state... |
I'm definitely not going to deal with those conflicts :) |
since this is such a big PR i've been merging from |
I don't know what to say, really. But we can't have that PR history, it's been super clean so far and we need to keep that |
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs |
---|---|---|---|---|---|
❌ | JVM tests - integration-tests - Java 17 | Run tests of integration-tests with JDK 17 |
Failures | Logs | Raw logs |
❌ | JVM tests - integration-tests - Java 21 | Run tests of integration-tests with JDK 21 |
Failures | Logs | Raw logs |
❌ | JVM tests - integration-tests - Java 24 | Run tests of integration-tests with JDK 24 |
Failures | Logs | Raw logs |
Full information is available in the Build summary check run.
Failures
⚙️ JVM tests - integration-tests - Java 17 #
📦 integration-tests/openai
❌ org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest.guardrailMetricsAvailable
line 49
- More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException:
Assertion condition defined as a Lambda expression in org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest
Expected size: 8 but was: 0 in:
[] within 10 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
⚙️ JVM tests - integration-tests - Java 21 #
📦 integration-tests/openai
❌ org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest.guardrailMetricsAvailable
line 49
- More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException:
Assertion condition defined as a Lambda expression in org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest
Expected size: 8 but was: 0 in:
[] within 10 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
⚙️ JVM tests - integration-tests - Java 24 #
📦 integration-tests/openai
❌ org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest.guardrailMetricsAvailable
line 49
- More details - Source on GitHub
org.awaitility.core.ConditionTimeoutException:
Assertion condition defined as a Lambda expression in org.acme.example.openai.aiservices.AssistantResourceWithGuardrailsAndObservabilityTest
Expected size: 8 but was: 0 in:
[] within 10 seconds.
at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
Closing in favor of #1728 |
👍🏽 |
Refactor to use upstream guardrails.
For now I'm leaving the existing Quarkus-specific implementation in place, but fully deprecated. We can decide if we'd like to rip it out as part of this PR, or have both implementations for a little bit of time in order to give people time to transition.
Closes #1284