-
Notifications
You must be signed in to change notification settings - Fork 214
Add OpenAI extension #8227
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
base: main
Are you sure you want to change the base?
Add OpenAI extension #8227
Conversation
… check-extension-pages mojo
| .statusCode(204); | ||
|
|
||
| // Assert the streamed results | ||
| IntStream.rangeClosed(1, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we rely on message ordering for expectation ? If yes, not sure the route would preserve order, not sure of the opposite though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do rely on ordering for this test...
Unless the LLM gets the order of numbers wrong (unlikely), won't the streamed chunks come in the expected 1-10 order?
Or do you think the messages arriving at the seda endpoint might be out of order (I've not seen it happen so far in local testing)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more fearing a kind of race condition with wireTap thread pool but nothing demonstrated. If we could not simply switch to non order expectations now, we are not taking a big risk anyway. Even if flaky, it would not happen a lot 🤞 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - thanks for the feedback. Lets see how it goes. If it does prove to be flaky on CI, I will rework it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aldettinger I think you are right about this!
Seems the latest Windows failure is due to unexpected ordering.
I'll try to rework the test to make it more reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that said the probability should have been quite low. Anyway, no order in the route is not a big deal, just need to relax assertion.
aldettinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few neat questions. The comments and doc are really helpful for review 👏
|
I need to fix things up for Windows where I added test file deletion: |
3e18d12 to
dd05e3a
Compare
dd05e3a to
f49327d
Compare
The
openaicomponent is not in the 4.17.0 catalog. This scenario happens occasionally, so I added a small 'enhancement' to thecheck-extension-pagesmojo. So that it can continue to generate the expected doc resources. I intend to immediately remove the additional plugin config once this change lands on thecamel-mainbranch.