-
Notifications
You must be signed in to change notification settings - Fork 14
Experiment Migration Management Command #1721
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Outdated
Show resolved
Hide resolved
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Outdated
Show resolved
Hide resolved
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.
Looking good! This is exciting.
I presume this logic is going to be re-used for the DIY button that users can click?
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Outdated
Show resolved
Hide resolved
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Outdated
Show resolved
Hide resolved
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Show resolved
Hide resolved
apps/experiments/management/commands/migrate_nonpipeline_to_pipeline_experiments.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Simon Kelly <[email protected]>
Co-authored-by: Simon Kelly <[email protected]>
Co-authored-by: Simon Kelly <[email protected]>
…y don't have one then working version
if experiment_id: | ||
query &= Q(id=experiment_id) |
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.
in this instance you could skip the rest of the queries and just fetch that one experiment right? Or maybe also check that it's a working version and also fetch the default version if there is one - but either way I think it should be handles differently because the rest of the queries and filters don't make sense if you're already filtering down to a single object.
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.
Looking good.
experiments_to_convert = Experiment.objects.filter(id=experiment.id).select_related( | ||
"team", "assistant", "llm_provider", "llm_provider_model" | ||
) |
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.
You could just do the select_related
in the query on line 60 and then you wouldn't have to re-fetch this here.
You could also avoid the extra count later by assigning experiment_count = 1
experiments_to_convert = Experiment.objects.filter(id__in=combined_ids).select_related( | ||
"team", "assistant", "llm_provider", "llm_provider_model" | ||
) |
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.
It may be that loading all of these into memory at once could be an issue so it would be prudent to use an iterator:
experiments_to_convert = Experiment.objects.filter(id__in=combined_ids).select_related( | |
"team", "assistant", "llm_provider", "llm_provider_model" | |
) | |
experiments_to_convert_queryset = Experiment.objects.filter(id__in=combined_ids).select_related( | |
"team", "assistant", "llm_provider", "llm_provider_model" | |
) | |
# do this here since you can't do a count on an iterator | |
experiment_count = experiments_to_convert_queryset.count() | |
experiments_to_convert = experiments_to_convert_queryset.iterator(20) |
Description
resolves #1664
second part here: #1724
User Impact
when run, all assistant and llm experiments will be converted to pipeline experiments
Demo
n/a
Docs and Changelog
n/a until the tool for users to do on their own is implemented