-
Couldn't load subscription status.
- Fork 5
Classification : retaining prediction and fetching data from s3 for model evaluation #359
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| """add additional columns to model evaluation table | ||
| Revision ID: 06a8bed9e7a6 | ||
| Revises: 72896bcc94da | ||
| Create Date: 2025-08-25 22:36:58.959768 | ||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| import sqlmodel.sql.sqltypes | ||
| from sqlalchemy.dialects import postgresql | ||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision = "06a8bed9e7a6" | ||
| down_revision = "72896bcc94da" | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade(): | ||
| op.drop_column("model_evaluation", "testing_file_id") | ||
| op.add_column( | ||
| "model_evaluation", | ||
| sa.Column( | ||
| "test_data_s3_url", sqlmodel.sql.sqltypes.AutoString(), nullable=False | ||
| ), | ||
| ) | ||
| op.add_column( | ||
| "model_evaluation", | ||
| sa.Column( | ||
| "prediction_data_s3_url", sqlmodel.sql.sqltypes.AutoString(), nullable=True | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def downgrade(): | ||
| op.drop_column("model_evaluation", "prediction_data_s3_url") | ||
| op.drop_column("model_evaluation", "test_data_s3_url") | ||
| op.add_column( | ||
| "model_evaluation", | ||
| sa.Column("testing_file_id", sa.VARCHAR(), autoincrement=False, nullable=True), | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| ModelEvaluationPublic, | ||
| ) | ||
| from app.core.db import engine | ||
| from app.core.cloud import AmazonCloudStorage | ||
| from app.core.finetune.evaluation import ModelEvaluator | ||
| from app.utils import get_openai_client, APIResponse | ||
| from app.api.deps import CurrentUserOrgProject, SessionDep | ||
|
|
@@ -35,14 +36,18 @@ | |
| def run_model_evaluation( | ||
| eval_id: int, | ||
| current_user: CurrentUserOrgProject, | ||
| client: OpenAI, | ||
| ): | ||
| start_time = time.time() | ||
| logger.info( | ||
| f"[run_model_evaluation] Starting | eval_id={eval_id}, project_id={current_user.project_id}" | ||
| ) | ||
|
|
||
| with Session(engine) as db: | ||
| client = get_openai_client( | ||
| db, current_user.organization_id, current_user.project_id | ||
| ) | ||
| storage = AmazonCloudStorage(current_user) | ||
|
|
||
| try: | ||
| model_eval = update_model_eval( | ||
| session=db, | ||
|
|
@@ -53,9 +58,10 @@ def run_model_evaluation( | |
|
|
||
| evaluator = ModelEvaluator( | ||
| model_name=model_eval.model_name, | ||
| testing_file_id=model_eval.testing_file_id, | ||
| test_data_s3_url=model_eval.test_data_s3_url, | ||
| system_prompt=model_eval.system_prompt, | ||
| client=client, | ||
| storage=storage, | ||
| ) | ||
| result = evaluator.run() | ||
|
|
||
|
|
@@ -64,7 +70,9 @@ def run_model_evaluation( | |
| eval_id=eval_id, | ||
| project_id=current_user.project_id, | ||
| update=ModelEvaluationUpdate( | ||
| score=result, status=ModelEvaluationStatus.completed | ||
| score=result["evaluation_score"], | ||
| prediction_data_s3_url=result["prediction_data_s3_url"], | ||
| status=ModelEvaluationStatus.completed, | ||
| ), | ||
| ) | ||
|
|
||
|
|
@@ -111,7 +119,8 @@ def evaluate_models( | |
| """ | ||
| client = get_openai_client( | ||
| session, current_user.organization_id, current_user.project_id | ||
| ) | ||
| ) # keeping this here for checking if the user's validated OpenAI key is present or not, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. didn't understand this part There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The client that actually carries out the OpenAI functions will be initialized inside the background task. The client being created in the router isn’t passed to the background task, because it’s generally not a good practice to pass objects into background tasks, and can cause same error as when passing router session to bg task. However, we still want to validate upfront that the user invoking this endpoint has a valid OpenAI key stored in our database. If we leave that check only to the background task, the user wouldn’t immediately know if their key is invalid or not present. That’s why the client is initialized in the router for validation purposes, while the actual client used to perform operations is re-initialized inside the background task. |
||
| # even though the client will be initialized separately inside the background task | ||
|
|
||
| if not request.fine_tuning_ids: | ||
| logger.error( | ||
|
|
@@ -150,9 +159,7 @@ def evaluate_models( | |
| f"[evaluate_model] Created evaluation for fine_tuning_id {job_id} with eval ID={model_eval.id}, project_id:{current_user.project_id}" | ||
| ) | ||
|
|
||
| background_tasks.add_task( | ||
| run_model_evaluation, model_eval.id, current_user, client | ||
| ) | ||
| background_tasks.add_task(run_model_evaluation, model_eval.id, current_user) | ||
|
|
||
| return APIResponse.success_response( | ||
| {"message": "Model evaluation(s) started successfully", "data": evals} | ||
|
|
||
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.
keep this consistent with other migrations, somewhere i saw it was just data_url and here it is data_s3_url
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.
Yes I have turned all the "data_url" to "data_s3_url" , and I have made sure that this is consistent