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.

Sign up for GitHub

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

Jump to bottom

[AI Tutor] CT-253: Teachers can view flagged chats #57669

Merged
merged 17 commits into from
Apr 9, 2024
Merged

[AI Tutor] CT-253: Teachers can view flagged chats #57669

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
aa332c1
controller method to fetch single student's AiTutorInteractions
Erin007 Mar 5, 2024
9528eae
add endpoint to interactionsApi
Erin007 Mar 6, 2024
8a51b60
refactor api routes for ai tutor interactions
ebeastlake Mar 29, 2024
dcf308a
refactor to more RESTful single route
ebeastlake Mar 29, 2024
45cd8f9
remove puts
ebeastlake Mar 29, 2024
2b3f1e0
fix querystring logic and autocorrect ruby linting
ebeastlake Mar 29, 2024
f1286fb
refactor ai tutor constants
ebeastlake Mar 29, 2024
424ecf5
resolve type issues
ebeastlake Mar 29, 2024
cd3ca98
populate dropdown
ebeastlake Mar 30, 2024
bde8c88
add filters for status, student, and time
ebeastlake Apr 1, 2024
15c5893
resolve merge conflicts
ebeastlake Apr 2, 2024
9b3f650
add enum for time filters
ebeastlake Apr 2, 2024
3f1a094
missed usage of enum
ebeastlake Apr 2, 2024
dd20f0f
loosen type checking
ebeastlake Apr 2, 2024
67fd49d
use AITutorInteractionStatusValue over string
ebeastlake Apr 2, 2024
2708594
Merge branch 'staging' into emily/ct-253/view-flagged-chats
ebeastlake Apr 2, 2024
56979a7
remove errant semicolon
ebeastlake Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/script/generateSharedConstants.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ def main
CHILD_ACCOUNT_COMPLIANCE_STATES
CENSUS_CONSTANTS
DANCE_SONG_MANIFEST_FILENAME
AI_TUTOR_INTERACTION_SAVE_STATUS
AI_TUTOR_INTERACTION_STATUS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the rationale below for refactoring here. Is it dangerous to refactor shared constants?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems ok to me, especially since it's something pretty clearly owned/consumed by your team?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about Ben's comment at https://github.com/code-dot-org/code-dot-org/pull/57669/files#r1548141051 - are their plans to use STATUS on the backend for aichat @sanchitmalhotra126 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could but it's a bit unclear since our API design is still being finalized. I'm okay with keeping this in the AI Tutor space for now and revisiting/refactoring when the API is more solidified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I said "dangerous," what I was thinking of was, "Is there some script or something I need to reseed this constant file?" If tests are passing, I'll assume not. I'm happy to refactor this more down the line if it's useful for generative AI.

AI_TUTOR_TYPES
PII_TYPES
FEATURED_PROJECT_STATUS
)

Expand Down
5 changes: 4 additions & 1 deletion apps/src/aiTutor/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import {Role, Status} from '@cdo/apps/aiTutor/types';
import {
Role,
AITutorInteractionStatus as Status,
} from '@cdo/apps/aiTutor/types';

export const compilationSystemPrompt =
'You are a tutor in a high school computer science class. Students in the class are studying Java and they would like to know in age-appropriate, clear language why their code does not compile. Do not write any code.';
Expand Down
51 changes: 37 additions & 14 deletions apps/src/aiTutor/interactionsApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import {AITutorInteraction} from './types';
import MetricsReporter from '@cdo/apps/lib/metrics/MetricsReporter';
import {MetricEvent} from '@cdo/apps/lib/metrics/events';

// TODO: Pagination options can be added here
interface FetchAITutorInteractionsOptions {
sectionId?: number;
userId?: number;
}

export async function savePromptAndResponse(
interactionData: AITutorInteraction
) {
Expand All @@ -18,30 +24,47 @@ export async function savePromptAndResponse(
} catch (error) {
MetricsReporter.logError({
event: MetricEvent.AI_TUTOR_CHAT_SAVE_FAIL,
errorMessage: error,
errorMessage:
(error as Error).message || 'Failed to save AI Tutor interaction',
});
}
}

