Feat/messages api #546

urjitbhatia · 2023-11-07T20:35:49Z

This branch implements thread messages API

Sibling to the threads api implementation #536

codecov · 2023-11-08T17:22:30Z

Codecov Report

Merging #546 (e152f43) into master (d6f3bdc) will increase coverage by 0.09%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   98.22%   98.31%   +0.09%     
==========================================
  Files          22       23       +1     
  Lines        1181     1247      +66     
==========================================
+ Hits         1160     1226      +66     
  Misses         15       15              
  Partials        6        6

Files	Coverage Δ
messages.go	`100.00% <100.00%> (ø)`

- go map access is not deterministic - this can lead to a route: /foo/bar/1 matching /foo/bar before matching /foo/bar/1 if the map iteration go through /foo/bar first since the regex match wasn't bound to start and end anchors - registering handlers now converts * in routes to .* for proper regex matching - test server route handling now tries to fully match the handler route

henomis · 2023-11-09T06:56:03Z

Hi, to cover your tests you need to add entries in TestCases in client_test.go

messages.go

henomis · 2023-11-09T09:03:27Z

messages.go

+
+type MessageContent struct {
+	Type string      `json:"type"`
+	Text MessageText `json:"text"`


Suggested change

Text MessageText `json:"text"`

Text *MessageText `json:"text,omitempty"`

ImageFile *ImageFile `json:"image_file,omitempty"` // <- define this one

wti806 · 2023-11-10T15:40:30Z

May I ask when the message API would be ready?

urjitbhatia · 2023-11-10T21:05:39Z

Thanks, I'll work on the fixes this weekend.

…

On Fri, Nov 10, 2023, 9:40 AM Ti Wang ***@***.***> wrote: May I ask when the message API would be ready? — Reply to this email directly, view it on GitHub <#546 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJLOR344QSLRTGXNR2EFS3YDZDHTAVCNFSM6AAAAAA7BZOV7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBVHE3DANRUGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

urjitbhatia · 2023-11-11T23:52:21Z

TestCases

Adding the test cases...

However, when looking at these tests - It makes sense to push for a high coverage but some of the work done to keep it high seems forced. For example, covering the branch when the API returns an error.

I'd suggest rethinking the requirement a little bit - it makes sense to cover the main logic but this test seem to add very little or no value - https://github.com/sashabaranov/go-openai/blob/master/client_test.go#L184 - am I missing something in the value of this test? It doesn't actually test that the request building failed due to bad/missing params, only that an if err was added to the calls - lmk if that's the intended use anyways.

@henomis @sashabaranov

Co-authored-by: Simone Vellei <[email protected]>

sashabaranov · 2023-11-12T12:16:44Z

messages.go

+}
+type MessageText struct {
+	Value       string        `json:"value"`
+	Annotations []interface{} `json:"annotations"`


let's use any instead of interface{}

sashabaranov · 2023-11-12T12:21:27Z

messages.go

+		encodedValues = "?" + urlValues.Encode()
+	}
+
+	urlSuffix := fmt.Sprintf("/threads/%s%s%s", threadID, messagesSuffix, encodedValues)


Just a suggestion: changing messagesSuffix from /messages to messages would make formatting more consistent without %s%s mixed with %s/%s

sashabaranov · 2023-11-12T12:54:35Z

@urjitbhatia thank you so much for the PR! A couple of minor additions and let's merge it 🙌🏻

However, when looking at these tests - It makes sense to push for a high coverage but some of the work done to keep it high seems forced. For example, covering the branch when the API returns an error.
I'd suggest rethinking the requirement a little bit - it makes sense to cover the main logic but this test seem to add very little or no value - https://github.com/sashabaranov/go-openai/blob/master/client_test.go#L184 - am I missing something in the value of this test?

You know, when I started working on this library a couple of years earlier, I would agree with you. I didn't see any point in extensive testing except for integration tests with the actual API to check that the client actually works.

Now, I have come to realise a couple of things about testing:

It's easy to test the "happy path", but to test error cases, you need to design the code with clear interfaces and separation of concerns (RequestBuilder and ErrorAccumulator are good examples of that)
You are either forcing code coverage or dealing with the code base with a constant decline of code coverage sometime in the future. I would prefer the former, especially for such a widely-used library
~100% code coverage makes it simpler to accept changes into the codebase — it's just easier to trust the code that way

I think the requirement of copy-pasting 3 lines of tests per client method is a reasonable requirement for all these benefits.

So the way forward is to actually reach 100% coverage, add more randomisation(see #491), fuzzing, and integration tests in CI.

It doesn't actually test that the request building failed due to bad/missing params

Currently, the validation happens on the OpenAI backend side. I'll be happy to accept any client validation, though 👍🏻

urjitbhatia · 2023-11-12T20:56:38Z

@sashabaranov added the style fixes from the last review :)

496672097 · 2023-11-13T03:46:51Z

"I need help. How can I obtain the result of a run after executing the CreateRun() method?"

vsumit89 · 2023-11-13T07:39:43Z

HI @sashabaranov @urjitbhatia any updates by when this PR will be merged.
Thanks for all the work you guys have been putting in!

lowczarc · 2023-11-14T15:30:46Z

messages.go

