-
Notifications
You must be signed in to change notification settings - Fork 930
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
Gemini help feature #6937
base: master
Are you sure you want to change the base?
Gemini help feature #6937
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6937 +/- ##
=======================================
Coverage 54.56% 54.57%
=======================================
Files 355 355
Lines 24723 24723
Branches 5107 5107
=======================================
+ Hits 13490 13492 +2
+ Misses 10013 10011 -2
Partials 1220 1220 ☔ View full report in Codecov by Sentry. |
// TODO(christhompson): Add project ID for this. | ||
// TODO(christhompson): Add preamble information about command flags. | ||
// TODO(christhompson): Figure out how to test this. | ||
const generationConfig = { |
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.
Give this object a structural type.
topK: 16, | ||
}; | ||
|
||
const genAI = new GoogleGenerativeAI("AIzaSyDDqii7EVJbMgfC3vhp3ER2o-EYa9qOSQw"); |
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.
Do you guys use a linter/formatter on the CLI? Generally recommend prettier, but the line length and formatting here seems a bit long.
let result; | ||
try { | ||
const newPrompt = getPreamble() + prompt; | ||
logger.debug("Running prompt: " + newPrompt); |
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.
nit: prefer a template string (for all string concatenations in file.)
logger.debug(`Running prompt: ${newPrompt}`);
responseText.length > 0 && | ||
responseText[0] === "`" && | ||
responseText[responseText.length - 1] === "`" | ||
) { |
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.
The response you get is actually markdown. It seems like you're relying a bit too much on some specifics in markdown parsing.
The preamble says to give one command and one command only, but we know that might not necessarily be the case.
The ideal would be to AST parse the markdown and then dig out the content of the first codeblock / code snip.
I know that yanking in a markdown parsing lib is hefty, but parsing this out manually yourself is going to be a bit problematic and brittle.
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.
We already have marked
as a dependency - consider using it here to parse
responseText[responseText.length - 1] === "`" | ||
) { | ||
// Assume this is a single command with backticks on each side. | ||
const trimmedResponse = responseText.slice(1, responseText.length - 1); |
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.
Is there a way to filter or validate this command?
Best: Against a known table
Okay: Validate the format.
}; | ||
|
||
const genAI = new GoogleGenerativeAI("AIzaSyDDqii7EVJbMgfC3vhp3ER2o-EYa9qOSQw"); | ||
const model = genAI.getGenerativeModel({ model: "gemini-pro", generationConfig }); |
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.
Consider adding "gemini-pro" to "generationConfig"
|
||
const { GoogleGenerativeAI } = require("@google/generative-ai"); | ||
|
||
// TODO(christhompson): Do we have an endpoint that's open and doesn't require a project w/ billing? |
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.
Pretty sure everything is going to require an API Key. You can get one from AIStudio tho pretty easily.
@@ -0,0 +1,135 @@ | |||
Below are some sample commands. If you're not able to recommend a command, please refer me to https://firebase.google.com/docs/cli. |
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.
Is this file generated or did you hand craft this?
If you handcrafted it -- It's interesting for a demo, but it might make sense to have some sort of command metadata that lives alongside each and every command -- Then you can weave that metadata together into the prompt format.
Having a central file will be annoying to scale, as people will forget to update it.
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.
Looks like this includes the output of firebase --help
. If you move forward with productionizing this, we should at least add a script to package.json
to update this (and a CI test that fails if running that script generates any changes)
| **setup:emulators:database** | Downloads the database emulator. | | ||
| **setup:emulators:firestore** | Downloads the firestore emulator. | | ||
|
||
### App Distribution Commands |
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.
nit: It might make sense to say "Firebase App Distribution" (etc etc for all product names)
// TODO(christhompson): Figure out how to test this. | ||
const generationConfig = { | ||
maxOutputTokens: 200, | ||
temperature: 0.9, |
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.
Temp seems a little spicy here -- Did you experiment with a different values? My assumption is that if the temp needs to be this high, then we're likely not prompting enough.
@@ -138,6 +138,7 @@ export function load(client: any): any { | |||
client.functions.secrets.prune = loadCommand("functions-secrets-prune"); | |||
client.functions.secrets.set = loadCommand("functions-secrets-set"); | |||
client.help = loadCommand("help"); | |||
client.help = loadCommand("gemini-help"); |
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.
Not needed for now since this is a draft/POC, but I definitely think we'd want to start this off behind an experiment
Iniital version, works well with some caveats (see TODOs).
The API_KEY can be stored in a secret and used if we can get ~1 QPS quota for a project to hit Gemini.
Some outstanding work:
Replacing the current process with the one from Gemini's recommended command
Adding better suggestions for commands with required flags
Multi-word queries require quotes which is less than intuitive for the user
Testing instructions:
clone the repo
cd firebase-tools
git checkout gemini-help
npm install
npm link
firebase gemini:help "hello there" # Note: quotes required for multi-word queries currently
If PERMISSION_DENIED, chmod 755 the firebase file