Skip to content

Conversation

@rasmi
Copy link
Contributor

@rasmi rasmi commented Dec 30, 2025

The Variables refactor (#859) changed the structure of VariableConfig objects but didn't include migration for existing experiments, causing them to crash when loading (see #909).

Old format (pre-v19):

{
  variableNames: string[],
  schema: TSchema,
  seedStrategy: SeedStrategy,
  values: string[]
}

New format (v19+):

{
  definition: { name, description, schema },
  scope: VariableScope,
  shuffleConfig: ShuffleConfig,
  values: string[],
  numToSelect?: number,
  expandListToSeparateVariables?: boolean
}

Changes

  • utils/src/variables.legacy.utils.ts - Migration utilities with migrateVariableConfig() and migrateVariableConfigs()
  • utils/src/variables.utils.ts - extractVariablesFromVariableConfigs() now auto-migrates old configs as a runtime fallback
  • functions/src/migrations/migrate-variable-configs.ts - One-time Firestore migration script

Running the Migration

Option 1: One-time database migration

cd functions

# Preview changes first
npm run migrate:variable-configs:dry-run

# Apply changes
npm run migrate:variable-configs

For production, set GOOGLE_APPLICATION_CREDENTIALS or use gcloud auth application-default login.

Option 2: Runtime migration (automatic fallback)

Old configs are automatically migrated at runtime when extractVariablesFromVariableConfigs() is called. This ensures experiments load even if the database migration hasn't been run, but the one-time script may be preferable for a full fix.

@rasmi rasmi force-pushed the variables-migration branch from 08e462b to 1ba7ee0 Compare January 6, 2026 19:25

for (const config of configs) {
const result = migrateVariableConfig(config);
if (result !== null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that this is where the null comes in handy. It's strange to me to see error cases swallowed like this. But maybe it's OK. I'm new here. 😅


function migrateVariableConfig(
config: LegacyOrNewConfig,
): VariableConfig | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: It's strange to me to see a null in general and especially as a response indicating an error. What's the current practice in this code base?

I'm persuadable that this is the way to do it. Just noting that I would generally expect something like a CustomError to be thrown.

@rasmi
Copy link
Contributor Author

rasmi commented Jan 9, 2026

FYI! I don't think this needs to be merged necessarily (I think you can just pull the script in and run the migration as a one-off without necessarily deploying the backport code)

@rasmi rasmi marked this pull request as draft January 9, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants