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

810: Session vs non-session routes #811

Merged
merged 5 commits into from
Feb 5, 2024
Merged

810: Session vs non-session routes #811

merged 5 commits into from
Feb 5, 2024

Conversation

chriswilty
Copy link
Member

@chriswilty chriswilty commented Feb 1, 2024

Description

We need to split API routes into session and non-session endpoints, so that we are not generating or noop-updating the session when we don't need to. This is particularly important for cloud deployment, where our load-balancer hits our healthcheck every 30secs, creating a new session every time 😱

Resolves #810

Checklist

Have you done the following?

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

Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

Struggling to run this locally. Some (but not all) endpoints come back with CORS errors.

Refreshing the frontend:

image

Sending a chat message:

image

Testing on postman the endpoints seem fine.

backend/package.json Show resolved Hide resolved
backend/src/langchain.ts Show resolved Hide resolved
backend/src/models/chat.ts Show resolved Hide resolved
backend/src/sessionRoutes.ts Show resolved Hide resolved
@chriswilty
Copy link
Member Author

chriswilty commented Feb 2, 2024

@pmarsh-scottlogic Ah yes sorry, you'll need to replace your backend .env with the new .env.example as I added the cors origin into there. I must remember to tell everyone else to do that once this is merged 😅

Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

Works a charm

@chriswilty
Copy link
Member Author

Note to self: will pull out the integration test changes plus the debugging log line in langchain.ts, cos it just complicates things elsewhere.

asRetriever() {
mockAsRetriever();
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@pmarsh-scottlogic It seemed unnecessary to create a class, so I inlined it below.

@@ -28,6 +28,7 @@ async function getDocuments(filePath: string) {
'.csv': (path: string) => new CSVLoader(path),
});
const docs = await loader.load();
console.debug(`${docs.length} documents found`);
Copy link
Member Author

@chriswilty chriswilty Feb 5, 2024

Choose a reason for hiding this comment

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

This is the line that exposed we weren't stubbing the loader to return anything. So I simply added that in my test changes. I'm happy for this line to be removed if you wish, it's not all that important any longer.

mockRetrievalQAChain.call.mockReset();
mockFromLLM.mockReset();
mockFromTemplate.mockReset();
mockLoader.mockReset();
Copy link
Contributor

Choose a reason for hiding this comment

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

could you use mockClear() instead and then not have to do mockLoader.mockResolvedValue([]); in the beforeEach?

doc

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic left a comment

Choose a reason for hiding this comment

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

lovely

@chriswilty chriswilty merged commit f5856e2 into dev Feb 5, 2024
2 checks passed
@chriswilty chriswilty deleted the 810-session-routes branch February 5, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split Express routing into session vs non-session
2 participants