Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Create AichatRequest model for ActiveJob - #60257

#60257
Merged
sanchitmalhotra126 merged 3 commits into
stagingcode-dot-org/code-dot-org:stagingfrom
sanchit/aichat-activejob-modelcode-dot-org/code-dot-org:sanchit/aichat-activejob-modelCopy head branch name to clipboard
Aug 8, 2024
Merged

Create AichatRequest model for ActiveJob#60257
sanchitmalhotra126 merged 3 commits into
stagingcode-dot-org/code-dot-org:stagingfrom
sanchit/aichat-activejob-modelcode-dot-org/code-dot-org:sanchit/aichat-activejob-modelCopy head branch name to clipboard

Conversation

@sanchitmalhotra126

Copy link
Copy Markdown
Contributor

Part 1 of moving the aichat chat completion request to an Active Job. This model will be used to store the inputs for the sagemaker request, the status of the job, and the output when completed (or appropriate failure status).

I've also included a list of execution status codes I'm thinking of using in shared_constants. These aren't directly used in this PR, but will be in the job processing and front end request/polling implementation. Included here for reference on how the "status" column would be used. These are modeled after the RUBRIC_AI_EVALUATION_STATUS codes that are similarly used to track job status.

Also note that because this model stores the user ID of the requestor, this table can also serve as a record of requests and responses from SageMaker (instead of storing this information in Cloudwatch). I also considered storing additional metadata like level ID, channel ID, etc, but opted to keep this table focused on the specific information necessary for the task. I figured if we wanted to look up a specific user, we could look up their full event history in the AichatEvents table.

Links

https://codedotorg.atlassian.net/browse/LABS-959

Testing story

Tested by creating models in the console and in ActiveJob code.

@sanchitmalhotra126
sanchitmalhotra126 requested review from a team and fisher-alice August 7, 2024 21:09
@sanchitmalhotra126
sanchitmalhotra126 requested a review from a team as a code owner August 7, 2024 21:09
t.integer :user_id, null: false
t.json :model_customizations, null: false
t.json :stored_messages
t.text :new_message, null: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In aichat_controller, 'newMessage' is a ChatMessage with fields role, chatMessageText, and status. Are we only storing the message text in this table?

If we are storing other fields (role and status): Would it be helpful to store as json as well? And maybe rename status as request_status below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch! yeah this should probably be json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, discussed offline - will rename status to execution_status

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the name update!

Comment thread dashboard/db/schema.rb
TEACHER: 'teacher',
).freeze

AI_REQUEST_EXECUTION_STATUS = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you naming as AI_REQUEST... instead of AICHAT_REQUEST... anticipating that these will also be used across AI projects?

@sanchitmalhotra126 sanchitmalhotra126 Aug 8, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was my thought (and that AICHAT_REQUEST_EXECUTION_STATUS felt a bit too long 😄). Though we can also keep it AICHAT-specific if that's clearer.

@fisher-alice fisher-alice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@sanchitmalhotra126

Copy link
Copy Markdown
Contributor Author

Added level_id, script_id, and project_id fields to the model after offline discussion with @fisher-alice

@cnbrenci cnbrenci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe we do already have work on the backlog for periodic cleanup / aging out of requests in this table. This will definitely be important to prioritize since we don't want the table to grow unbounded forever :) LGTM!

@sanchitmalhotra126

Copy link
Copy Markdown
Contributor Author

I believe we do already have work on the backlog for periodic cleanup / aging out of requests in this table. This will definitely be important to prioritize since we don't want the table to grow unbounded forever :) LGTM!

@cnbrenci yep, good callout! I'll add this specific table to the JIRA notes to make sure we clean this up too.

@sanchitmalhotra126
sanchitmalhotra126 merged commit 3331f09 into staging Aug 8, 2024
@sanchitmalhotra126
sanchitmalhotra126 deleted the sanchit/aichat-activejob-model branch August 8, 2024 22:01
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.