Skip to content

User experiment migration tool #1724

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

Open
wants to merge 11 commits into
base: smh/exp-pipeline-migration
Choose a base branch
from

Conversation

stephherbers
Copy link
Contributor

@stephherbers stephherbers commented Jun 6, 2025

Description

part of #1664
merges into #1721
After pressing banner button to migrate experiment. it navigates to the newly created chatbot
Screenshot 2025-06-06 at 3 46 20 PM

User Impact

users are able to migrate LLM and Assistant type experiments themselves

Demo

Docs and Changelog

yes to changelog-- the docs have information about the transition, but I may add something specifically for this migration

@stephherbers stephherbers requested review from snopoke and SmittieC June 6, 2025 19:50
@stephherbers stephherbers changed the base branch from main to smh/exp-pipeline-migration June 6, 2025 19:51
@stephherbers stephherbers changed the title Smh/migration tool User experiment migration tool Jun 6, 2025
@stephherbers stephherbers marked this pull request as ready for review June 6, 2025 19:53
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 19.56522% with 37 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/pipelines/helper.py 13.79% 25 Missing ⚠️
apps/experiments/views/experiment.py 29.41% 12 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

"""
try:
experiment = Experiment.objects.get(id=experiment_id)
call_command("migrate_nonpipeline_to_pipeline_experiments", experiment_id=experiment_id, skip_confirmation=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

A preferable alternative to calling the command would be to extract the logic into functions which can be used here and in the command. That also makes it easier to test.


# TODO: function is temporary and can be deleted after the exp -> chatbot transition is complete
def convert_non_pipeline_experiment_to_pipeline(experiment):
if experiment.assistant:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra paranoid validation that we can actually migrate this before we do

Suggested change
if experiment.assistant:
if experiment.pipeline:
raise ValueError(f"Experiment already has a pipeline attached: {experiment.id}")
elif experiment.assistant:

Copy link
Contributor

@snopoke snopoke left a comment

Choose a reason for hiding this comment

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

I think this needs to be updated with the changes from the parent branch

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.

4 participants