chore: remove google language manager#10639
Conversation
rafaelromcar-parabol
left a comment
rafaelromcar-parabol
left a comment
There was a problem hiding this comment.
I just wanted to make sure you were not removing the GOOGLE_CLOUD variables, because they are also used for the GCP GCS file store. Good from my side.
|
Hey @mattkrick could you review this for @nickoferrall ? It's also useful for me, as it would remove one more thing from GCP 😸 I want to move the old PPMIs to their new GCP projects and I want to enable only useful APIs 😄 |
mattkrick
left a comment
mattkrick
left a comment
There was a problem hiding this comment.
I'm a bit concerned that the demo now operates VERY differently from what happens on the server, specifically that entities won't exist on the server, but it still exists on the demo entities & there's server code that still shows a reference to it. That's gonna get really confusing for us as devs in 6 months from now when we forget what happened here.
My suggestion:
- get rid of all references to GoogleAnalyzedEntities (lemma, salience), including removing it from the GQL schema
- change the demo version of
updateReflectionContentso the logic matches what the server version does. you can have it create a title with OpenAI or use a precanned version, but it shouldn't reference entities because those shouldn't exist anymore
| .filter((word) => word.length > 3) // Skip small words | ||
| .slice(0, 3) | ||
| .join(' ') | ||
| .toLowerCase() |
There was a problem hiding this comment.
+1 what about acronyms or proper nouns?
| import {GoogleAnalyzedEntity} from '../../public/resolverTypes' | ||
|
|
||
| const getReflectionEntities = async (plaintextContent: string) => { | ||
| const getBasicLemma = (word: string): string => { |
There was a problem hiding this comment.
-2 this is just too brittle. it'll only work on english, and even then words like "bread" and "Brent" are gonna get recognized as the same lemma. In cases like this, where there's a high likelihood of bad data, we're usually better of with just no data at all.
|
Sounds good! I'll need to refactor Spotlight to use OpenAI so that we can fully remove it. Working on that now |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
| // Then it auto-generates a header | ||
| await expect( | ||
| page.locator( | ||
| `[data-cy=group-column-Start] [data-cy*="Start-group-"] input[value="Things Notion"]` |
There was a problem hiding this comment.
Now that we're using AI to generate group titles, they could be anything, so I've removed these tests
packages/server/graphql/public/types/GetDemoGroupTitleSuccess.ts
Outdated
Show resolved
Hide resolved
packages/server/graphql/public/typeDefs/GetDemoGroupTitlePayload.graphql
Outdated
Show resolved
Hide resolved
| Query: { | ||
| '*': isAuthenticated, | ||
| getDemoEntities: rateLimit({perMinute: 5, perHour: 50}), | ||
| getDemoGroupTitle: allow, |
There was a problem hiding this comment.
-2 this endpoint could be hit by anyone as much as they want, they don't even need to be logged in. could this be abused by an attacker to cost us a ton of $$? are there any cheap mitigation strategies we can implement?
There was a problem hiding this comment.
Added the rate limit!
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
3 participants
Please review #10862 first.
This PR removes the Google Language Manager now that we're using ChatGPT for reflection group titles (#10546).
The demo continues to use the entities logic.
This also removes the Sentiment Score logic. See Slack discussion here.
To test