Preparing for genkit 0.9.0 #7907

ifielker · 2024-11-05T17:35:07Z

preparing for genkit version 0.9.0+

src/init/features/genkit/index.ts

pavelgj · 2024-11-05T19:19:22Z

src/init/features/genkit/index.ts

+  if (semver.gte(genkitVersion, "1.0.0")) {
+    // We don't know about this version.
+    throw new FirebaseError(
+      "Please update your firebase-tools. Alternatively you can set a GENKIT_DEV_VERSION environment variable to choose a genkit version < 1.0.0",
+    );
+  } else if (semver.gte(genkitVersion, "0.6.0")) {
+    sampleVersion = "0.9.0";
+    useInit = false;
+  } else {
+    sampleVersion = "";
+    useInit = true;
+  }


I'm thinking instead of doing this we should just hardcode the version that firebase init genkit installs to be ^0.9. The template here is something we'll have to maintain, and update versions for each major release.

The whole reason we're doing this PR is because we don't want things to break and have no solution.
With this setup, I can submit this PR now, people can update to it, and it will continue to allow them to install 0.5.17 (the latest) up until we release 0.9.0 and then it will install 0.9.0.
If I don't do this, then I can't submit this PR until sometime after we release 0.9.0 and there will be a period where it is just broken with no "just update to the latest firebase-tools" fix.
It would also not notify people to update their firebase-tools because a newer version (>=1.0.0) is available.
If we had this kind of logic in place from the beginning, then nothing would break when we release 0.9.0. So I'm a bit confused why you want me to remove it. People do not necessarily update their firebase-tools unless they have a reason to do so.
I wouldn't want them to be "stuck" getting firebase init genkit installs for 0.9.0 when the latest version of genkit is 1.5.7 and there have been breaking change and the docs are for 1.5.7 and they just have a bad time trying to figure out what's going on and end up not liking genkit because it's "broken". I'd much rather have them getting a message saying "you need to update your firebase-tools, or explicitly pick an "old" version of genkit to install". If we do it this way then, we can remove support for older versions and add support for newer versions and not be coupled to the release of genkit versions without breaking anything. I feel very strongly that this is a good solution.

supporting 0.5 is fine...
I'm more thinking about the semver.gte(genkitVersion, "1.0.0") case. I think that if version is semver.gte(genkitVersion, "0.6.0") we pin it to 0.9. until we update the firebase cli in the future.

pavelgj · 2024-11-06T15:55:23Z

src/init/features/genkit/index.ts