+// CreateMessage creates a new message.
+func (c *Client) CreateMessage(ctx context.Context, threadID string, request MessageRequest) (msg Message, err error) {
+	urlSuffix := fmt.Sprintf("/threads/%s/%s", threadID, messagesSuffix)
+	req, err := c.newRequest(ctx, http.MethodPost, c.fullURL(urlSuffix), withBody(request))


Any reason for not adding withBetaAssistantV1() here ? When using CreateMessage I still get the error You must provide the 'OpenAI-Beta' header to access the Assistants API. Please try again by setting the header 'OpenAI-Beta: assistants=v1'.

@lowczarc I ran into this and submitted: #566

* fix test server setup: - go map access is not deterministic - this can lead to a route: /foo/bar/1 matching /foo/bar before matching /foo/bar/1 if the map iteration go through /foo/bar first since the regex match wasn't bound to start and end anchors - registering handlers now converts * in routes to .* for proper regex matching - test server route handling now tries to fully match the handler route * add missing /v1 prefix to fine-tuning job cancel test server handler * add create message call * add messages list call * add get message call * add modify message call, fix return types for other message calls * add message file retrieve call * add list message files call * code style fixes * add test for list messages with pagination options * add beta header to msg calls now that sashabaranov#545 is merged * Update messages.go Co-authored-by: Simone Vellei <[email protected]> * Update messages.go Co-authored-by: Simone Vellei <[email protected]> * add missing object details for message, fix tests * fix merge formatting * minor style fixes --------- Co-authored-by: Simone Vellei <[email protected]>

urjitbhatia force-pushed the feat/messages-api branch from 94c712d to 6084633 Compare November 8, 2023 17:21

urjitbhatia force-pushed the feat/messages-api branch from 6084633 to 290e8fd Compare November 8, 2023 17:38

urjitbhatia added 10 commits November 8, 2023 11:42

add missing /v1 prefix to fine-tuning job cancel test server handler

e100cd9

add create message call

3da7578

add messages list call

5507d4a

add get message call

4fd5161

add modify message call, fix return types for other message calls

6beb908

add message file retrieve call

d750798

add list message files call

57796cc

code style fixes

c806b64

add test for list messages with pagination options

46f7798

urjitbhatia force-pushed the feat/messages-api branch from 290e8fd to 46f7798 Compare November 8, 2023 17:43

add beta header to msg calls now that sashabaranov#545 is merged

17673a1

henomis suggested changes Nov 9, 2023

View reviewed changes

raulsh mentioned this pull request Nov 11, 2023

How to use ASSISTANTS and Threads to get response。 #563

Open

urjitbhatia and others added 5 commits November 11, 2023 16:06

Update messages.go

f552c45

Co-authored-by: Simone Vellei <[email protected]>

Update messages.go

8defb64

Co-authored-by: Simone Vellei <[email protected]>

add missing object details for message, fix tests

60b6f17

Merge branch 'master' into feat/messages-api

2e63e47

fix merge formatting

c07ed29

sashabaranov reviewed Nov 12, 2023

View reviewed changes

minor style fixes

e152f43

sashabaranov approved these changes Nov 13, 2023

View reviewed changes

sashabaranov merged commit b7cac70 into sashabaranov:master Nov 13, 2023
3 checks passed

lowczarc reviewed Nov 14, 2023

View reviewed changes

lowczarc mentioned this pull request Nov 14, 2023

fix: Missing Beta assistant Header in CreateMessage #578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/messages api #546

Feat/messages api #546

urjitbhatia commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading

henomis commented Nov 9, 2023

henomis Nov 9, 2023

wti806 commented Nov 10, 2023

urjitbhatia commented Nov 10, 2023 via email

urjitbhatia commented Nov 11, 2023

sashabaranov Nov 12, 2023

sashabaranov Nov 12, 2023

sashabaranov commented Nov 12, 2023 •

edited

Loading

urjitbhatia commented Nov 12, 2023

496672097 commented Nov 13, 2023

vsumit89 commented Nov 13, 2023

lowczarc Nov 14, 2023

floodfx Nov 14, 2023

	Text MessageText `json:"text"`
	Text *MessageText `json:"text,omitempty"`
	ImageFile *ImageFile `json:"image_file,omitempty"` // <- define this one

Feat/messages api #546

Feat/messages api #546

Conversation

urjitbhatia commented Nov 7, 2023 • edited Loading

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

henomis commented Nov 9, 2023

henomis Nov 9, 2023

Choose a reason for hiding this comment

wti806 commented Nov 10, 2023

urjitbhatia commented Nov 10, 2023 via email

urjitbhatia commented Nov 11, 2023

sashabaranov Nov 12, 2023

Choose a reason for hiding this comment

sashabaranov Nov 12, 2023

Choose a reason for hiding this comment

sashabaranov commented Nov 12, 2023 • edited Loading

urjitbhatia commented Nov 12, 2023

496672097 commented Nov 13, 2023

vsumit89 commented Nov 13, 2023

lowczarc Nov 14, 2023

Choose a reason for hiding this comment

floodfx Nov 14, 2023

Choose a reason for hiding this comment

urjitbhatia commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading

sashabaranov commented Nov 12, 2023 •

edited

Loading