707 refactor of chatgptchatcompletion #758

pmarsh-scottlogic · 2024-01-11T17:04:47Z

Description

REFACTOR: moves the logic for setting the system role in chat history to its own method (setSystemRoleInChatHistory), and then instead of calling that method from chatGptChatCompletion (openAI module), we call it from handleChatToGPT (chatController module). The idea of moving where it's invoked is to make methods responsible for fewer things.

Rewrites tests accordingly.

Notes

we also alter the logic to reduce mutation. Except we change the value of the chat history in the session once we've made the updated value (with the system role set)
unit tests the new method setSystemRoleInChatHistory on its own, then just checks that it gets called in handleChatToGPT
removes the old integration test that checked setting system roles in the chat history

Concerns

Wasn't sure where to put the definition of setSystemRoleInChatHistory, so I left it in openai. In my ideal word, we'd have some module "chatService" which sits in between the chatController and the lower level modules (openai, langchain) which would deal with this sort of thing.

Checklist

Have you done the following?

Linked the relevant Issue
Added tests
Ensured the workflow steps are passing

…t naming

gsproston-scottlogic

Very nice! The only thing I'd prefer would be to move setSystemRoleInChatHistory out of openai.ts.
I'd argue it can go in the chat controller, as that's the only place that uses it, although I appreciate that file is getting a little large.
Adding services for each controller would be nice, but a little overkill for this ticket. Could be worth making a new issue for it though? 👀
Alternatively, in #740 I added a chat utils file, could go there?

backend/src/openai.ts

pmarsh-scottlogic · 2024-01-12T14:49:38Z

Very nice! The only thing I'd prefer would be to move setSystemRoleInChatHistory out of openai.ts. I'd argue it can go in the chat controller, as that's the only place that uses it, although I appreciate that file is getting a little large. Adding services for each controller would be nice, but a little overkill for this ticket. Could be worth making a new issue for it though? 👀 Alternatively, in #740 I added a chat utils file, could go there?

Re chat Service, I've just made #761 to introduce that. I don't think the other controllers are big enough to warrant a service however.

I reckon I'll wait for #740 and put setSystemRoleInChatHistory in there. Thus, consider this PR BLOCKED by #740

chriswilty

Just a few thoughts.

backend/src/openai.ts

backend/test/integration/chatController.test.ts

backend/test/integration/openai.test.ts

backend/test/unit/openai.test.ts

…failures

…pushMessageToHistory

pmarsh-scottlogic · 2024-01-23T15:43:31Z

backend/test/unit/utils/chat.test.ts

I added the tests for setSystemRoleInChatHistory then decided I could split the file apart into pushMessageToHistory.test.ts and setSystemRoleInChatHistory

pmarsh-scottlogic · 2024-01-23T15:46:23Z

backend/test/unit/controller/chatController.test.ts

@@ -297,6 +324,41 @@ describe('handleChatToGPT unit tests', () => {
 				})
 			);
 		});
