-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security GenAI] Fix and un-skip Knowledge Base Integration Tests #198558
Conversation
const setupAvailable = await kbDataClient.isSetupAvailable(); | ||
const isModelDeployed = await kbDataClient.isModelDeployed(); | ||
|
||
const body: ReadKnowledgeBaseResponse = { | ||
elser_exists: modelExists, | ||
elser_exists: isModelDeployed, |
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 use elser_exists
to see the KB readiness and is model deployed
represents that more accurate than is model installed
state.
Pinging @elastic/security-solution (Team: SecuritySolution) |
await waitFor( | ||
async () => { | ||
const status = await getKnowledgeBaseStatus(supertest, log); | ||
return !!status.elser_exists; |
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.
Here we wait for the KB to be ready to receive entry API calls.
before(async () => { | ||
await installTinyElser(ml); | ||
await setupKnowledgeBase(supertest, log); | ||
setupKnowledgeBase(supertest, log); | ||
await waitForKnowledgeBaseModelToBeDeployed(supertest, log); |
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.
Does waiting for the knowledge base setup take up to 10 minutes? If so, I think we should find a way to not install all of the security docs for this test. Perhaps there is a way to use the test framework to mock the directory where the documents are installed from, and only install one document?
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.
Yeah, that will be a good option as well. I will explore this.
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.
By the way, below this there is an afterEach
with clearKnowledgeBase
that deletes all of the knowledge base documents, so it is extra silly to wait 10 minutes to install them. And setupKnowledgeBase
only happens once
Closing in favour of #198861 |
💔 Build Failed
Failed CI StepsMetrics [docs]
History
cc @e40pud |
Summary
This is a followup to #198178 where we skipped KB integration tests. We enable it with this PR.
Since it takes a lot of time to setup all Security Labs docs, the idea is to call the setup KB route without waiting it to finish the whole job. Instead we would monitor the KB status and wait for inference endpoint (model) to be deployed. For that we monitor
elser_exists
status attribute which I updated to holdis model deployed
instead ofis model installed
state. confirmed with @patrykkopycinski that that is a more accurate representation of the KB readiness.