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

Remove inference_id field if no inference endpoint is selected #205660

Conversation

Samiul-TheSoccerFan
Copy link
Contributor

@Samiul-TheSoccerFan Samiul-TheSoccerFan commented Jan 6, 2025

Summary

Currently, the semantic_text field supports a default inference_id, meaning users are not required to explicitly select an inference endpoint during mapping. However, a bug has been identified: if the Select inference Id popover is not opened, the inference_id field property remains as an empty string. This causes Elasticsearch (ES) to throw an error, as it requires a value to be present if the property is defined.

To address this issue, the proposed solution is to remove the inference_id property from the semantic_text field during field mapping if its value is empty.

Screen Recording

default-elser.mov

@Samiul-TheSoccerFan Samiul-TheSoccerFan added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:version Backport to applied version labels v8.18.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project labels Jan 7, 2025
@Samiul-TheSoccerFan Samiul-TheSoccerFan force-pushed the fix-default-inference-endpoint branch from 0361868 to 6c02702 Compare January 7, 2025 14:08
@Samiul-TheSoccerFan Samiul-TheSoccerFan marked this pull request as ready for review January 7, 2025 14:08
@Samiul-TheSoccerFan Samiul-TheSoccerFan requested a review from a team as a code owner January 7, 2025 14:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

@mattkime
Copy link
Contributor

mattkime commented Jan 9, 2025

/ci

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

This could really use a test, otherwise the changes look good

