-
Notifications
You must be signed in to change notification settings - Fork 4
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
connect to chatgpt #2419
connect to chatgpt #2419
Conversation
Move AI conversation into external function.
Move queue to Firestore collections.
collaborative-learning Run #13971
Run Properties:
|
Project |
collaborative-learning
|
Run status |
Passed #13971
|
Run duration | 14m 44s |
Commit |
1b715aa2ba: Fix test
|
Committer | Boris Goldowsky |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
1
|
Pending |
3
|
Skipped |
0
|
Passing |
112
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2419 +/- ##
==========================================
- Coverage 86.29% 86.27% -0.02%
==========================================
Files 742 742
Lines 38353 38379 +26
Branches 9796 9801 +5
==========================================
+ Hits 33096 33112 +16
- Misses 4957 4967 +10
Partials 300 300
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Store special metadata when evaluation is wanted, and use that to trigger the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good.
I left a few comments. The only one that I'd recommend addressing in this PR is changing AIEvaluation
to aiEvaluation
.
const message = reply.parsed.discussion + | ||
` Key Indicators: ${reply.parsed.keyIndicators.join(", ")}`; | ||
|
||
const commentsPath = `demo/AI/documents/${docId}/comments`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simplified path to comment might not work in all cases.
For non-networked teacher comments they are stored under uid:[someId]_[docId]
. I can't remember if someId
is the id of the teacher that is commenting or the student that owns the document.
For networked teacher comments they are stored under [networkName]_[docId]
.
The important thing to figure out is if the CLUE runtime will show both the comments stored under the [docId]
and the comments under [prefix]_[docId]
. If it doesn't show both the comments, then making do that is probably the right thing do. That seems safer than having this system figure out which prefix to store the comments under.
} else { | ||
logger.info("Creating comment for", event.document); | ||
// NOTE we are leaving the "network" and "tileId" fields empty in the comment doc. | ||
await firestore.collection(commentsPath).add({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something general that we are going to have to deal with is that the list of strategies in the Firestore metadata documents are currently updated by the CLUE runtime instead of via a Firebase function that is monitoring the comments. So just adding this comment with a strategy is not going to update the sort work view.
So in a separate story and PR we should add a Firebase function that monitors the comments and updates the Firestore metadata. And in that same PR we can delete the client side code which is updating the strategies. There is a story about handling deleted strategies: https://www.pivotaltracker.com/story/show/188144655 This Firebase function can take care of that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. I think we can try deploying these functions and see what if they work. The deploy script in the functions-v2 folder will just deploy the v2 functions.
const response = await fetch(shutterbugURL, | ||
{ | ||
method: "POST", | ||
body: JSON.stringify({content: html, height: 1500}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This height might be a little short for some documents, but it seems like a good place to start until we can use dynamic heights.
A local fake secret is provided for firebase-functions-test to use.
Includes:
This causes a special database key to be written out when a document is updated in a unit that has AI evaluation configured. Firebase functions notice this key and put the document into a queue that goes through 2 further stages:
For now, the functions only trigger if the user is within the
AI
demo space, so that is required along with being in a unit that configures AI Evaluation. The only such unit at the moment is./demo/units/qa/content.json