+      plugin: "@genkit-ai/vertexai",
+      package: `@genkit-ai/vertexai@${genkitVersion}`,
+    },
+    ollama: {


please remove ollama as an option here -- it requires many more setup/deployment steps that we can't cover in this init flow.

pavelgj · 2024-11-06T15:58:38Z

src/init/features/genkit/index.ts

+/**
+ * Updates tsconfig.json with required flags for Genkit.
+ */
+async function updateTsConfig(nonInteractive: boolean, projectDir: string) {


Please see if we can just add skipLibCheck: true to https://github.com/firebase/firebase-tools/blob/master/templates/init/functions/typescript/tsconfig.json
This way we can drop the updateTsConfig entirely here.

In the interest of getting this submitted quickly because genkit 0.9.0 release is approaching, I could leave it like this for now because it works, and then in a second PR, we could look at having a discussion with the functions folks, and simplify the code if possible. Is that ok?

pavelgj · 2024-11-06T15:59:42Z

src/init/features/genkit/index.ts

+/**
+ * Updates package.json with Genkit-expected fields.
+ */
+async function updatePackageJson(nonInteractive: boolean, projectDir: string) {


I don't think this is necessary for firebase case... this can be dropped entirely.

Recall that even with instructions, I had a bad time trying to figure out why the UI was "broken" and spent quite some time trying to fix "step 7" before moving on to "step 8". And the solution was "you need to run step 7, then not open the UI yet, then run step 8, then go back and open the UI from step 7". That's why I added package.json scripts like genkit:start that runs step 7 and step 8 in order together, so there's no confusion. (and also genit:stop which just stops the ui). That way people are not left in a "ok, it's installed, now what?" state, but rather they have an easy way to get everything going so they can focus on playing with a working genkit and not on trying to "fix a broken step 7" like I was.

pavelgj · 2024-11-06T16:01:29Z

src/init/features/genkit/index.ts

+        logger.info("Start the Genkit developer experience by running:");
+        logger.info(`    cd ${setup.functions.source} && npm run genkit:start`);


I wonder if we need these... drop maybe?

We absolutely need these. I know you know all the details about how to start genkit and get it running and the right order to do those things, but the genkit newbie does not (i.e. people like me who spent too much trying with a "broken" genkit trying to debug what was wrong.)

pavelgj · 2024-11-11T19:17:05Z

src/init/features/genkit/index.ts

+      "genkit:start": "genkit ui:start && GENKIT_ENV=dev tsx --watch src/genkit-sample.ts",
+      "genkit:stop": "genkit ui:stop",


Suggested change

"genkit:start": "genkit ui:start && GENKIT_ENV=dev tsx --watch src/genkit-sample.ts",

"genkit:stop": "genkit ui:stop",

"genkit:start": "genkit start -- tsx --watch src/genkit-sample.ts"

pavelgj · 2024-11-11T19:22:19Z

templates/genkit/firebase.0.9.0.template

+
+		// Handle the response from the model API. In this sample, we just
+		// convert it to a string, but more complicated flows might coerce the
+		// response into structured output or chain the response into another
+		// LLM call, etc.
+    return llmResponse.text;


Suggested change

// Handle the response from the model API. In this sample, we just

// convert it to a string, but more complicated flows might coerce the

// response into structured output or chain the response into another

// LLM call, etc.

return llmResponse.text;

// Handle the response from the model API. In this sample, we just

// convert it to a string, but more complicated flows might coerce the

// response into structured output or chain the response into another

// LLM call, etc.

return llmResponse.text;

I'm not seeing that in the current version of the template. I might have fixed it along the way (possibly it was tabs before?)

pavelgj · 2024-11-11T19:22:58Z

templates/genkit/firebase.0.9.0.template

+// Import the Genkit core libraries and plugins.
+import {genkit} from 'genkit';
+import {logger} from 'genkit/logging';
+$GENKIT_CONFIG_IMPORTS
+$GENKIT_MODEL_IMPORT
+
+// From the Firebase plugin, import the functions needed to deploy flows using
+// Cloud Functions.
+import {firebaseAuth} from "@genkit-ai/firebase/auth";
+import {onFlow} from "@genkit-ai/firebase/functions";
+
+// Log debug output to tbe console.
+logger.setLogLevel("debug");


Suggested change

// Import the Genkit core libraries and plugins.

import {genkit} from 'genkit';

import {logger} from 'genkit/logging';

$GENKIT_CONFIG_IMPORTS

$GENKIT_MODEL_IMPORT

// From the Firebase plugin, import the functions needed to deploy flows using

// Cloud Functions.

import {firebaseAuth} from "@genkit-ai/firebase/auth";

import {onFlow} from "@genkit-ai/firebase/functions";

// Log debug output to tbe console.

logger.setLogLevel("debug");

// Import the Genkit core libraries and plugins.

import {genkit} from 'genkit';

$GENKIT_CONFIG_IMPORTS

$GENKIT_MODEL_IMPORT

// From the Firebase plugin, import the functions needed to deploy flows using

// Cloud Functions.

import {firebaseAuth} from "@genkit-ai/firebase/auth";

import {onFlow} from "@genkit-ai/firebase/functions";

pavelgj · 2024-11-12T01:14:48Z

src/init/features/genkit/index.ts

+
+  if (semver.gte(genkitVersion, "1.0.0")) {
+    // We don't know about this version.
+    throw new FirebaseError(


as discussed please change this to a warning

codecov-commenter · 2024-11-12T01:56:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.37%. Comparing base (a177fe0) to head (bf7756f).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7907   +/-   ##
=======================================
  Coverage   51.36%   51.37%           
=======================================
  Files         422      422           
  Lines       29344    29344           
  Branches     5996     5996           
=======================================
+ Hits        15074    15076    +2     
+ Misses      12915    12914    -1     
+ Partials     1355     1354    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

done

f02168c

pavelgj reviewed Nov 5, 2024

View reviewed changes

src/init/features/genkit/index.ts Outdated Show resolved Hide resolved

Ingrid Fielker added 2 commits November 5, 2024 14:44

install genkit-cli too

f91f15c

lint

a869f88

pavelgj reviewed Nov 7, 2024

View reviewed changes

Ingrid Fielker and others added 2 commits November 8, 2024 15:19

various fixes

596508e

Merge branch 'master' into if-genkit1

58e8c41

pavelgj reviewed Nov 11, 2024

View reviewed changes

ifielker and others added 3 commits November 11, 2024 15:45

Merge branch 'master' into if-genkit1

f107152

review changes

f756652

and the comma

4e41c55

pavelgj reviewed Nov 12, 2024

View reviewed changes

change to warning

bf7756f

pavelgj approved these changes Nov 12, 2024

View reviewed changes

ifielker merged commit eb438ad into master Nov 12, 2024
44 of 45 checks passed

ifielker deleted the if-genkit1 branch November 12, 2024 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparing for genkit 0.9.0 #7907

Preparing for genkit 0.9.0 #7907

ifielker commented Nov 5, 2024

pavelgj Nov 5, 2024

ifielker Nov 8, 2024

pavelgj Nov 11, 2024 •

edited

Loading

pavelgj Nov 6, 2024

ifielker Nov 8, 2024

pavelgj Nov 6, 2024

ifielker Nov 8, 2024

pavelgj Nov 6, 2024

ifielker Nov 8, 2024

pavelgj Nov 6, 2024

ifielker Nov 8, 2024

pavelgj Nov 11, 2024

pavelgj Nov 11, 2024

ifielker Nov 11, 2024

pavelgj Nov 11, 2024

ifielker Nov 11, 2024

pavelgj Nov 12, 2024

ifielker Nov 12, 2024

codecov-commenter commented Nov 12, 2024

		logger.info("Start the Genkit developer experience by running:");
		logger.info(` cd ${setup.functions.source} && npm run genkit:start`);

		"genkit:start": "genkit ui:start && GENKIT_ENV=dev tsx --watch src/genkit-sample.ts",
		"genkit:stop": "genkit ui:stop",

	"genkit:start": "genkit ui:start && GENKIT_ENV=dev tsx --watch src/genkit-sample.ts",
	"genkit:stop": "genkit ui:stop",
	"genkit:start": "genkit start -- tsx --watch src/genkit-sample.ts"

Preparing for genkit 0.9.0 #7907

Preparing for genkit 0.9.0 #7907

Conversation

ifielker commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelgj Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 12, 2024

Codecov Report

pavelgj Nov 11, 2024 •

edited

Loading