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.

Sign up for GitHub

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

Jump to bottom

[Obs AI Assistant] Add ES function API test #187465

Merged
merged 11 commits into from
Jul 6, 2024
Merged

[Obs AI Assistant] Add ES function API test #187465

merged 11 commits into from
Jul 6, 2024

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jul 3, 2024 •
edited by kibanamachine
Loading

Related to #180787

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@sorenlouv
Copy link
Member Author

buildkite test this

@sorenlouv sorenlouv added release_note:skip Skip the PR/issue when compiling release notes v8.15.0 labels Jul 3, 2024
@sorenlouv sorenlouv marked this pull request as ready for review July 3, 2024 13:53
@sorenlouv sorenlouv requested a review from a team as a code owner July 3, 2024 13:53
dgieselaar
dgieselaar reviewed Jul 5, 2024
View reviewed changes
x-pack/test/observability_ai_assistant_api_integration/tests/complete/functions/helpers.ts Outdated
proxy.close();
}

export function foo(
Copy link
Member

Choose a reason for hiding this comment

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

foo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just keeping you on your toes :D Good catch!

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:Obs AI Assistant Observability AI Assistant labels Jul 5, 2024
@dgieselaar
Copy link
Member

Thanks for adding these!

sorenlouv
sorenlouv commented Jul 5, 2024
View reviewed changes
...n/observability_ai_assistant/server/service/client/get_context_function_request_if_needed.ts
@@ -22,11 +22,10 @@ export function getContextFunctionRequestIfNeeded(
.slice(indexOfLastUserMessage)
.some((message) => message.message.name === CONTEXT_FUNCTION_NAME);

if (hasContextSinceLastUserMessage) {
const isLastMessageFunctionRequest = !!last(messages)?.message.function_call?.name;
if (hasContextSinceLastUserMessage || isLastMessageFunctionRequest) {
Copy link
Member Author

@sorenlouv sorenlouv Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dgieselaar Does this change look ok to you? Before we'd inject the context request immediately after other function requests effectively overwriting them. This changes fixes that but I'm not sure if it causes problems to context function request injection in some cases

sorenlouv
sorenlouv commented Jul 5, 2024
View reviewed changes
x-pack/plugins/observability_solution/observability_ai_assistant/server/service/client/index.ts

// if a function request is at the very end, close the stream to consumer
// without persisting or updating the conversation. we need to wait
// on the function response to have a valid conversation
const isFunctionRequest = lastMessage.message.function_call?.name;
const isFunctionRequest = !!lastMessage?.message.function_call?.name;
Copy link
Member Author

Choose a reason for hiding this comment

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

cast to bool

sorenlouv
sorenlouv commented Jul 5, 2024
View reviewed changes
x-pack/plugins/observability_solution/observability_ai_assistant/server/service/client/index.ts
@@ -335,13 +335,12 @@ export class ObservabilityAIAssistantClient {
const initialMessagesWithAddedMessages =
messagesWithUpdatedSystemMessage.concat(addedMessages);

const lastMessage =
initialMessagesWithAddedMessages[initialMessagesWithAddedMessages.length - 1];
const lastMessage = last(initialMessagesWithAddedMessages);
Copy link
Member Author

Choose a reason for hiding this comment

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

Use utility for improved readability

@sorenlouv sorenlouv enabled auto-merge (squash) July 5, 2024 11:43
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 6, 2024 •
edited
Loading

⏳ Build in-progress

History

@sorenlouv sorenlouv merged commit 4504088 into elastic:main Jul 6, 2024
21 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 6, 2024
@sorenlouv
[Obs AI Assistant] Add ES function API test (elastic#187465)
1d1502e 
Related to elastic#180787

(cherry picked from commit 4504088)
@kibanamachine kibanamachine mentioned this pull request Jul 8, 2024
Merged
@kibanamachine kibanamachine mentioned this pull request Aug 5, 2024
Merged
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 27, 2024
@neptunian neptunian mentioned this pull request Sep 10, 2024
Closed
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

15 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@neptunian neptunian removed the v8.15.0 label Oct 22, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

5 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 187465 locally

@sorenlouv sorenlouv added the backport:skip This commit does not require backporting label Oct 31, 2024
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:Obs AI Assistant Observability AI Assistant v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sorenlouv @obltmachine @dgieselaar @elasticmachine @kibanamachine @neptunian