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

feat: display token usage #77

Merged
merged 8 commits into from
Jan 17, 2025
Merged

feat: display token usage #77

merged 8 commits into from
Jan 17, 2025

Conversation

fengsh27
Copy link
Collaborator

@fengsh27 fengsh27 commented Jan 6, 2025

Abstract

This submission is to display the token usage. If it reaches server daily token limit (ERROR_BIOSERVER_EXCEEDS_TOKEN_LIMIT), we will restrict further usage and encourage users to provide their OpenAI API key for access.

This PR also fixed #48

fengsh27 and others added 3 commits December 12, 2024 08:51
* display token usage

* adjust sidebar style

* save session id as user name for client authorization

* prompt users to provide openai api key if server community daily token limit reached

* minor changes

---------

Co-authored-by: fengsh <[email protected]>
@fengsh27 fengsh27 requested a review from slobentanzer January 6, 2025 18:55
@slobentanzer
Copy link
Contributor

Hi @fengsh27, many thanks for the PR!

The chat is functional for me, but I don't see the actual tokens in the sidebar (they stay at 0). I built the biochatter-server PR locally as biochatter-server:0.5.5 and am using it in the docker compose of this PR. Do we always want to display token usage, also in the case of a user-supplied key?

Where is the sqlite database built in those use cases? Is it the server or the next that manages the DB?

Minor detail: I think it makes no sense to display the OncoKB radio button in the generic version of the Next app; this should be hidden if not specifically requested / implemented. If we show something there, it should be a generic "tool calling" option, but that we would need to implement to be able to select any of the supported tools in the API agent of biochatter, which seems like a bit of work. I would just hide the OncoKB button for now in the generic biochatter-next.

@fengsh27
Copy link
Collaborator Author

Hi @fengsh27, many thanks for the PR!

The chat is functional for me, but I don't see the actual tokens in the sidebar (they stay at 0). I built the biochatter-server PR locally as biochatter-server:0.5.5 and am using it in the docker compose of this PR. Do we always want to display token usage, also in the case of a user-supplied key?

Where is the sqlite database built in those use cases? Is it the server or the next that manages the DB?

Minor detail: I think it makes no sense to display the OncoKB radio button in the generic version of the Next app; this should be hidden if not specifically requested / implemented. If we show something there, it should be a generic "tool calling" option, but that we would need to implement to be able to select any of the supported tools in the API agent of biochatter, which seems like a bit of work. I would just hide the OncoKB button for now in the generic biochatter-next.

Yes, it is supposed to display token usage for user-provided key, let me check what is wrong.

The sqlite database is managed by biochatter-server.

For OncoKB in generic app, it makes sense to disable it, I'll submit a fix for it, it won't take much efforts.

@fengsh27
Copy link
Collaborator Author

fengsh27 commented Jan 10, 2025

@slobentanzer , I've submitted a fix in biochatter-server, which fixed an error in get_embedding_function. The error was introduced in upgrading langchain_community.openai to langchain_openai, in which the server ENV OPENAI_API_KEY=azure will affect OpenAIEmbeddings.
Could you check if this fix works on your machine?
Additionally, please make sure to set ENV DATA_DIR in .bioserver.env that specifies where to save sqlite database

@slobentanzer
Copy link
Contributor

If the location is necessary for the functionality, wouldn't it make sense to set a reasonable default that works also in the context of running the server in docker compose, and only expect the user to set the variable if they want to override the default?

@fengsh27
Copy link
Collaborator Author

Yes, the DATA_DIR is not necessary. Currently, it worked well without setting it as it already has default value "./data", see the following code in token_usage_database.py:

db_path = os.environ.get(DATA_DIR, "./data")

@fengsh27
Copy link
Collaborator Author

@slobentanzer, Could you please check if the fix in PR in biochatter-server resolves the issue that client-provided key does not work?

@slobentanzer
Copy link
Contributor

@fengsh27 apologies for the delay, was working on another PR. I have now tested with the new server fix, it shows the tokens correctly.

@slobentanzer
Copy link
Contributor

Server is merged and bumped to 0.5.5; I pushed the image to dockerhub.

@fengsh27
Copy link
Collaborator Author

Thanks.

It seems I forgot to disable OncoKB for generic app. I'll fix it with a separate PR.

@fengsh27
Copy link
Collaborator Author

@slobentanzer I've made a submission to disable OncoKB feature for generic app

Copy link
Contributor

@slobentanzer slobentanzer left a comment

Choose a reason for hiding this comment

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

@fengsh27 thank you, looks good! Good to merge from my end.

@fengsh27 fengsh27 merged commit 6e3eb19 into biocypher:main Jan 17, 2025
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.

Typo in global variable
2 participants