Skip to content
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

Add additional step to pipeline to generate a metrics report #241

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MichaelClifford
Copy link
Collaborator

This PR adds a new step to the end of the pipeline,generate_metrics_report_op. The purpose of this step is to create a number of kfp Mertic artifacts that can help users easily compare the performance between different runs using the compare runs feature of the Data Science Pipeline UI.

eval/final/components.py Outdated Show resolved Hide resolved
@MichaelClifford MichaelClifford force-pushed the metrics branch 3 times, most recently from cad432f to 8a6be72 Compare December 19, 2024 21:50
@MichaelClifford
Copy link
Collaborator Author

@tumido updated the PR to use PVC's instead of interim output artifacts.

pipeline.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/mt_bench/components.py Outdated Show resolved Hide resolved
eval/mt_bench/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/final/components.py Outdated Show resolved Hide resolved
eval/mt_bench/components.py Outdated Show resolved Hide resolved
@MichaelClifford MichaelClifford force-pushed the metrics branch 3 times, most recently from 2508289 to 898b6f7 Compare January 9, 2025 02:20
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I have one last suggestion, otherwise LGTM. 👍 🙌

pipeline.py Outdated Show resolved Hide resolved
@MichaelClifford MichaelClifford force-pushed the metrics branch 3 times, most recently from 6259904 to 9458ad4 Compare January 17, 2025 21:31
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

It seems like you've forgot to include pipeline.py changes after rebase. As a result, the pvc_to_mt_bench_branch_op and pvc_to_mmlu_branch_op tasks are not used at all.

@MichaelClifford MichaelClifford force-pushed the metrics branch 3 times, most recently from e8904e4 to 665b632 Compare January 21, 2025 21:33
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Sorry to be annoying, just 2 last nits... otherwise LGTM 🙂 🙌

pipeline.py Outdated Show resolved Hide resolved
pipeline.py Outdated Show resolved Hide resolved
Comment on lines +467 to +469
output_mmlu_branch_task,
output_mt_bench_branch_task,
generate_metrics_report_task,
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry to be so stubborn here. 😄 I still think this is not a correct list of dependencies. I think it should be:

Suggested change
output_mmlu_branch_task,
output_mt_bench_branch_task,
generate_metrics_report_task,
output_model_task,
output_mt_bench_task,
output_mmlu_branch_task,
output_mt_bench_branch_task,
generate_metrics_report_task,

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.

3 participants