Claude: Setup & Minor fixes with it #380
Conversation
WalkthroughConverts the fine-tuning CSV flow to an async file-upload endpoint storing CSVs as Documents, creates per-split FineTuningJobs, returns document_id, adds automatic ModelEvaluation creation + background evaluation queuing on job completion, centralizes OpenAI error handling, adds a cancelled fine-tune status, adds CLAUDE.md, and updates tests to use moto/S3 mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as FastAPI Route
participant Storage as Cloud Storage
participant DB as Database
participant Worker as BackgroundTasks
participant Eval as Evaluation Orchestrator
rect rgb(238,245,255)
note right of Client: CSV upload & job creation
Client->>API: POST /fine-tuning (multipart: file, base_model, split_ratio, system_prompt)
API->>Storage: Upload CSV object
Storage-->>API: Object key / URL
API->>DB: Create Document (CSV) and FineTuningJob(s)
API-->>Client: { document_id, jobs[] }
end
rect rgb(245,255,238)
note right of API: Status refresh -> auto-evaluation
Client->>API: POST /fine-tuning/{id}/refresh
API->>DB: Update job status (e.g., completed)
alt transitioned to completed AND no active evaluation
API->>DB: Create ModelEvaluation (pending)
API->>Worker: queue run_model_evaluation(eval_id)
Worker->>Eval: run_model_evaluation(eval_id)
else otherwise
API-->>Client: No evaluation queued
end
API-->>Client: Updated job status
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ged from in progress to completed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/core/finetune/evaluation.py (2)
142-147: Timeout path can loop forever; increment attempts on latency exceed.
continuewithoutattempt += 1can create an infinite retry if latency keeps exceeding.- if elapsed_time > self.max_latency: - logger.warning( - f"[generate_predictions] Timeout exceeded for prompt {idx}/{total_prompts}. Retrying..." - ) - continue + if elapsed_time > self.max_latency: + attempt += 1 + logger.warning( + f"[generate_predictions] Timeout exceeded for prompt {idx}/{total_prompts} " + f"(attempt {attempt}/{self.retries}). Retrying..." + ) + if attempt == self.retries: + predictions.append("timeout") + logger.error( + f"[generate_predictions] Max retries reached after latency timeouts for " + f"prompt {idx}/{total_prompts}. Appending 'timeout'." + ) + break + continue
79-85: Guard against empty label sets.If the CSV has no labels,
next(iter(self.allowed_labels))will raise. Fail fast with a clear error.self.prompts = prompts self.y_true = labels self.allowed_labels = set(labels) self.query_col = query_col self.label_col = label_col + if not self.allowed_labels: + raise ValueError("[ModelEvaluator.load_labels_and_prompts] No labels found in CSV.")backend/app/api/routes/responses.py (1)
75-82: Bug: usingresultsto build itself yields empty/duplicated chunks.You’re iterating over
resultsinstead of the tool call’s hits. This currently returns an empty list on first call and can double-append later.def get_file_search_results(response): results: list[FileResultChunk] = [] for tool_call in response.output: if tool_call.type == "file_search_call": - results.extend( - [FileResultChunk(score=hit.score, text=hit.text) for hit in results] - ) + results.extend( + FileResultChunk(score=hit.score, text=hit.text) + for hit in getattr(tool_call, "results", []) # defensive + ) return resultsbackend/app/api/routes/threads.py (1)
138-140: Use dict access for optional field.
requestis a dict; userequest.get(...)instead ofgetattr(...).- "endpoint": getattr(request, "endpoint", "some-default-endpoint"), + "endpoint": request.get("endpoint", "some-default-endpoint"),
🧹 Nitpick comments (5)
backend/app/core/finetune/evaluation.py (1)
183-187: Sanitize filename for S3 key safety.Model names can include chars that don’t play well as path segments. Slugify before composing the key.
- filename = f"predictions_{self.fine_tuned_model}_{unique_id}.csv" + safe_model = re.sub(r"[^a-zA-Z0-9_.-]+", "-", str(self.fine_tuned_model)) + filename = f"predictions_{safe_model}_{unique_id}.csv"Add at file top:
import rebackend/app/utils.py (1)
52-56: Make OpenAI error parsing more robust.Errors can be shaped as
{"error": {"message": ...}}or exposemessagedirectly. Cover both.-def handle_openai_error(e: openai.OpenAIError) -> str: - """Extract error message from OpenAI error.""" - if hasattr(e, "body") and isinstance(e.body, dict) and "message" in e.body: - return e.body["message"] - return str(e) +def handle_openai_error(e: openai.OpenAIError) -> str: + """Extract a human-readable error message from OpenAI errors.""" + body = getattr(e, "body", None) + if isinstance(body, dict): + # Newer shapes often nest under { "error": { "message": ... } } + err = body.get("error") + if isinstance(err, dict) and "message" in err: + return str(err["message"]) + if "message" in body: + return str(body["message"]) + # Fallbacks + msg = getattr(e, "message", None) + return str(msg or e)backend/app/api/routes/threads.py (2)
262-266: Normalize OpenAI error handling in polling.Use
handle_openai_error(e)for consistency with other handlers.- except openai.OpenAIError as e: - status = "failed" - error = str(e) + except openai.OpenAIError as e: + status = "failed" + error = handle_openai_error(e)
43-44: Set a timeout on callback POST to avoid hanging workers.Add a conservative timeout (e.g., 10s).
- response = session.post(callback_url, json=data) + response = session.post(callback_url, json=data, timeout=10)backend/app/api/routes/fine_tuning.py (1)
229-233: Preserve extension and use a stable prefix for storage path.Improves organization and downstream content-type handling.
- object_store_url = storage.put(file, Path(str(document_id))) + object_store_url = storage.put( + file, Path("documents") / f"{document_id}.csv" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(1 hunks)backend/app/api/routes/fine_tuning.py(5 hunks)backend/app/api/routes/responses.py(1 hunks)backend/app/api/routes/threads.py(1 hunks)backend/app/core/finetune/evaluation.py(1 hunks)backend/app/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
backend/app/api/routes/threads.py (1)
backend/app/utils.py (1)
handle_openai_error(52-56)
backend/app/core/finetune/evaluation.py (1)
backend/app/utils.py (1)
handle_openai_error(52-56)
backend/app/api/routes/responses.py (1)
backend/app/utils.py (3)
APIResponse(28-49)mask_string(166-175)handle_openai_error(52-56)
backend/app/api/routes/fine_tuning.py (10)
backend/app/models/fine_tuning.py (4)
FineTuningJobCreate(28-51)FineTuningJobPublic(103-119)FineTuningUpdate(92-100)FineTuningStatus(13-17)backend/app/models/document.py (1)
Document(20-40)backend/app/models/model_evaluation.py (2)
ModelEvaluationBase(21-26)ModelEvaluationStatus(14-18)backend/app/core/cloud/storage.py (4)
get_cloud_storage(262-279)client(30-42)put(120-122)put(150-177)backend/app/crud/document.py (1)
DocumentCrud(14-135)backend/app/utils.py (2)
get_openai_client(178-208)handle_openai_error(52-56)backend/app/crud/fine_tuning.py (4)
create_fine_tuning_job(20-66)fetch_by_id(88-104)update_finetune_job(165-182)fetch_by_document_id(107-127)backend/app/crud/model_evaluation.py (2)
create_model_evaluation(20-58)fetch_active_model_evals(150-170)backend/app/core/finetune/preprocessing.py (1)
DataPreprocessor(17-157)backend/app/api/routes/model_evaluation.py (1)
run_model_evaluation(49-112)
🪛 Ruff (0.13.1)
backend/app/api/routes/fine_tuning.py
222-222: Local variable client is assigned to but never used
Remove assignment to unused variable client
(F841)
🔇 Additional comments (5)
backend/app/core/finetune/evaluation.py (1)
14-14: Centralizing error handling import looks good.backend/app/api/routes/fine_tuning.py (4)
296-298: Response payload addition looks good (document_id + jobs).
346-355: Nice: detect transition to completed before queuing evaluation.
371-385: Auto‑queueing evaluation is solid; background task is decoupled.
236-241: Don't add project_id here — DocumentCrud.update already sets and enforces it.
DocumentBase defines a non-nullable project_id and DocumentCrud.update sets document.project_id = self.project_id when missing (and checks mismatches). See backend/app/models/document.py (DocumentBase.project_id, DocumentCrud.update).Likely an incorrect or invalid review comment.
| # Validate file is CSV | ||
| if not file.filename.lower().endswith(".csv"): | ||
| raise HTTPException(status_code=400, detail="File must be a CSV file") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick
Also validate content-type for CSV.
Some browsers send non-.csv names; check MIME too.
- if not file.filename.lower().endswith(".csv"):
+ if not file.filename.lower().endswith(".csv") and file.content_type != "text/csv":
raise HTTPException(status_code=400, detail="File must be a CSV file")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Validate file is CSV | |
| if not file.filename.lower().endswith(".csv"): | |
| raise HTTPException(status_code=400, detail="File must be a CSV file") | |
| # Validate file is CSV | |
| if not file.filename.lower().endswith(".csv") and file.content_type != "text/csv": | |
| raise HTTPException(status_code=400, detail="File must be a CSV file") |
🤖 Prompt for AI Agents
In backend/app/api/routes/fine_tuning.py around lines 218 to 221, the code only
checks the filename suffix; add a MIME-type check on the uploaded file (use
file.content_type) and validate it against common CSV MIME types (e.g.
"text/csv", "application/csv", "application/vnd.ms-excel", and optionally
"text/plain") before proceeding; if the content_type is not one of the allowed
values, raise HTTPException(status_code=400, detail="File must be a CSV file");
keep the existing filename extension check as an additional safeguard.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_fine_tuning.py (1)
62-65: Remove unused temp JSONL creation.
Delete the for-loop that writes "/tmp/train.jsonl" and "/tmp/test.jsonl" in backend/app/tests/api/routes/test_fine_tuning.py:62-65 — a repo search shows no other references and the test already posts an in-memory CSV via multipart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_fine_tuning.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/tests/api/routes/test_fine_tuning.py (5)
backend/app/tests/utils/utils.py (1)
get_document(116-137)backend/app/models/fine_tuning.py (2)
Fine_Tuning(54-89)FineTuningStatus(13-17)backend/app/models/model_evaluation.py (2)
ModelEvaluation(38-85)ModelEvaluationStatus(14-18)backend/app/core/cloud/storage.py (3)
client(30-42)put(120-122)put(150-177)backend/app/tests/conftest.py (2)
db(24-41)user_api_key_header(77-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (11)
backend/app/tests/api/routes/test_fine_tuning.py (11)
1-2: LGTM!The new imports for
osandioare correctly added to support AWS credentials setup and file handling respectively.
4-6: LGTM!The imports for
mock_awsandboto3are correctly added to enable AWS mocking functionality in the tests.
10-17: LGTM!The model imports are appropriately updated to include the new enums and model classes needed for the reworked test structure.
19-27: LGTM!The AWS credentials fixture properly sets up the required environment variables for moto mocking. The configuration aligns with the test requirements.
45-45: LGTM!Adding the
aws_credentialsfixture to the test class is necessary for the AWS mocking functionality.
47-56: LGTM!The
@mock_awsdecorator and S3 bucket setup are correctly implemented for the mocked AWS environment.
58-60: LGTM!The test CSV content is appropriately structured for the file upload testing scenario.
66-95: LGTM!The mocking setup is well-structured and properly isolates the dependencies. The simplified patching approach with
get_cloud_storage,get_openai_client, andprocess_fine_tuning_jobprovides clean separation of concerns for testing the API endpoint functionality.
102-105: LGTM!The assertions correctly validate the new response structure that includes
document_idandjobsfields, aligning with the updated API behavior described in the AI summary.
106-108: LGTM!The verification that the background task is called three times (once per split ratio) correctly validates the multi-job creation functionality.
112-118: LGTM!The database assertions properly validate that:
- Three Fine_Tuning jobs are created in the database
- All jobs have "pending" status (since background processing is mocked)
- Split ratios match the expected values [0.5, 0.7, 0.9]
This aligns with the test's input parameters and expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/tests/api/routes/test_fine_tuning.py (3)
19-28: Consider using more secure test credentials.While these are test credentials for moto mocking, using generic "testing" values for all AWS credentials could potentially mask issues if real credentials accidentally leak through. Consider using more descriptive test values or environment-specific prefixes.
- os.environ["AWS_ACCESS_KEY_ID"] = "testing" - os.environ["AWS_SECRET_ACCESS_KEY"] = "testing" - os.environ["AWS_SECURITY_TOKEN"] = "testing" - os.environ["AWS_SESSION_TOKEN"] = "testing" + os.environ["AWS_ACCESS_KEY_ID"] = "test_access_key_id" + os.environ["AWS_SECRET_ACCESS_KEY"] = "test_secret_key" + os.environ["AWS_SECURITY_TOKEN"] = "test_security_token" + os.environ["AWS_SESSION_TOKEN"] = "test_session_token"
112-118: String comparison for enum value should use the enum directly.Line 115 compares
job.status(which should be aFineTuningStatusenum) against the string"pending". For type safety and consistency, use the enum value directly.- assert ( - job.status == "pending" - ) # Since background processing is mocked, status remains pending + assert ( + job.status == FineTuningStatus.pending + ) # Since background processing is mocked, status remains pending
204-206: Use enum .value instead of hard-coded string for status assertion
Replaceassert job["status"] == "pending"withassert job["status"] == FineTuningStatus.pending.value— FineTuningStatus is a str Enum (value "pending"), so use .value to avoid a magic string and match other tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_fine_tuning.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/tests/api/routes/test_fine_tuning.py (6)
backend/app/tests/utils/test_data.py (1)
create_test_fine_tuning_jobs(133-160)backend/app/tests/utils/utils.py (1)
get_document(116-137)backend/app/models/fine_tuning.py (2)
Fine_Tuning(54-89)FineTuningStatus(13-17)backend/app/models/model_evaluation.py (2)
ModelEvaluation(38-85)ModelEvaluationStatus(14-18)backend/app/core/cloud/storage.py (5)
client(30-42)put(120-122)put(150-177)get_signed_url(135-137)get_signed_url(217-242)backend/app/tests/conftest.py (2)
db(24-41)user_api_key_header(77-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (8)
backend/app/tests/api/routes/test_fine_tuning.py (8)
1-17: LGTM! Well-organized imports for AWS moto and model testing.The imports correctly introduce AWS moto for S3 mocking, boto3 for AWS operations, and the necessary model classes including
FineTuningStatusandModelEvaluationStatusenums. The configuration import fromapp.core.configis appropriately placed.
66-83: Good use of nested context managers for mocking.The nested patching approach correctly isolates the cloud storage, OpenAI client, and background processing. This ensures proper test isolation and prevents actual API calls.
106-108: Verify background task invocation count matches expectations.The test correctly verifies that
process_fine_tuning_jobis called 3 times for 3 split ratios. This ensures the background processing is triggered appropriately.
208-213: LGTM! Well-structured test class for auto-evaluation triggering.The new
TestAutoEvaluationTriggerclass is properly structured with comprehensive patching of external dependencies. The class docstring clearly describes its purpose.
215-277: Excellent test for successful auto-evaluation trigger.This test comprehensively verifies:
- Status transition from running to completed
- Automatic model evaluation creation
- Background task invocation
- Proper evaluation record creation with correct associations
The test setup properly includes all required fields (
test_data_s3_object,system_prompt) needed for model evaluation.
278-353: Well-designed test for preventing duplicate evaluations.This test correctly verifies that no new evaluation is created when one already exists for the fine-tuning job. The setup properly creates an existing evaluation with all required fields.
354-413: Comprehensive test coverage for non-completion status changes.This test thoroughly covers multiple scenarios:
- Transition from pending to running (should not trigger evaluation)
- Transition from running to failed (should not trigger evaluation)
Both cases correctly assert that
mock_run_model_evaluationis not called.
414-470: Good edge case coverage for already completed jobs.This test ensures that refreshing an already completed job doesn't trigger a redundant evaluation. This is important for idempotency and preventing duplicate work.
| # Setup S3 bucket for moto | ||
| s3 = boto3.client("s3", region_name="us-east-1") | ||
| s3.create_bucket(Bucket="test-bucket") | ||
|
|
||
| # Create a test CSV file content | ||
| csv_content = "prompt,label\ntest1,label1\ntest2,label2\ntest3,label3" | ||
|
|
||
| # Setup test files for preprocessing | ||
| for path in ["/tmp/train.jsonl", "/tmp/test.jsonl"]: | ||
| with open(path, "w") as f: | ||
| f.write("{}") | ||
|
|
||
| mock_preprocessor = MagicMock() | ||
| mock_preprocessor.process.return_value = { | ||
| "train_jsonl_temp_filepath": "/tmp/train.jsonl", | ||
| "train_csv_s3_object": "s3://bucket/train.csv", | ||
| "test_csv_s3_object": "s3://bucket/test.csv", | ||
| } | ||
| mock_preprocessor.cleanup = MagicMock() | ||
| mock_preprocessor_cls.return_value = mock_preprocessor | ||
|
|
||
| mock_openai = MagicMock() | ||
| mock_openai.files.create.side_effect = create_file_mock("fine-tune") | ||
| mock_openai.fine_tuning.jobs.create.side_effect = [ | ||
| MagicMock(id=f"ft_mock_job_{i}", status="running") for i in range(1, 4) | ||
| ] | ||
| mock_get_openai_client.return_value = mock_openai | ||
|
|
||
| body = { | ||
| "document_id": str(document.id), | ||
| "base_model": "gpt-4", | ||
| "split_ratio": [0.5, 0.7, 0.9], | ||
| "system_prompt": "you are a model able to classify", | ||
| } | ||
|
|
||
| with patch("app.api.routes.fine_tuning.Session") as SessionMock: | ||
| SessionMock.return_value.__enter__.return_value = db | ||
| SessionMock.return_value.__exit__.return_value = None | ||
|
|
||
| response = client.post( | ||
| "/api/v1/fine_tuning/fine_tune", | ||
| json=body, | ||
| headers=user_api_key_header, | ||
| ) | ||
| f.write('{"prompt": "test", "completion": "label"}') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the hardcoded file paths and minimal CSV test data.
- The hardcoded file paths
/tmp/train.jsonland/tmp/test.jsonlmay cause issues in different environments (Windows, containerized environments with read-only filesystems). - The CSV test data with only 3 rows might not be sufficient to test split ratio functionality properly with ratios like 0.5, 0.7, 0.9.
Consider using temporary directories and more robust test data:
+import tempfile
+from pathlib import Path
def test_finetune_from_csv_multiple_split_ratio(
self,
client,
db,
user_api_key_header,
):
# Setup S3 bucket for moto
s3 = boto3.client("s3", region_name="us-east-1")
s3.create_bucket(Bucket="test-bucket")
- # Create a test CSV file content
- csv_content = "prompt,label\ntest1,label1\ntest2,label2\ntest3,label3"
+ # Create a test CSV file content with more rows for proper split testing
+ csv_rows = ["prompt,label"]
+ for i in range(1, 21): # 20 rows for better split ratio testing
+ csv_rows.append(f"test{i},label{i}")
+ csv_content = "\n".join(csv_rows)
- # Setup test files for preprocessing
- for path in ["/tmp/train.jsonl", "/tmp/test.jsonl"]:
- with open(path, "w") as f:
- f.write('{"prompt": "test", "completion": "label"}')
+ # Setup test files for preprocessing in a temporary directory
+ with tempfile.TemporaryDirectory() as tmpdir:
+ for filename in ["train.jsonl", "test.jsonl"]:
+ path = Path(tmpdir) / filename
+ with open(path, "w") as f:
+ f.write('{"prompt": "test", "completion": "label"}')Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/tests/api/routes/test_fine_tuning.py around lines 54 to 65, the
test uses hardcoded /tmp file paths and only three CSV rows which can break on
Windows or read-only containers and won't properly validate split ratios;
replace the hardcoded paths with a tempfile.TemporaryDirectory (or pytest
tmp_path) and create test files inside it, and expand the CSV test data to
include more rows (e.g., 20+) so split ratios like 0.5/0.7/0.9 produce
meaningful train/test sizes; ensure files are cleaned up automatically by the
tempdir and update any path references in the test to point to the generated
temp files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/app/api/routes/fine_tuning.py (4)
230-234: Ensure Content-Type is set before uploading to storage.If a client omits the part’s Content-Type, S3 upload with ExtraArgs.ContentType=None can fail.
- # Upload the file to storage and create document + # Upload the file to storage and create document + # Ensure a sane Content-Type for storage uploads + if not (file.content_type and file.content_type.strip()): + file.content_type = "text/csv" storage = get_cloud_storage(session=session, project_id=current_user.project_id) document_id = uuid4() object_store_url = storage.put(file, Path(str(document_id)))
83-118: Always run preprocessor.cleanup(), even if preprocessing fails.Cleanup currently runs only after a successful upload attempt; failures before that can leak temp files.
- preprocessor = DataPreprocessor( - document, storage, ratio, request.system_prompt - ) - result = preprocessor.process() - train_data_temp_filepath = result["train_jsonl_temp_filepath"] - train_data_s3_object = result["train_csv_s3_object"] - test_data_s3_object = result["test_csv_s3_object"] - - try: - with open(train_data_temp_filepath, "rb") as train_f: - uploaded_train = client.files.create( - file=train_f, purpose="fine-tune" - ) - logger.info( - f"[process_fine_tuning_job] File uploaded to OpenAI successfully | " - f"job_id={job_id}, project_id={project_id}|" - ) - except openai.OpenAIError as e: - error_msg = handle_openai_error(e) - logger.error( - f"[process_fine_tuning_job] Failed to upload to OpenAI: {error_msg} | " - f"job_id={job_id}, project_id={project_id}|" - ) - update_finetune_job( - session=session, - job=fine_tune, - update=FineTuningUpdate( - status=FineTuningStatus.failed, - error_message="Error while uploading file to openai : " - + error_msg, - ), - ) - return - finally: - preprocessor.cleanup() + preprocessor = DataPreprocessor( + document, storage, ratio, request.system_prompt + ) + try: + result = preprocessor.process() + train_data_temp_filepath = result["train_jsonl_temp_filepath"] + train_data_s3_object = result["train_csv_s3_object"] + test_data_s3_object = result["test_csv_s3_object"] + + try: + with open(train_data_temp_filepath, "rb") as train_f: + uploaded_train = client.files.create( + file=train_f, purpose="fine-tune" + ) + logger.info( + f"[process_fine_tuning_job] File uploaded to OpenAI successfully | " + f"job_id={job_id}, project_id={project_id}|" + ) + except openai.OpenAIError as e: + error_msg = handle_openai_error(e) + logger.error( + f"[process_fine_tuning_job] Failed to upload to OpenAI: {error_msg} | " + f"job_id={job_id}, project_id={project_id}|" + ) + update_finetune_job( + session=session, + job=fine_tune, + update=FineTuningUpdate( + status=FineTuningStatus.failed, + error_message="Error while uploading file to openai : " + + error_msg, + ), + ) + return + finally: + preprocessor.cleanup()
167-181: Avoid None deref when job lookup fails.If fetch_by_id raises before assigning fine_tune, updating it in the except block will crash.
- update_finetune_job( - session=session, - job=fine_tune, - update=FineTuningUpdate( - status=FineTuningStatus.failed, - error_message="Error while processing the background job : " - + error_msg, - ), - ) + if fine_tune is not None: + update_finetune_job( + session=session, + job=fine_tune, + update=FineTuningUpdate( + status=FineTuningStatus.failed, + error_message="Error while processing the background job : " + + error_msg, + ), + ) + else: + logger.error( + "[process_fine_tuning_job] No DB job loaded; skipping failure update" + )
424-424: Fix log message typo.Minor: “retrive” → “retrieve”.
- f"[retrive_job_by_document]No fine-tuning jobs found for document_id={document_id}, project_id={project_id}" + f"[retrieve_job_by_document]No fine-tuning jobs found for document_id={document_id}, project_id={project_id}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/routes/fine_tuning.py(6 hunks)backend/app/models/fine_tuning.py(1 hunks)backend/app/tests/api/routes/test_fine_tuning.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/app/models/fine_tuning.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/api/routes/test_fine_tuning.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/routes/fine_tuning.py (9)
backend/app/models/fine_tuning.py (4)
FineTuningJobCreate(29-52)FineTuningJobPublic(104-120)FineTuningUpdate(93-101)FineTuningStatus(13-18)backend/app/models/document.py (1)
Document(20-40)backend/app/core/cloud/storage.py (4)
get_cloud_storage(262-279)client(30-42)put(120-122)put(150-177)backend/app/crud/document.py (1)
DocumentCrud(14-135)backend/app/utils.py (5)
get_openai_client(178-208)APIResponse(28-49)mask_string(166-175)handle_openai_error(52-56)success_response(35-38)backend/app/crud/fine_tuning.py (4)
create_fine_tuning_job(20-66)fetch_by_id(88-104)update_finetune_job(165-182)fetch_by_document_id(107-127)backend/app/crud/model_evaluation.py (2)
create_model_evaluation(20-58)fetch_active_model_evals(150-170)backend/app/core/finetune/preprocessing.py (1)
DataPreprocessor(17-157)backend/app/api/routes/model_evaluation.py (1)
run_model_evaluation(49-112)
🪛 Ruff (0.13.1)
backend/app/api/routes/fine_tuning.py
223-223: Local variable client is assigned to but never used
Remove assignment to unused variable client
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (3)
backend/app/api/routes/fine_tuning.py (3)
219-221: Also validate CSV MIME type (keep extension check as a safeguard).Add a MIME allowlist; accept either a .csv extension or a CSV-ish content-type.
- # Validate file is CSV - if not file.filename.lower().endswith(".csv"): - raise HTTPException(status_code=400, detail="File must be a CSV file") + # Validate file is CSV + allowed_mime_types = { + "text/csv", + "application/csv", + "application/vnd.ms-excel", + "text/plain", + } + if (not file.filename.lower().endswith(".csv")) and ( + (file.content_type or "") not in allowed_mime_types + ): + raise HTTPException(status_code=400, detail="File must be a CSV file")
223-228: Fix linter warning: unused variableclient.Call solely for validation; don’t bind.
- client = get_openai_client( # Used here only to validate the user's OpenAI key; + get_openai_client( # Used here only to validate the user's OpenAI key; # the actual client is re-initialized separately inside the background task session, current_user.organization_id, current_user.project_id, )
54-55: Nice: added cancelled status mapping.Good to keep internal statuses aligned with provider states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_fine_tuning.py (1)
468-468: Minor style consistency.There's an extra blank line at the end of the file that could be removed for cleaner formatting.
- assert len(evaluations) == 0 - + assert len(evaluations) == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/api/routes/test_fine_tuning.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/tests/api/routes/test_fine_tuning.py (6)
backend/app/tests/utils/test_data.py (1)
create_test_fine_tuning_jobs(133-160)backend/app/tests/utils/utils.py (1)
get_document(116-137)backend/app/models/fine_tuning.py (2)
Fine_Tuning(55-90)FineTuningStatus(13-18)backend/app/models/model_evaluation.py (2)
ModelEvaluation(38-85)ModelEvaluationStatus(14-18)backend/app/core/cloud/storage.py (3)
client(30-42)get_signed_url(135-137)get_signed_url(217-242)backend/app/tests/conftest.py (2)
db(24-41)user_api_key_header(77-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (9)
backend/app/tests/api/routes/test_fine_tuning.py (9)
1-15: LGTM!The imports are comprehensive and include all the necessary modules for the updated test structure, including moto for AWS mocking, boto3 for S3 interaction, and all the required model classes and settings.
35-35: LGTM!The
@mock_awsdecorator is correctly applied to enable moto's AWS service mocking for the test.
42-53: LGTM!The S3 bucket setup is correctly implemented with region-aware bucket creation that follows AWS requirements for non-us-east-1 regions.
63-94: LGTM!The mock setup is comprehensive and correctly isolates cloud storage, OpenAI client, and background processing. The test data structure and API call format are appropriate.
101-106: LGTM!The response validation correctly verifies the new API response structure with
document_idandjobsfields, and confirms background processing was called for each split ratio.
111-117: LGTM!The job validation logic correctly checks the pending status (as processing is mocked) and verifies the split ratios are as expected.
127-127: LGTM!The provider job ID format update from
ft_mock_job_123toftjob-mock_job_123is consistent across tests and aligns with the expected OpenAI fine-tuning job ID format.Also applies to: 156-156
207-461: LGTM!The
TestAutoEvaluationTriggerclass provides comprehensive test coverage for the auto-evaluation feature:
- Tests successful evaluation trigger on completion status change
- Tests that evaluations are skipped when one already exists
- Tests that evaluations aren't triggered for non-completion status changes
- Tests that evaluations aren't triggered for already completed jobs
The test logic is sound and properly mocks all dependencies. The assertions correctly verify database state and function calls.
56-62: Address the hardcoded file paths and minimal CSV test data.The test still uses hardcoded
/tmp/paths and insufficient CSV data, exactly as flagged in the previous review. This creates cross-platform compatibility issues and doesn't properly test split ratio functionality.Apply this fix to use pytest's
tmp_pathfixture and expand the CSV data:-def test_finetune_from_csv_multiple_split_ratio( +def test_finetune_from_csv_multiple_split_ratio( self, client, db, user_api_key_header, + tmp_path, ):- # Create a test CSV file content - csv_content = "prompt,label\ntest1,label1\ntest2,label2\ntest3,label3" + # Create a test CSV file content with more rows for proper split testing + csv_rows = ["prompt,label"] + for i in range(1, 21): # 20 rows for better split ratio testing + csv_rows.append(f"test{i},label{i}") + csv_content = "\n".join(csv_rows) - # Setup test files for preprocessing - for path in ["/tmp/train.jsonl", "/tmp/test.jsonl"]: - with open(path, "w") as f: - f.write('{"prompt": "test", "completion": "label"}') + # Setup test files for preprocessing in temporary directory + for filename in ["train.jsonl", "test.jsonl"]: + filepath = tmp_path / filename + with open(filepath, "w") as f: + f.write('{"prompt": "test", "completion": "label"}')
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/api/routes/fine_tuning.py (1)
223-228: Fix linter warning: unusedclientvariable.The
clientvariable is assigned but never used, causing a linter warning.- client = get_openai_client( # Used here only to validate the user's OpenAI key; + get_openai_client( # Used here only to validate the user's OpenAI key; # the actual client is re-initialized separately inside the background task session, current_user.organization_id, current_user.project_id, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/fine_tuning.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/routes/fine_tuning.py (9)
backend/app/models/fine_tuning.py (4)
FineTuningJobCreate(29-52)FineTuningJobPublic(104-120)FineTuningUpdate(93-101)FineTuningStatus(13-18)backend/app/models/document.py (1)
Document(20-40)backend/app/core/cloud/storage.py (4)
get_cloud_storage(262-279)client(30-42)put(120-122)put(150-177)backend/app/crud/document.py (1)
DocumentCrud(14-135)backend/app/utils.py (3)
get_openai_client(178-208)mask_string(166-175)handle_openai_error(52-56)backend/app/crud/fine_tuning.py (4)
create_fine_tuning_job(20-66)fetch_by_id(88-104)update_finetune_job(165-182)fetch_by_document_id(107-127)backend/app/crud/model_evaluation.py (2)
create_model_evaluation(20-58)fetch_active_model_evals(150-170)backend/app/core/finetune/preprocessing.py (1)
DataPreprocessor(17-157)backend/app/api/routes/model_evaluation.py (1)
run_model_evaluation(49-112)
🪛 Ruff (0.13.1)
backend/app/api/routes/fine_tuning.py
223-223: Local variable client is assigned to but never used
Remove assignment to unused variable client
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (19)
backend/app/api/routes/fine_tuning.py (19)
4-5: LGTM! Imports added for new file upload functionality.The UUID and Path imports are appropriate for the new file handling and document creation features.
9-9: LGTM! FastAPI imports updated for multipart form handling.The addition of File, Form, and UploadFile imports correctly supports the new async file upload endpoint.
16-18: LGTM! Model imports for document and evaluation integration.The added imports support the new document creation and automatic model evaluation features.
22-28: LGTM! Utility imports consolidated and error handling centralized.Good refactoring to centralize the
handle_openai_errorutility function fromapp.utils.
34-35: LGTM! Model evaluation CRUD imports added.The imports support the new automatic evaluation creation functionality.
40-40: LGTM! Import for evaluation orchestration.The
run_model_evaluationimport enables background evaluation task queuing.
54-54: LGTM! Status mapping updated for cancelled status.The addition of the cancelled status mapping aligns with the new FineTuningStatus enum.
189-201: LGTM! Function signature updated for multipart form handling.The async function signature correctly uses FastAPI's Form and File parameters for handling CSV uploads. The parameter descriptions are clear and helpful.
202-212: LGTM! Split ratio validation is robust.The validation correctly parses comma-separated ratios and ensures they're within the valid range (0, 1).
213-218: LGTM! System prompt validation added.Good validation to ensure the system prompt is not empty after stripping whitespace.
219-222: File validation already addresses past review comment.The implementation correctly validates both filename extension and MIME type as suggested in the previous review.
230-243: LGTM! Document creation and storage upload implemented correctly.The file upload to cloud storage and document creation follows the established patterns. The use of UUID for document ID and proper error handling are appropriate.
244-251: LGTM! FineTuningJobCreate request object properly constructed.The request object is built correctly with all validated parameters, including the document ID from the newly created document.
254-268: LGTM! Multiple job creation for split ratios handled correctly.The loop correctly creates one job per split ratio and queues background tasks only for newly created jobs.
297-299: LGTM! Response includes document ID for client tracking.The response correctly includes the
document_idalongside the job information, enabling clients to track the uploaded document.
307-312: LGTM! Function signature updated for background task support.The parameter reordering and addition of
BackgroundTaskscorrectly supports the new auto-evaluation feature.
347-352: LGTM! Status transition detection for auto-evaluation.The logic correctly identifies when a job transitions from running to completed, which triggers automatic evaluation.
360-396: LGTM! Automatic evaluation creation and queuing implemented well.The auto-evaluation logic is comprehensive:
- Checks for existing active evaluations to avoid duplicates
- Creates new evaluation with proper parameters
- Queues background evaluation task
- Includes appropriate logging
The implementation follows established patterns and handles the workflow correctly.
100-101: LGTM! Centralized error handling implementation.The usage of the centralized
handle_openai_errorfunction improves code maintainability and consistency across the codebase.Also applies to: 130-131, 322-322
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/api/routes/fine_tuning.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/routes/fine_tuning.py (10)
backend/app/models/fine_tuning.py (3)
FineTuningJobCreate(29-52)FineTuningUpdate(93-101)FineTuningStatus(13-18)backend/app/models/document.py (1)
Document(20-40)backend/app/models/model_evaluation.py (2)
ModelEvaluationBase(21-26)ModelEvaluationStatus(14-18)backend/app/core/cloud/storage.py (3)
get_cloud_storage(262-279)put(120-122)put(150-177)backend/app/crud/document.py (1)
DocumentCrud(14-135)backend/app/utils.py (4)
get_openai_client(170-200)APIResponse(27-48)mask_string(158-167)success_response(34-37)backend/app/crud/fine_tuning.py (4)
create_fine_tuning_job(20-66)fetch_by_id(88-104)update_finetune_job(165-182)fetch_by_document_id(107-127)backend/app/crud/model_evaluation.py (2)
create_model_evaluation(20-58)fetch_active_model_evals(150-170)backend/app/core/finetune/preprocessing.py (1)
DataPreprocessor(17-157)backend/app/api/routes/model_evaluation.py (1)
run_model_evaluation(49-112)
🪛 GitHub Actions: AI Platform CI
backend/app/api/routes/fine_tuning.py
[error] 22-22: ImportError: cannot import name 'handle_openai_error' from 'app.utils'. Ensure 'handle_openai_error' is defined in app/utils.py and exported, or adjust imports.
| # Validate file is CSV | ||
| if not file.filename.lower().endswith(".csv") and file.content_type != "text/csv": | ||
| raise HTTPException(status_code=400, detail="File must be a CSV file") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
CSV validation condition is inverted
The current and check only rejects the upload when both the extension and the MIME type are wrong, so a file named data.csv with an arbitrary content-type slips through—undoing the MIME validation you meant to add. Flip the logic to fail when either check fails and accept the common CSV MIME types:
- if not file.filename.lower().endswith(".csv") and file.content_type != "text/csv":
+ allowed_mime_types = {
+ "text/csv",
+ "application/csv",
+ "application/vnd.ms-excel",
+ "text/plain",
+ }
+ if (
+ not file.filename.lower().endswith(".csv")
+ or file.content_type not in allowed_mime_types
+ ):
raise HTTPException(status_code=400, detail="File must be a CSV file")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Validate file is CSV | |
| if not file.filename.lower().endswith(".csv") and file.content_type != "text/csv": | |
| raise HTTPException(status_code=400, detail="File must be a CSV file") | |
| # Validate file is CSV | |
| allowed_mime_types = { | |
| "text/csv", | |
| "application/csv", | |
| "application/vnd.ms-excel", | |
| "text/plain", | |
| } | |
| if ( | |
| not file.filename.lower().endswith(".csv") | |
| or file.content_type not in allowed_mime_types | |
| ): | |
| raise HTTPException(status_code=400, detail="File must be a CSV file") |
🤖 Prompt for AI Agents
In backend/app/api/routes/fine_tuning.py around lines 219 to 221, the CSV
validation uses an AND which only rejects when both extension and MIME type are
wrong; change it to fail when either check fails by replacing the condition with
an OR and validate against a small allowed MIME set (e.g., "text/csv",
"application/csv", "text/plain", "application/vnd.ms-excel") using lowercased
comparisons for both filename and content_type, and raise the HTTPException if
the filename doesn't end with ".csv" or the content_type is not in that allowed
set.
AkhileshNegi
changed the title
|
|
||
| # Queue the evaluation task | ||
| background_tasks.add_task( | ||
| run_model_evaluation, model_eval.id, current_user |
There was a problem hiding this comment.
"currect_user" is an object and object should not be passed to background tasks
| raise HTTPException( | ||
| status_code=400, detail="System prompt must be a non-empty string" | ||
| ) | ||
|
|
There was a problem hiding this comment.
I thought we were going ot do that validation as well in this that the cvs has two column , one with query and another with the name label, then only will the fine tuning process starts, otherwise an error is through by the endpoint itself
| current_user: CurrentUserOrgProject, | ||
| request: FineTuningJobCreate, | ||
| background_tasks: BackgroundTasks, | ||
| file: UploadFile = File(..., description="CSV file to use for fine-tuning"), |
There was a problem hiding this comment.
the part from L187 to L194 should be moved to fine tuning model file
2 participants
Summary
Issue: #362
Enhancing the fine-tuning workflow by adding automatic evaluation triggers and improving status handling.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
New Features
Documentation
Tests