[Inference] create the `@kbn/inference-common` package #193464

pgayvallet · 2024-09-19T18:28:33Z

Summary

At the moment, all inference API related types and utilities (chatComplete, output and more) are living inside the inference plugin's common folder.

This is somewhat problematic because it forces any consumers of those types to explicitly depends on the inference plugin (via plugin dep or ts ref), which could lead to any kind of cyclic dependency issues, in addition to being overall a bad design pattern.

This also makes it more complicated that it should to try to split the inference logic / task framework / task implementation into distinct packages or plugins, due to some (concrete) utilities living in the inference plugin's code.

It's also a bad experience for consumers, as it's quite difficult to easily resolve imports they need (we're mixing internal and public exports atm, plus not all types are exported from a single entry point, making it very tedious to find the right path for each individual import we need to consume the inference APIs)

This PR addresses most of those points, by introducing a new @kbn/inference-common package and moving all the low level types and utilities to it, while exposing all of them from the package's entrypoint.

…workflow-1

pgayvallet · 2024-10-30T08:23:32Z

/ci

pgayvallet · 2024-10-30T08:24:47Z

/ci

pgayvallet · 2024-10-30T08:40:42Z

/ci

pgayvallet · 2024-10-30T09:18:41Z

/ci

pgayvallet · 2024-10-30T09:31:54Z

/ci

pgayvallet · 2024-10-30T11:08:52Z

/ci

pgayvallet · 2024-10-30T11:56:22Z

/ci

…fix'

pgayvallet · 2024-10-30T12:17:35Z

/ci

…fix'

pgayvallet · 2024-10-30T13:02:07Z

/ci

legrego

I left a few minor comments. Additionally, I'd like to see us improve the docs for our exposed types in a followup.

legrego · 2024-10-30T18:15:57Z

x-pack/packages/ai-infra/inference-common/src/errors.ts

-  message: string = i18n.translate('xpack.inference.internalError', {
-    defaultMessage: 'An internal error occurred',
-  }),
+  message = 'An internal error occurred',


question: why did we un-internationalize this string?

We never i18n-ize technical errors. ES doesn't, Kibana doesn't.

legrego · 2024-10-30T18:20:24Z

x-pack/plugins/inference/common/http_apis.ts

@@ -15,3 +15,7 @@ export type ChatCompleteRequestBody = {
  messages: Message[];
  functionCalling?: FunctionCallingMode;
 } & ToolOptions;
+
+export interface GetConnectorsResponseBody {


Why are these types not extracted to the inference-common package? Is it because they aren't used outside of the inference plugin?

Correct, those are the types for the response payloads of the http APIs, it's only used internally by the plugin.

legrego · 2024-10-30T18:23:16Z

x-pack/plugins/inference/server/chat_complete/errors.ts

+export function createToolNotFoundError(name: string): ChatCompletionToolNotFoundError {
+  return new InferenceTaskError(
+    ChatCompletionErrorCode.ToolNotFoundError,
+    `Tool ${name} called but was not available`,


Does this need i18n?

Same as #193464 (comment) - I'd say no (fwiw this was only moved, not introduced by the PR)

legrego · 2024-10-30T18:28:16Z

x-pack/plugins/inference/server/tasks/nl_to_esql/actions/generate_esql.ts


-export const generateEsqlTask = <TToolOptions extends ToolOptions>({
-  chatCompleteApi,
+export const generateEsqlTaskFn = <TToolOptions extends ToolOptions>({


The changes in the nl_to_esql directory appear to be more substantial than (& unrelated to?) type/utility extraction. Can you help me understand the context for these changes?

Oh right, I realized I did not mention anything on that part, sorry...

There are two things:

I refactored the flow of the task to use a shared output observable instead of chaining observable calls (which was a nightmare to follow and to evolve, it's way easier to add steps and flow now)

I introduced a summarizing pre-step, to see the effect on generation efficiency, but the step is disabled by default and there's no way to enable it from production usages atm (it's only for testing via evaluation)
(3. I apparently also tweaked a few prompts, from evaluation tuning I did a while ago - the PR root is old)

That refactor is definitely something I want to merge, but tell me if we're not comfortable mixings things in that PR, I can extract those changes elsewhere.

if it's not too much, maybe it's good to pull the changes to the ES|QL task out and into a separate PR, and then we can run the evaluation suites on both sides against it?

Agreed. I actually forgot I used that other PR as base for those changes... I will revert that part from there

Note for future me: kbx-xxx-nl-to-esql-refactor-2

legrego · 2024-10-30T18:31:47Z

x-pack/plugins/inference/server/tasks/nl_to_esql/utils/validate_query_ast.ts

+ * Run the query through the kibana
+ * @param query
+ */
+export const validateQueryAst = async (query: string): Promise<QuerySyntaxError[]> => {


Perhaps out of scope or premature, but this feels like something that should exist within an es|ql package.

Yeah, I want to take a closer look at the ESQL task later, as I'd like to even extract it from the inference plugin (this is a task, not part of the inference plugin/framework), but I was planning on doing it on a distinct PR as it's still not totally clear to me how exactly we should restructure.

(but fwiw, not sure it would benefit from living into an esql package rather than with the task esql, the function already mostly depend on low-level ESQL directives)

legrego · 2024-10-30T18:35:50Z

x-pack/plugins/inference/server/util/validate_tool_calls.test.ts

-import { isToolValidationError } from '../../common/chat_complete/errors';
-import { ToolChoiceType } from '../../common/chat_complete/tools';
+import { ToolChoiceType } from '@kbn/inference-common';
+import { isToolValidationError } from '@kbn/inference-common/src/chat_complete/errors';


nit: should we be reaching this far into the inference-common package for the import?

pgayvallet · 2024-10-31T07:35:45Z

@legrego

I'd like to see us improve the docs for our exposed types in a followup

Agreed, we absolutely need to do that

…workflow-1

pgayvallet · 2024-11-01T07:00:11Z

@elasticmachine merge upstream

elasticmachine · 2024-11-01T08:03:40Z

⏳ Build in-progress

Buildkite Build
Commit: 8ec8ecb
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193464-8ec8ecbe464f

History

💚 Build #247474 succeeded 798a60b
💚 Build #247419 succeeded 08d1e30
💔 Build #247360 failed 7aff5bf
💚 Build #247324 succeeded 7904a14
💚 Build #247013 succeeded 71c0d75

kibanamachine · 2024-11-01T09:09:02Z

Starting backport for target branches: 8.x

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

kibanamachine · 2024-11-01T09:13:34Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 193464

Questions ?

Please refer to the Backport tool documentation

## Summary At the moment, all inference API related types and utilities (`chatComplete`, `output` and more) are living inside the `inference` plugin's common folder. This is somewhat problematic because it forces any consumers of those types to explicitly depends on the `inference` plugin (via plugin dep or ts ref), which could lead to any kind of cyclic dependency issues, in addition to being overall a bad design pattern. This also makes it more complicated that it should to try to split the inference logic / task framework / task implementation into distinct packages or plugins, due to some (concrete) utilities living in the inference plugin's code. It's also a bad experience for consumers, as it's quite difficult to easily resolve imports they need (we're mixing internal and public exports atm, plus not all types are exported from a single entry point, making it very tedious to find the right path for each individual import we need to consume the inference APIs) This PR addresses most of those points, by introducing a new `@kbn/inference-common` package and moving all the low level types and utilities to it, while exposing all of them from the package's entrypoint. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

kibanamachine · 2024-11-04T09:48:49Z

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 193464 locally

kibanamachine · 2024-11-05T09:49:02Z

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 193464 locally

## Summary At the moment, all inference API related types and utilities (`chatComplete`, `output` and more) are living inside the `inference` plugin's common folder. This is somewhat problematic because it forces any consumers of those types to explicitly depends on the `inference` plugin (via plugin dep or ts ref), which could lead to any kind of cyclic dependency issues, in addition to being overall a bad design pattern. This also makes it more complicated that it should to try to split the inference logic / task framework / task implementation into distinct packages or plugins, due to some (concrete) utilities living in the inference plugin's code. It's also a bad experience for consumers, as it's quite difficult to easily resolve imports they need (we're mixing internal and public exports atm, plus not all types are exported from a single entry point, making it very tedious to find the right path for each individual import we need to consume the inference APIs) This PR addresses most of those points, by introducing a new `@kbn/inference-common` package and moving all the low level types and utilities to it, while exposing all of them from the package's entrypoint. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 631ccb0) # Conflicts: # .github/CODEOWNERS

#199002) # Backport This will backport the following commits from `main` to `8.x`: - [Inference] create the `@kbn/inference-common` package (#193464) (631ccb0)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

Work in progress

0502292

pgayvallet added the Team:AI Infra AppEx AI Infrastructure Team label Sep 19, 2024

pgayvallet added 4 commits October 29, 2024 09:37

Merge remote-tracking branch 'upstream/main' into kbn-xxx-nl-to-esql-…

156c6d1

…workflow-1

remove additional things

71dfeb2

prepare for type extraction

1748dce

move types to @kbn/inference-common

ffd9db5

pgayvallet changed the title ~~[NL-to-ESQL] Improve workflow~~ [Inference] create the @kbn/inference-common package Oct 30, 2024

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.17.0 labels Oct 30, 2024

fix imports, first batch

3e2173e

add kbn reference

4607c49

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

68f3e72

[CI] Auto-commit changed files from 'node scripts/notice'

7d5ec25

why are you even importing that from there?

f354cfe

more import fixes

35ec204

more type fix

512ec1d

kibanamachine and others added 2 commits October 30, 2024 12:07

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

ac2aedc

…fix'

last type fixes

7387e47

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

71c0d75

…fix'

pgayvallet marked this pull request as ready for review October 30, 2024 15:11

patrykkopycinski approved these changes Oct 30, 2024

View reviewed changes

shahzad31 approved these changes Oct 30, 2024

View reviewed changes

legrego reviewed Oct 30, 2024

View reviewed changes

pgayvallet added 2 commits October 31, 2024 08:34

extract option type

4e28e14

export chat complete errors from entry point

7904a14

Merge remote-tracking branch 'upstream/main' into kbn-xxx-nl-to-esql-…

7aff5bf

…workflow-1

pgayvallet requested a review from legrego October 31, 2024 10:08

pgayvallet and others added 5 commits October 31, 2024 13:21

revert changes to ESQL task

c14b4f6

fix imports again due to revert

f194638

[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'

08d1e30

Add TSdoc for most public types

9d858a2

[CI] Auto-commit changed files from 'node scripts/notice'

798a60b

legrego approved these changes Oct 31, 2024

View reviewed changes

Merge branch 'main' into kbn-xxx-nl-to-esql-workflow-1

8ec8ecb

pgayvallet enabled auto-merge (squash) November 1, 2024 07:00

pgayvallet merged commit 631ccb0 into elastic:main Nov 1, 2024
50 checks passed

kibanamachine added the v9.0.0 label Nov 1, 2024

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 4, 2024

pgayvallet mentioned this pull request Nov 5, 2024

[8.x] [Inference] create the @kbn/inference-common package (#193464) #199002

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Inference] create the `@kbn/inference-common` package #193464

[Inference] create the `@kbn/inference-common` package #193464

pgayvallet commented Sep 19, 2024 •

edited by kibanamachine

Loading

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

legrego left a comment

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

dgieselaar Oct 31, 2024

pgayvallet Oct 31, 2024 •

edited

Loading

pgayvallet Oct 31, 2024

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

legrego Oct 30, 2024

pgayvallet Oct 31, 2024

pgayvallet commented Oct 31, 2024

pgayvallet commented Nov 1, 2024

elasticmachine commented Nov 1, 2024

kibanamachine commented Nov 1, 2024

kibanamachine commented Nov 1, 2024

kibanamachine commented Nov 4, 2024

kibanamachine commented Nov 5, 2024

[Inference] create the @kbn/inference-common package #193464

[Inference] create the @kbn/inference-common package #193464

Conversation

pgayvallet commented Sep 19, 2024 • edited by kibanamachine Loading

Summary

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

pgayvallet commented Oct 30, 2024

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Oct 31, 2024

pgayvallet commented Nov 1, 2024

elasticmachine commented Nov 1, 2024

⏳ Build in-progress

History

kibanamachine commented Nov 1, 2024

kibanamachine commented Nov 1, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Nov 4, 2024

kibanamachine commented Nov 5, 2024

[Inference] create the `@kbn/inference-common` package #193464

[Inference] create the `@kbn/inference-common` package #193464

pgayvallet commented Sep 19, 2024 •

edited by kibanamachine

Loading

pgayvallet Oct 31, 2024 •

edited

Loading