// Fetch student chat messages to display to teachers.
export const fetchStudentChatMessages = async (sectionId: number) => {
// Fetch AI Tutor chat messages based on context: for all students, a specific section, or a specific student
export const fetchAITutorInteractions = async (
options: FetchAITutorInteractionsOptions
) => {
const baseUrl = `/ai_tutor_interactions`;
const queryParams = [];

if (options.sectionId) {
queryParams.push(`sectionId=${options.sectionId}`);
}
if (options.userId) {
queryParams.push(`userId=${options.userId}`);
}

const queryString = queryParams.join('&');
const url = `${baseUrl}${queryString ? `?${queryString}` : ''}`;
try {
const response = await fetch(
`/ai_tutor_interactions?sectionId=${sectionId}`,
{
method: 'GET',
headers: {
'Content-Type': 'application/json',
'X-CSRF-Token': await getAuthenticityToken(),
},
}
);
const response = await fetch(url, {
method: 'GET',
headers: {
'Content-Type': 'application/json',
'X-CSRF-Token': await getAuthenticityToken(),
},
});
if (!response.ok) {
throw new Error('Network response was not ok');
}
const data = await response.json();
return data;
} catch (error) {
MetricsReporter.logError({
event: MetricEvent.AI_TUTOR_CHAT_FETCH_FAIL,
errorMessage: error,
errorMessage:
(error as Error).message || 'Failed to fetch AI Tutor chat messages',
});
return null;
}
};
18 changes: 10 additions & 8 deletions apps/src/aiTutor/redux/aiTutorRedux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,20 @@ import {
} from '@cdo/apps/aiTutor/constants';
import {savePromptAndResponse} from '../interactionsApi';
import {
TutorType,
Role,
Status,
AITutorInteractionStatus as Status,
ChatCompletionMessage,
Level,
ChatContext,
AITutorTypesValue,
AITutorTypes as TutorTypes,
AITutorInteractionStatusValue,
} from '../types';

const registerReducers = require('@cdo/apps/redux').registerReducers;

export interface AITutorState {
selectedTutorType: TutorType | undefined;
selectedTutorType: AITutorTypesValue | undefined;
level: Level | undefined;
scriptId: number | undefined;
aiResponse: string | undefined;
Expand Down Expand Up @@ -58,9 +60,9 @@ export const askAITutor = createAsyncThunk(
};

const tutorType = chatContext.tutorType;
const generalChat = tutorType === TutorType.GENERAL_CHAT;
const compilation = tutorType === TutorType.COMPILATION;
const validation = tutorType === TutorType.VALIDATION;
const generalChat = tutorType === TutorTypes.GENERAL_CHAT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a little refactor happy and renamed TutorType to TutorTypes to indicate that this references an object that stores all of the types/multiple fields 😅

const compilation = tutorType === TutorTypes.COMPILATION;
const validation = tutorType === TutorTypes.VALIDATION;

let systemPrompt;
if (validation) {
Expand Down Expand Up @@ -129,7 +131,7 @@ const aiTutorSlice = createSlice({
reducers: {
setSelectedTutorType: (
state,
action: PayloadAction<TutorType | undefined>
action: PayloadAction<AITutorTypesValue | undefined>
) => {
state.selectedTutorType = action.payload;
},
Expand Down Expand Up @@ -159,7 +161,7 @@ const aiTutorSlice = createSlice({
},
updateChatMessageStatus: (
state,
action: PayloadAction<{id: number; status: Status}>
action: PayloadAction<{id: number; status: AITutorInteractionStatusValue}>
) => {
const {id, status} = action.payload;
const chatMessage = state.chatMessages.find(msg => msg.id === id);
Expand Down
55 changes: 31 additions & 24 deletions apps/src/aiTutor/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,68 @@
import {
AiTutorInteractionSaveStatus,
AiTutorTypes,
AiTutorInteractionStatus as AITutorInteractionStatus,
AiTutorTypes as AITutorTypes,
PiiTypes as PII,
} from '@cdo/apps/util/sharedConstants';

export type ChatCompletionMessage = {
export type AITutorTypesValue =
(typeof AITutorTypes)[keyof typeof AITutorTypes];
export type AITutorInteractionStatusValue =
(typeof AITutorInteractionStatus)[keyof typeof AITutorInteractionStatus];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was operating at the edge of my TypeScript knowledge, so I'd appreciate a second set of eyes. I'm also tripped up by naming conventions here and would appreciate any guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this syntax before, but let's try to figure it out! AITutorInteractionStatus is

 AI_TUTOR_INTERACTION_STATUS = {
    ERROR: 'error',
    PII_VIOLATION: 'pii_violation',
    PROFANITY_VIOLATION: 'profanity_violation',
    OK: 'ok',
    UNKNOWN: 'unknown',
}

keyof typeof AITutorInteractionStatus gives you all the keys in an object (helpful article). In this case it is 'ERROR' | 'PII_VIOLATION' | 'PROFANITY_VIOLATION' | 'OK' | 'UNKNOWN'

typeof AITutorInteractionStatus is (referencing that same article) {ERROR: string, PII_VIOLATION: string, ...}.

If you put those together, we are getting the types for each key in typeof AITutorInteractionStatus, which ends up being string | string | string ..., which gets combined into just string (I tried this out locally and that's what I ended up with, assuming my initial assumption on the object was correct). If you hover over AITutorInteractionStatusValue do you see string or something else? I'm guessing we actually wanted the values of the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find an easy way to turn the values into a type if that's what we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

The as const assertion can be handy here - if you add that to the original object, TS treat the types as the literal values (instead of string)

Copy link
Contributor

Choose a reason for hiding this comment

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

For example -

const Status = {
  key1: 'value1',
  key2: 'value2',
};

// type is string
type StatusValues = (typeof Status)[keyof typeof Status];

const StatusConst = {
  key1: 'value1',
  key2: 'value2',
} as const;

// type is 'value1' | 'value2'
type StatusConstValue = (typeof StatusConst)[keyof typeof StatusConst];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you so much for the deep dive here, Molly and Sanchit!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrinkle: The imported constant comes from here:

code-dot-org/apps/script/generateSharedConstants.rb

Lines 40 to 41 in 15c5893

raw = source_module.const_get(shared_const_name)
begin
.

I tried

import { AiTutorTypes } from '@cdo/apps/util/sharedConstants';

const AITutorTypes = AiTutorTypes as const;

And that doesn't work. One idea is to modify generateSharedConstants to re-export with const assertion, but that doesn't seem worth the trouble assuming it's possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this doesn't seem to work either:

export type AITutorInteractionStatusValue =
  | AITutorInteractionStatus.ERROR
  | AITutorInteractionStatus.PII_VIOLATION
  | AITutorInteractionStatus.PROFANITY_VIOLATION
  | AITutorInteractionStatus.OK
  | AITutorInteractionStatus.UNKNOWN;

I'm wondering if it's just easiest to have...

export type AITutorInteractionStatusValue =
  | 'error'
  | 'pii_violation'
  | 'profanity_violation'
  | 'ok'
  | 'unknown';

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think just copying the constants over seems fine here


export {AITutorInteractionStatus, AITutorTypes, PII};

export interface ChatCompletionMessage {
id: number;
role: Role;
chatMessageText: string;
status: Status;
status: AITutorInteractionStatusValue;
timestamp?: string;
};
}

export type AITutorInteraction = {
export interface AITutorInteraction {
userId?: number;
levelId?: number;
scriptId?: number;
type: TutorType | undefined;
type: AITutorTypesValue | undefined;
isProjectBacked?: boolean;
prompt: string;
status: string;
aiResponse?: string;
};
}

export type StudentChatRow = {
export interface StudentChatRow {
aiModelVersion: string;
aiResponse?: string;
createdAt: string;
id: number;
studentName: string;
type: TutorType;
levelId?: number;
projectId?: string;
prompt: string;
scriptId?: number;
status: string;
aiResponse?: string;
createdAt: string;
};
studentName: string;
type: AITutorTypesValue;
updatedAt?: string;
userId: number;
Copy link
Contributor Author

@ebeastlake ebeastlake Apr 1, 2024 •
edited

Choose a reason for hiding this comment

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

These fields seemed missing but were on the chatMessage on the front end.

}

export type Level = {
export interface Level {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per recommendation from @molly-moen, prefer interface to type where possible.

id: number;
type: string;
hasValidation: boolean;
isAssessment: boolean;
isProjectBacked: boolean;
};
}

export interface ChatContext {
// studentInput is the last user message for general chat
// or the student's code for compilation and validaiton.
studentInput: string;
tutorType: TutorType | undefined;
tutorType: AITutorTypesValue | undefined;
}

export enum Role {
ASSISTANT = 'assistant',
USER = 'user',
SYSTEM = 'system',
}
export type Status =
(typeof AiTutorInteractionSaveStatus)[keyof typeof AiTutorInteractionSaveStatus];
export const Status = AiTutorInteractionSaveStatus;
export const PII = [Status.EMAIL, Status.ADDRESS, Status.PHONE];

export type TutorType = (typeof AiTutorTypes)[keyof typeof AiTutorTypes];
export const TutorType = AiTutorTypes;
22 changes: 11 additions & 11 deletions apps/src/aichat/chatApi.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I expect that for Gen AI we may just create a new API file since we're no longer using this chat API. If it makes more sense to just move this to the aiTutor directory, feel free!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right that refactoring this code makes sense. It looks like the only way this file is used is to import {getChatCompletionMessage} from '../chatApi'; in two files: aichatRedux.ts and aiTutorRedux.ts. There will be a fairly significant diff with that refactor, so I'd like to address it as a follow-up task!

Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
import {
Role,
Status,
AITutorInteractionStatus as Status,
AITutorInteractionStatusValue,
AITutorTypesValue,
ChatCompletionMessage,
PII,
} from '@cdo/apps/aiTutor/types';
import HttpClient from '@cdo/apps/util/HttpClient';
import {CHAT_COMPLETION_URL} from './constants';
import Lab2Registry from '../lab2/Lab2Registry';
import {TutorType} from '../aiTutor/types';

/**
* This function sends a POST request to the chat completion backend controller.
*/
export async function postOpenaiChatCompletion(
messagesToSend: OpenaiChatCompletionMessage[],
levelId?: number,
tutorType?: TutorType
tutorType?: AITutorTypesValue
): Promise<OpenaiChatCompletionMessage | null> {
const payload = levelId
? {levelId: levelId, messages: messagesToSend, type: tutorType}
Expand Down Expand Up @@ -54,7 +54,7 @@ export async function getChatCompletionMessage(
newMessage: string,
chatMessages: ChatCompletionMessage[],
levelId?: number,
tutorType?: TutorType
tutorType?: AITutorTypesValue
): Promise<ChatCompletionResponse> {
const messagesToSend = [
{role: Role.SYSTEM, content: systemPrompt},
Expand All @@ -77,16 +77,16 @@ export async function getChatCompletionMessage(
// For now, response will be null if there was an error.
if (!response) {
return {status: Status.ERROR, id: userMessageId};
} else if (response.status === Status.PROFANITY) {
} else if (response?.status === Status.PROFANITY_VIOLATION) {
return {
status: Status.PROFANITY,
status: Status.PROFANITY_VIOLATION,
id: userMessageId,
assistantResponse:
"I can't respond because your message is inappropriate. Please don't use profanity.",
};
} else if (response && response.status && PII.includes(response.status)) {
} else if (response?.status === Status.PII_VIOLATION) {
return {
status: Status.PERSONAL,
status: Status.PII_VIOLATION,
id: userMessageId,
assistantResponse: `I can't respond because your message is inappropriate. Please don't include personal information like your ${response.status}.`,
};
Expand All @@ -99,12 +99,12 @@ export async function getChatCompletionMessage(
}

type OpenaiChatCompletionMessage = {
status?: Status;
status?: AITutorInteractionStatusValue;
role: Role;
content: string;
};
type ChatCompletionResponse = {
status: Status;
status: AITutorInteractionStatusValue;
id: number;
assistantResponse?: string;
};
6 changes: 5 additions & 1 deletion apps/src/aichat/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
export const CHAT_COMPLETION_URL = '/openai/chat_completion';
import {ChatCompletionMessage, Role, Status} from './types';
import {
ChatCompletionMessage,
Role,
AITutorInteractionStatus as Status,
} from './types';

const initialChatMessages: ChatCompletionMessage[] = [
{
Expand Down
5 changes: 3 additions & 2 deletions apps/src/aichat/redux/aichatRedux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import {
ChatCompletionMessage,
AichatLevelProperties,
Role,
Status,
AITutorInteractionStatus as Status,
AITutorInteractionStatusType,
} from '../types';

const getCurrentTimestamp = () => moment(Date.now()).format('YYYY-MM-DD HH:mm');
Expand Down Expand Up @@ -116,7 +117,7 @@ const aichatSlice = createSlice({
},
updateChatMessageStatus: (
state,
action: PayloadAction<{id: number; status: Status}>
action: PayloadAction<{id: number; status: AITutorInteractionStatusType}>
) => {
const {id, status} = action.payload;
const chatMessage = state.chatMessages.find(msg => msg.id === id);
Expand Down
18 changes: 10 additions & 8 deletions apps/src/aichat/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
import {LevelProperties} from '@cdo/apps/lab2/types';
import {AiTutorInteractionSaveStatus} from '@cdo/apps/util/sharedConstants';
import {
AiTutorInteractionStatus as AITutorInteractionStatus,
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly out of scope, but should this be called AiInteractionStatus if it's used across aichat and aitutor?

Copy link
Contributor Author

@ebeastlake ebeastlake Apr 2, 2024 •
edited

Choose a reason for hiding this comment

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

@bencodeorg I think my ultimate goal is in the spirit of this comment, which is to condense/standardize things. I don't know how long the AI Chat use case will continue to be relevant.

Per @sanchitmalhotra126's comment above, the best way to achieve clarity/reusability might be to 1) wait until we have a better understanding of the API for the gen AI unit, 2) use it to make these values more reusable across gen AI and AI tutor (and then rename them then if applicable), and 3) ultimately look to deprecate any aichat code that doesn't move to AI Tutor or Gen AI code? As far as I know, the AI Chat allthethings level is no longer used...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you all creating a new directory or using the aichat directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're actively doing development of the gen AI unit using the AI Chat allthethings level! AI Chat is being repurposed as gen AI. The API will probably be replaced though, to Sanchit's point. Fine to leave as-is for now, just a thought :)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry just saw comment re: directory, using the aichat directory!

PiiTypes as PII,
} from '@cdo/apps/util/sharedConstants';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this file is used much anymore, but it shares types with AI tutor code. Tagging @sanchitmalhotra126 @bencodeorg @fisher-alice for confirmation this is okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new "generative AI" lab is using being built on the aichat lab, so it's back in use!


export type AITutorInteractionStatusType =
(typeof AITutorInteractionStatus)[keyof typeof AITutorInteractionStatus];

export {PII, AITutorInteractionStatus};

export type ChatCompletionMessage = {
id: number;
role: Role;
chatMessageText: string;
status: Status;
status: AITutorInteractionStatusType;
timestamp?: string;
};

Expand All @@ -14,12 +22,6 @@ export enum Role {
USER = 'user',
SYSTEM = 'system',
}

export type Status =
(typeof AiTutorInteractionSaveStatus)[keyof typeof AiTutorInteractionSaveStatus];
export const Status = AiTutorInteractionSaveStatus;
export const PII = [Status.EMAIL, Status.ADDRESS, Status.PHONE];

export interface AichatLevelProperties extends LevelProperties {
// --- DEPRECATED - used for old AI Chat
systemPrompt: string;
Expand Down