@@ -130,8 +131,9 @@ export const CreateField = React.memo(function CreateFieldComponent({
const { isValid, data } = await form.submit();

if (isValid && !clickOutside) {
if (isSemanticTextField(data)) {
handleSemanticText(data);
if (isSemanticTextField(data) && isEmpty(data.inference_id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have unit tests around this assemblage of content.

Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

couple nitpicks but otherwise LGTM

@@ -237,6 +238,7 @@ export const DetailsPageMappingsContent: FunctionComponent<{
.map((field) => field.inference_id)
.filter(
(inferenceId: string) =>
!isEmpty(inferenceId) &&
Copy link
Member

Choose a reason for hiding this comment

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

You can just do !inferenceId, no need to import a lodash function that checks way more than is necessary.

@@ -130,8 +131,9 @@ export const CreateField = React.memo(function CreateFieldComponent({
const { isValid, data } = await form.submit();

if (isValid && !clickOutside) {
if (isSemanticTextField(data)) {
handleSemanticText(data);
if (isSemanticTextField(data) && isEmpty(data.inference_id)) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for isEmpty, you can just do !data.inference_id

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

changes lgtm

@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 20, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
indexManagement 734.8KB 734.9KB +58.0B

History

@Samiul-TheSoccerFan Samiul-TheSoccerFan merged commit c8e0408 into elastic:main Jan 20, 2025
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12868385896

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 20, 2025
…ic#205660)

## Summary

Currently, the `semantic_text` field supports a default `inference_id`,
meaning users are not required to explicitly select an inference
endpoint during mapping. However, a bug has been identified: if the
`Select inference Id` popover is not opened, the `inference_id` field
property remains as an empty string. This causes Elasticsearch (ES) to
throw an error, as it requires a value to be present if the property is
defined.

To address this issue, the proposed solution is to remove the
`inference_id` property from the `semantic_text` field during field
mapping if its value is empty.

### Screen Recording

https://github.com/user-attachments/assets/e8d8d471-7ff2-493e-8872-e42838579d44

---------

Co-authored-by: Matthew Kime <[email protected]>
(cherry picked from commit c8e0408)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 20, 2025
…205660) (#207189)

# Backport

This will backport the following commits from `main` to `8.x`:
- [Remove inference_id field if no inference endpoint is selected
(#205660)](#205660)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Samiul
Monir","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-20T12:49:21Z","message":"Remove
inference_id field if no inference endpoint is selected (#205660)\n\n##
Summary\r\n\r\nCurrently, the `semantic_text` field supports a default
`inference_id`,\r\nmeaning users are not required to explicitly select
an inference\r\nendpoint during mapping. However, a bug has been
identified: if the\r\n`Select inference Id` popover is not opened, the
`inference_id` field\r\nproperty remains as an empty string. This causes
Elasticsearch (ES) to\r\nthrow an error, as it requires a value to be
present if the property is\r\ndefined.\r\n\r\nTo address this issue, the
proposed solution is to remove the\r\n`inference_id` property from the
`semantic_text` field during field\r\nmapping if its value is
empty.\r\n\r\n### Screen
Recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e8d8d471-7ff2-493e-8872-e42838579d44\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthew Kime
<[email protected]>","sha":"c8e0408e71e4bdfde59a083833f73b4ef1eb6407","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Kibana
Management","release_note:skip","v9.0.0","ci:project-deploy-elasticsearch","backport:version","v8.18.0"],"title":"Remove
inference_id field if no inference endpoint is
selected","number":205660,"url":"https://github.com/elastic/kibana/pull/205660","mergeCommit":{"message":"Remove
inference_id field if no inference endpoint is selected (#205660)\n\n##
Summary\r\n\r\nCurrently, the `semantic_text` field supports a default
`inference_id`,\r\nmeaning users are not required to explicitly select
an inference\r\nendpoint during mapping. However, a bug has been
identified: if the\r\n`Select inference Id` popover is not opened, the
`inference_id` field\r\nproperty remains as an empty string. This causes
Elasticsearch (ES) to\r\nthrow an error, as it requires a value to be
present if the property is\r\ndefined.\r\n\r\nTo address this issue, the
proposed solution is to remove the\r\n`inference_id` property from the
`semantic_text` field during field\r\nmapping if its value is
empty.\r\n\r\n### Screen
Recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e8d8d471-7ff2-493e-8872-e42838579d44\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthew Kime
<[email protected]>","sha":"c8e0408e71e4bdfde59a083833f73b4ef1eb6407"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/205660","number":205660,"mergeCommit":{"message":"Remove
inference_id field if no inference endpoint is selected (#205660)\n\n##
Summary\r\n\r\nCurrently, the `semantic_text` field supports a default
`inference_id`,\r\nmeaning users are not required to explicitly select
an inference\r\nendpoint during mapping. However, a bug has been
identified: if the\r\n`Select inference Id` popover is not opened, the
`inference_id` field\r\nproperty remains as an empty string. This causes
Elasticsearch (ES) to\r\nthrow an error, as it requires a value to be
present if the property is\r\ndefined.\r\n\r\nTo address this issue, the
proposed solution is to remove the\r\n`inference_id` property from the
`semantic_text` field during field\r\nmapping if its value is
empty.\r\n\r\n### Screen
Recording\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e8d8d471-7ff2-493e-8872-e42838579d44\r\n\r\n---------\r\n\r\nCo-authored-by:
Matthew Kime
<[email protected]>","sha":"c8e0408e71e4bdfde59a083833f73b4ef1eb6407"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Samiul Monir <[email protected]>
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
…ic#205660)

## Summary

Currently, the `semantic_text` field supports a default `inference_id`,
meaning users are not required to explicitly select an inference
endpoint during mapping. However, a bug has been identified: if the
`Select inference Id` popover is not opened, the `inference_id` field
property remains as an empty string. This causes Elasticsearch (ES) to
throw an error, as it requires a value to be present if the property is
defined.

To address this issue, the proposed solution is to remove the
`inference_id` property from the `semantic_text` field during field
mapping if its value is empty.

### Screen Recording


https://github.com/user-attachments/assets/e8d8d471-7ff2-493e-8872-e42838579d44

---------

Co-authored-by: Matthew Kime <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels ci:project-deploy-elasticsearch Create an Elasticsearch Serverless project release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants