[typescript] Save output.data with text content instead of response data #603

rossdanlm · 2023-12-23T22:43:56Z

[typescript] Save output.data with text content instead of response data

This comes after Sarmad's schema updates in #589. To keep diffs small and easier to review, this simply converts from model-specific outputs --> pure text. I have a diff in #610 which converts from pure text --> OutputData format.

We only needed to update the hf.py and openai.py, because palm.py already returns output in the form of string | null type.

Ran yarn automated tests, but there aren't any specifically for openai. I also ran the typescript demos to make sure that they still work. Run these commands from aiconfig top-level dir:

npx ts-node typescript/demo/function-call-stream.ts
npx ts-node typescript/demo/demo.ts
npx ts-node typescript/demo/test-hf.ts

For the extensions, we only have typescript for hf.ts (trivial: just changed response to response.generated_text), while llama.ts already outputs it in text format so no changes needed

TODO

I still need to add function call support directly to OutputData format. See

saqadri · 2023-12-26T16:06:10Z

FYI to run demo.ts, simply run

ts-node demo.ts

in that folder. You may need to install ts-node if you don't already have it, from here: https://www.npmjs.com/package/ts-node#installation

extensions/HuggingFace/typescript/hf.ts

typescript/lib/parsers/openai.ts

rholinshead · 2023-12-26T16:14:16Z

Can you make sure to test the function calling example,

npx ts-node demo/function-call-stream.ts

I think it could be broken by the change to move it to metadata here?

Also, just curious why you chose to do this first and then move to the OutputData type in #610 instead of doing it in one go? I agree that using OutputData with 'string' kind is the correct way to go

saqadri

Requesting changes on the breaking changes introduced here.

extensions/HuggingFace/typescript/hf.ts

typescript/lib/parsers/hf.ts

typescript/lib/parsers/openai.ts

saqadri · 2023-12-26T18:12:42Z

I agree that using OutputData with 'string' kind is the correct way to go

@rholinshead is that significantly different on the frontend? It would be a much simpler JSON/YAML file for string outputs if the "string" kind was inferred if the typeof output.data was string.

rossdanlm · 2023-12-26T21:02:46Z

Can you make sure to test the function calling example,
npx ts-node demo/function-call-stream.ts
I think it could be broken by the change to move it to metadata here?

Also, just curious why you chose to do this first and then move to the OutputData type in #610 instead of doing it in one go? I agree that using OutputData with 'string' kind is the correct way to go

cc thanks @rholinshead for flagging, you are totally correct this breaks because of the way we implement it in places like this, but moving forwards this isn't something we should continue to support and we should use the function call directly instead from OutputData or metadata

@saqadri I know this breaks existing code API implementation that may be calling into this, but we can't both choose to support old formats while also wanting to make our output types easy to parse and understand. The getOutputText() methods are still supported, it's just if anyone is engaging with the old data structure their implementation will break

saqadri · 2023-12-26T21:07:18Z

Can you make sure to test the function calling example,
npx ts-node demo/function-call-stream.ts
I think it could be broken by the change to move it to metadata here?

Also, just curious why you chose to do this first and then move to the OutputData type in #610 instead of doing it in one go? I agree that using OutputData with 'string' kind is the correct way to go

cc thanks @rholinshead for flagging, you are totally correct this breaks because of the way we implement it in places like this, but moving forwards this isn't something we should continue to support and we should use the function call directly instead from OutputData or metadata

@saqadri I know this breaks existing code API implementation that may be calling into this, but we can't both choose to support old formats while also wanting to make our output types easy to parse and understand. The getOutputText() methods are still supported, it's just if anyone is engaging with the old data structure their implementation will break

Can you share how the function call script should be updated to support the new format? One other option can be adding output kind "function" to the schema to it's distinguished from plain string.

We cannot have breaking changes without explicitly mentioning it and having a clear major version bump. The issue is there are several implicit breaking changes introduced in this stack

rossdanlm · 2023-12-26T21:16:37Z

Can you share how the function call script should be updated to support the new format? One other option can be adding output kind "function" to the schema to it's distinguished from plain string.

Saw your comments, I didn't have time to update the diff summary before you replied but just did. Basically we need to do this:

I updated the change to the function-call-stream.ts file

We cannot have breaking changes without explicitly mentioning it and having a clear major version bump. The issue is there are several implicit breaking changes introduced in this stack

Alright, I'm in favour of doing a clear major version (I haven't posted changelog yet for this reason). Let's also include in this NOT to support arbitrary JSON in OutputData schema.

rossdanlm · 2023-12-26T21:36:36Z

Updated based on feedback, main changes:

sending rawResponse as a field in metadata for all non-streaming options
no longer using delta instead of accumulatedData, created task for it: [ts] HuggingFace extension uses delta instead of accumulatedText in output.data #620
Made note to support function calling more directly as part of OutputData. I will do this end of diff stack
Supported all old AIConfig schemas with backwards compatbility inside of the getOutputText() methods

Note that this change will introduce breaking the SDK implementation. Ex: instead of check for data.content you now just check data instead. I will land changelog for today before landing these changes, so this won't be included or mentioned in that

rholinshead · 2023-12-26T21:58:01Z

I agree that using OutputData with 'string' kind is the correct way to go

@rholinshead is that significantly different on the frontend? It would be a much simpler JSON/YAML file for string outputs if the "string" kind was inferred if the typeof output.data was string.

It isn't significantly different on the front end, it's just checking if the type is a string or an object first instead of just checking the keys of the object. In general, was just thinking it is a much cleaner API to get rid of the redundant union types (i.e. can represent string output in 2 supported ways) that don't have a clear standard or recommendation.

If it's a lot nicer for the JSON/YAML format then maybe it's worth supporting raw string there, we should just consider:

what is our recommendation / standard for how we think it should be used? All our example configs should be consistent
do we even want to consider {kind: 'string'} then, or just remove it to have the single way (raw text as the data value for text outputs)

rholinshead · 2023-12-26T22:09:50Z

Let's also include in this NOT to support arbitrary JSON in OutputData schema.

Based on previous PR discussion with @saqadri I think we should still support arbitrary JSON for now and just provide the new structured types. We can revisit later to make a better informed decision with more data (e.g. once we see more examples of user-created parsers to know how they're using it)

rholinshead · 2023-12-26T22:17:26Z

typescript/demo/function-call-stream.ts

-    const message = output.data as ChatCompletionMessageParam;
+    const rawResponse = output.metadata?.rawResponse;
+    const function_call = rawResponse?.function_call;
+    console.log("function_call=", function_call);


nit, can remove

I wanted to keep it because we have other console.logs() earlier and it shows more clearly in the demo how function_call is being used. Ex: https://github.com/lastmile-ai/aiconfig/blob/main/typescript/demo/function-call-stream.ts#L176

rholinshead · 2023-12-26T22:38:57Z

typescript/lib/parsers/openai.ts

        // If the prompt has output saved, add it to the messages array
-        if (outputMessage.role === "assistant") {
-          messages.push(outputMessage);
+        if (output.metadata?.role === "assistant") {


I think we still need to handle the existing outputMessage-as-ChatCompletionMessageParam case here for backwards compatibility on deserialize. We can reserialize in our updated format, but it's possible the config being loaded has the old format

I think you can do similar check to above

if (typeof output.data === "string") { ... handle how you do here } else if (output.data.hasOwnProperty("role")) { // for backwards compatibility const outputMessage = output.data as unknown as Chat.ChatCompletionMessageParam; if (outputMessage.role === "assistant") { messages.push(outputMessage); } }

You are correct, thank you for catching!

saqadri · 2023-12-27T00:20:15Z

typescript/demo/function-call-stream.ts

+    const rawResponse = output.metadata?.rawResponse;
+    const function_call = rawResponse?.function_call;


I don't think this is a good model. If the only way for someone to support function calling is by delving into the raw response, then we've missed a critical scenario.

Ok after rebasing onto #628, I will update but do this at the end of the diff stack. I just want to keep this for string-only to make it easier to review, then the rest of diffs will be easier to followup

saqadri

Approving to unblock :). We are aligned on the acceptable breaking changes to the SDK while still supporting previous aiconfig schemas.

Also, as you land this stack please document the breaking changes explicitly in our changelog doc for next week so we can communicate it explicitly

saqadri · 2023-12-27T01:12:23Z

typescript/package-lock.json

Delete this

This comes after Sarmad's schema updates in #589. To keep diffs small and easier to review, this simply converts from model-specific outputs --> pure text. I have a diff in #610 which converts from pure text --> `OutputData` format. We only needed to update the `hf.py` and `openai.py`, because `palm.py` already returns output in the form of `string | null` type. Ran yarn automated tests, but there aren't any specifically for openai. I also ran the typescript demos to make sure that they still work. Run these commands from `aiconfig` top-level dir: ``` npx ts-node typescript/demo/function-call-stream.ts npx ts-node typescript/demo/demo.ts npx ts-node typescript/demo/test-hf.ts ``` For the extensions, we only have typescript for `hf.ts` (trivial: just changed `response` to `response.generated_text`), while `llama.ts` already outputs it in text format so no changes needed ## TODO I still need to add function call support directly to `OutputData` format. See

rossdanlm changed the title ~~todo later, text types~~ [typescript] Save output.data with text content instead of response data Dec 24, 2023

rossdanlm force-pushed the pr603 branch 2 times, most recently from c9c2e87 to af41545 Compare December 24, 2023 05:27

This was referenced Dec 24, 2023

[python][non-image]Save output.data with text content instead of response data #605

Merged

[python][image] Save output.data images in OutputData format, not just strings #608

Merged

rossdanlm marked this pull request as ready for review December 25, 2023 06:45

rossdanlm requested review from saqadri, rholinshead, suyoglastmileai, Ankush-lastmile and jonathanlastmileai as code owners December 25, 2023 06:45

rossdanlm mentioned this pull request Dec 25, 2023

[typescript] Use OutputDataWithValue type for function calls #610

Merged

rossdanlm force-pushed the pr603 branch from af41545 to 8e3a0be Compare December 25, 2023 22:16

rossdanlm mentioned this pull request Dec 25, 2023

[python] Use OutputDataWithValue type for function calls #611

Merged

rholinshead reviewed Dec 26, 2023

View reviewed changes

extensions/HuggingFace/typescript/hf.ts Outdated Show resolved Hide resolved

extensions/HuggingFace/typescript/hf.ts Outdated Show resolved Hide resolved

typescript/lib/parsers/openai.ts Outdated Show resolved Hide resolved

saqadri requested changes Dec 26, 2023

View reviewed changes

rossdanlm mentioned this pull request Dec 26, 2023

[ts] HuggingFace extension uses delta instead of accumulatedText in output.data #620

Open

rossdanlm force-pushed the pr603 branch from 8e3a0be to 3b08d0e Compare December 26, 2023 21:03

rossdanlm force-pushed the pr603 branch 3 times, most recently from 729d773 to 5878f96 Compare December 26, 2023 21:14

rossdanlm force-pushed the pr603 branch from 5878f96 to 15c2e66 Compare December 26, 2023 21:30

rossdanlm force-pushed the pr603 branch from 15c2e66 to 5aa7a2c Compare December 26, 2023 21:47

rholinshead reviewed Dec 26, 2023

View reviewed changes

saqadri reviewed Dec 27, 2023

View reviewed changes

saqadri approved these changes Dec 27, 2023

View reviewed changes

saqadri reviewed Dec 27, 2023

View reviewed changes

typescript/package-lock.json Outdated

Copy link

Contributor

saqadri Dec 27, 2023

Choose a reason for hiding this comment

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

Delete this

rossdanlm reacted with thumbs up emoji

rossdanlm force-pushed the pr603 branch 2 times, most recently from 5f2a6d1 to 8a0218f Compare December 27, 2023 01:20

rossdanlm force-pushed the pr603 branch from 8a0218f to 01ccc33 Compare December 27, 2023 01:26

rossdanlm merged commit a1e1378 into main Dec 27, 2023
2 checks passed

rossdanlm deleted the pr603 branch December 27, 2023 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript] Save output.data with text content instead of response data #603

[typescript] Save output.data with text content instead of response data #603

rossdanlm commented Dec 23, 2023 •

edited

Loading

saqadri commented Dec 26, 2023

rholinshead commented Dec 26, 2023

saqadri left a comment

saqadri commented Dec 26, 2023

rossdanlm commented Dec 26, 2023

saqadri commented Dec 26, 2023

rossdanlm commented Dec 26, 2023 •

edited

Loading

rossdanlm commented Dec 26, 2023

rholinshead commented Dec 26, 2023 •

edited

Loading

rholinshead commented Dec 26, 2023

rholinshead Dec 26, 2023

rossdanlm Dec 27, 2023

rholinshead Dec 26, 2023

rossdanlm Dec 27, 2023

saqadri Dec 27, 2023

rossdanlm Dec 27, 2023

saqadri left a comment

saqadri Dec 27, 2023

		const rawResponse = output.metadata?.rawResponse;
		const function_call = rawResponse?.function_call;

[typescript] Save output.data with text content instead of response data #603

[typescript] Save output.data with text content instead of response data #603

Conversation

rossdanlm commented Dec 23, 2023 • edited Loading

TODO

saqadri commented Dec 26, 2023

rholinshead commented Dec 26, 2023

saqadri left a comment

Choose a reason for hiding this comment

saqadri commented Dec 26, 2023

rossdanlm commented Dec 26, 2023

saqadri commented Dec 26, 2023

rossdanlm commented Dec 26, 2023 • edited Loading

rossdanlm commented Dec 26, 2023

rholinshead commented Dec 26, 2023 • edited Loading

rholinshead commented Dec 26, 2023

rholinshead Dec 26, 2023

Choose a reason for hiding this comment

rossdanlm Dec 27, 2023

Choose a reason for hiding this comment

rholinshead Dec 26, 2023

Choose a reason for hiding this comment

rossdanlm Dec 27, 2023

Choose a reason for hiding this comment

saqadri Dec 27, 2023

Choose a reason for hiding this comment

rossdanlm Dec 27, 2023

Choose a reason for hiding this comment

saqadri left a comment

Choose a reason for hiding this comment

saqadri Dec 27, 2023

Choose a reason for hiding this comment

rossdanlm commented Dec 23, 2023 •

edited

Loading

rossdanlm commented Dec 26, 2023 •

edited

Loading

rholinshead commented Dec 26, 2023 •

edited

Loading