+
+		test('GIVEN output filtering defence enabled WHEN handleChatToGPT called THEN it should return 200 and blocked reason', async () => {


this test ain't got anything to do with this ticket, but I noticed it was missing and it's pretty straightforward so I added it in.

chriswilty

This is great 👌

One suggestion for test mocking. Mocking classes is pretty complex.... One very good reason to avoid classes in our own code!

backend/test/unit/openai.test.ts

backend/test/unit/utils/chat.ts/pushMessageToHistory.test.ts

chriswilty

Just an import to correct and we're all good 👍

backend/test/unit/openai.test.ts

chriswilty

⚡ Go!

No longer on project :(

pmarsh-scottlogic added 15 commits January 11, 2024 11:42

removes unneeded eslint disabling

dab6741

moves setting system role into its own method and improves if statmen…

9c04d11

…t naming

removes mutation from within setSystemRoleInChatHistory

2540b83

renames variable, plus moves prinst statement back to the main method

40e2da6

sets system role in chat history in chatController

c72b33b

updates props in openai that were needed for setting system role

59b6770

adds placeholder unit tests for setSystemRoleInChatHistory

a279310

tidies test file

c42f55b

adds skeleton tests with fleshed out names

8647792

write first unit test

965a6df

added extra mock and finished another test

b54ce11

finishes adding unit tests for setSystemRoleInChatHistory

a166f8c

removes big comment

97a2eac

tests that we set the system role when we chat with gpt

57069c9

removes tests for setting system role from integration/openai.test

21ecaa5

pmarsh-scottlogic linked an issue Jan 11, 2024 that may be closed by this pull request

Small refactor of chatGptChatCompletion() #707

Closed

pmarsh-scottlogic requested review from chriswilty, gsproston-scottlogic, heatherlogan-scottlogic and dhinrichs-scottlogic January 11, 2024 17:23

pmarsh-scottlogic marked this pull request as ready for review January 11, 2024 17:25

gsproston-scottlogic previously requested changes Jan 12, 2024

View reviewed changes

backend/src/openai.ts Outdated Show resolved Hide resolved

pmarsh-scottlogic mentioned this pull request Jan 12, 2024

Reorganise backend modules used for chat #761

Open

pmarsh-scottlogic added the blocked Blocked from progressing label Jan 12, 2024

chriswilty requested changes Jan 15, 2024

View reviewed changes

pmarsh-scottlogic added 4 commits January 17, 2024 09:16

simplifies bool expression

f8d6819

removes unneeded mock

a34e494

remove some extra fluff

5f352b5

merge dev

2cad40f

pmarsh-scottlogic added 16 commits January 17, 2024 10:25

cleans up mocking of @src/defence

cb7a28c

more tidying to mock getSystemRole

ad00a0e

remove mock from variable names in test

6fcf3a8

moves some variables into describe block

b5a93d7

removes further unnecessary mocks

c78308f

removes unit test frofom dwescribe block names

a896428

runs linter

3af68f7

merge dev

9d805b9

mocks setSystemRoleInChatHistory in chatController unit tests to fix …

cde1894

…failures

moves setSystemRoleInChatHistory to utils file

1560ea5

changes chat utils unit tests, removes descirbe block and renames to …

8c608a4

…pushMessageToHistory

fixes imports in chatController unit tests

643f906

does way too many things for one commit

2ecb735

removes mutation to set system role

436b322

don't check that system role set method is called

b0ee499

adds unit test to chatController for dealing with blocked ouput

6632976

pmarsh-scottlogic commented Jan 23, 2024

View reviewed changes

pmarsh-scottlogic removed the blocked Blocked from progressing label Jan 23, 2024

pmarsh-scottlogic changed the title ~~707 small refactor of chatgptchatcompletion~~ 707 refactor of chatgptchatcompletion Jan 23, 2024

chriswilty requested changes Jan 24, 2024

View reviewed changes

backend/test/unit/openai.test.ts Outdated Show resolved Hide resolved

backend/test/unit/utils/chat.ts/pushMessageToHistory.test.ts Show resolved Hide resolved

mocks library method in better way

36bc014

chriswilty requested changes Jan 26, 2024

View reviewed changes

backend/test/unit/openai.test.ts Outdated Show resolved Hide resolved

corrects import

8148127

chriswilty approved these changes Jan 29, 2024

View reviewed changes

pmarsh-scottlogic requested a review from gsproston-scottlogic January 29, 2024 14:31

chriswilty merged commit 2901fb3 into dev Jan 30, 2024
2 checks passed

chriswilty deleted the 707-small-refactor-of-chatgptchatcompletion branch January 30, 2024 10:03

chriswilty pushed a commit that referenced this pull request Apr 8, 2024

707 refactor of chatgptchatcompletion (#758)

c324b37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

707 refactor of chatgptchatcompletion #758

707 refactor of chatgptchatcompletion #758

pmarsh-scottlogic commented Jan 11, 2024 •

edited

Loading

gsproston-scottlogic left a comment

pmarsh-scottlogic commented Jan 12, 2024 •

edited

Loading

chriswilty left a comment

pmarsh-scottlogic Jan 23, 2024

pmarsh-scottlogic Jan 23, 2024

chriswilty left a comment

chriswilty left a comment

chriswilty left a comment

707 refactor of chatgptchatcompletion #758

707 refactor of chatgptchatcompletion #758

Conversation

pmarsh-scottlogic commented Jan 11, 2024 • edited Loading

Description

Notes

Concerns

Checklist

gsproston-scottlogic left a comment

Choose a reason for hiding this comment

pmarsh-scottlogic commented Jan 12, 2024 • edited Loading

chriswilty left a comment

Choose a reason for hiding this comment

pmarsh-scottlogic Jan 23, 2024

Choose a reason for hiding this comment

pmarsh-scottlogic Jan 23, 2024

Choose a reason for hiding this comment

chriswilty left a comment

Choose a reason for hiding this comment

chriswilty left a comment

Choose a reason for hiding this comment

chriswilty left a comment

Choose a reason for hiding this comment

pmarsh-scottlogic commented Jan 11, 2024 •

edited

Loading

pmarsh-scottlogic commented Jan 12, 2024 •

edited

Loading