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: PPT-568 Added Controller for ChatBot #364

Merged
merged 6 commits into from
Oct 17, 2023
Merged

feat: PPT-568 Added Controller for ChatBot #364

merged 6 commits into from
Oct 17, 2023

Conversation

naqvis
Copy link
Contributor

@naqvis naqvis commented Oct 16, 2023

No description provided.

@naqvis naqvis requested a review from stakach October 16, 2023 06:04
@github-actions github-actions bot added the type: enhancement new feature or request label Oct 16, 2023
Copy link
Member

@stakach stakach left a comment

Choose a reason for hiding this comment

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

I'm thinking we rename openai.cr to chat_gpt.cr
just as i feel it describes the functionality better and matches the class name

Also we should make the API keys configurable per-domain. Similar to what we do in webrtc.cr

      authority = current_authority.not_nil!
      authority.internals["webrtc_ice"]?.try(&.to_json) || WEBRTC_DEFAULT_ICE_CONFIG

just with openai / azure credentials

@stakach
Copy link
Member

stakach commented Oct 17, 2023

also seeing this error on our dev server when opening a websocket

level=[I] time=2023-10-17T01:18:17Z program=EventBus source=action-controller client_ip=119.18.0.136 request_id=e16aa21c-b88e-44e2-8b7b-6becac4f5923 user_id=user-C2KyPySJYghf6g event=response method=GET path=/api/engine/v2/chatgpt/chat/sys-ELNF3KjDuK?bearer_token=[FILTERED] status=101 duration=2.5878
level=[D] time=2023-10-17T01:18:17Z program=EventBus source=db message="Executing query" query=SELECT * FROM "user" WHERE "id" = $1 LIMIT 1 args=["user-C2KyPySJYghf6g"]
level=[D] time=2023-10-17T01:18:17Z program=EventBus source=db message="Executing query" query=BEGIN args=[]
level=[D] time=2023-10-17T01:18:17Z program=EventBus source=db message="Executing query" query=ROLLBACK args=[]
Unhandled exception in spawn: PlaceOS::Model::Chat has an invalid field. `summary` is required (PgORM::Error::RecordInvalid)
  from app/lib/pg-orm/src/pg-orm/persistence.cr:210:13 in '__create'
  from app/lib/pg-orm/src/pg-orm/persistence.cr:107:42 in 'save!'
  from app/lib/pg-orm/src/pg-orm/base.cr:19:5 in 'create!:user_id:system_id:summary'
  from app/src/placeos-rest-api/controllers/openai.cr:42:9 in 'chat'
  from app/src/placeos-rest-api/controllers/openai.cr:4:3 in 'ws_chat__system_id'
  from app/src/placeos-rest-api/controllers/openai.cr:4:3 in '->'
  from app/lib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/crystal/http_server.cr:230:19 in 'process'
  from usr/share/crystal/src/http/server.cr:521:5 in 'handle_client'
  from app/lib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/crystal/http_server.cr:86:9 in 'handle_client'
  from usr/share/crystal/src/http/server.cr:451:5 in '->'
  from usr/share/crystal/src/fiber.cr:146:11 in 'run'
  from usr/share/crystal/src/fiber.cr:98:34 in '->'
  from ???

@stakach
Copy link
Member

stakach commented Oct 17, 2023

I feel like we should only create a chat if the user actually asks a question
maybe add an on_summary callback that provides a summary of the request and first response once available

@naqvis
Copy link
Contributor Author

naqvis commented Oct 17, 2023

I feel like we should only create a chat if the user actually asks a question maybe add an on_summary callback that provides a summary of the request and first response once available

Yeah, but this was coded per our initial design in which call to web socket was done with initial question and payload. I removed the payload and chat, but kept the initial logic. But I agree it make more sense to create the model only after receiving the very first chat message.

@naqvis
Copy link
Contributor Author

naqvis commented Oct 17, 2023

I'm thinking we rename openai.cr to chat_gpt.cr just as i feel it describes the functionality better and matches the class name

Also we should make the API keys configurable per-domain. Similar to what we do in webrtc.cr

      authority = current_authority.not_nil!
      authority.internals["webrtc_ice"]?.try(&.to_json) || WEBRTC_DEFAULT_ICE_CONFIG

just with openai / azure credentials

Agree and will make these changes

@naqvis naqvis requested a review from stakach October 17, 2023 05:24
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Oct 17, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Oct 17, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Oct 17, 2023
@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Oct 17, 2023
Comment on lines 166 to 176
private def build_executor(chat, payload : Payload?)
executor = OpenAI::FunctionExecutor.new

description = if payload
"You have the following capability list, described in the following JSON:\n```json\n#{payload.capabilities.to_json}\n```\n" +
"if a request could benefit from these capabilities you can obtain the list of functions by providing the id string.\n" +
"id strings are case sensitive and must not be modified."
else
"if a request could benefit from a capability you can obtain the list of functions by providing the id string\n" +
"id strings are case sensitive and must not be modified."
end
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I think that needs tweaking is this chat resume
It would be nice to have the same description with the capabilities embedded

So we need to make the LLM.new_chat request call for chat resume so the data can be available

@github-actions github-actions bot added type: enhancement new feature or request and removed type: enhancement new feature or request labels Oct 17, 2023
@stakach stakach merged commit 7104d11 into master Oct 17, 2023
8 of 11 checks passed
@stakach stakach deleted the PPT-568 branch October 17, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants