-
Couldn't load subscription status.
- Fork 5
Classification : train and test data to s3 #343
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
| result = preprocessor.process() | ||
| train_path = result["train_file"] | ||
| test_path = result["test_file"] | ||
| train_path = result["train_jsonl_path"] |
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.
how is train_path different than train_data_url? and do we need it?
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.
train_path is the local temp path where the training data is saved i think and train_data_url is the s3 path
@nishika26 maybe we can have more descriptive names? like training_data_tmp_file_path and training_data_object_store_url - they are longer for sure, but better represent what the vars mean
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.
exactly as what @kartpop stated here, and maybe instead of using over descriptive names, I could add a comment along the lines of the varaibles? that would be better I think
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.
made the variable names descriptive
|
|
||
| self.system_message = {"role": "system", "content": system_message.strip()} | ||
|
|
||
| def upload_csv_to_s3(self, df, filename: str) -> str: |
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.
do we need separate function to upload? why can't we use existing function to upload file?
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.
We are already using the existing put function for the actual upload inside this function. The upload_csv_to_s3 method is just a wrapper that converts a DataFrame into CSV bytes, wraps it in an UploadFile (the format that put(source: UploadFile, basename: Path) expects), and then delegates the upload to put.
| f"train_data_{int(self.split_ratio * 100)}_{uuid.uuid4().hex}.jsonl" | ||
| ) | ||
| test_file = f"test_data_{int(self.split_ratio * 100)}_{uuid.uuid4().hex}.jsonl" | ||
| uuids = uuid.uuid4().hex |
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.
uuids or uuid ?
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.
uuids, because if I name the variable uuid, it will shadow the imported uuid module, which can confuse both the interpreter and anyone reading the code. But i guess "uuids" is confusing as well so I will name it something better
| ) | ||
| test_file = f"test_data_{int(self.split_ratio * 100)}_{uuid.uuid4().hex}.jsonl" | ||
| uuids = uuid.uuid4().hex | ||
| pct = int(self.split_ratio * 100) # percentage of data |
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.
name variables in a way that is self explainatory
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.
+1
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.
noted
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.
approving with comments; can be merged after comments by @AkhileshNegi are resolved
| document_crud = DocumentCrud(session=session, owner_id=current_user.id) | ||
| document = document_crud.read_one(request.document_id) | ||
| preprocessor = DataPreprocessor( | ||
| document, storage, ratio, request.system_prompt |
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.
the storage = AmazonCloudStorage(current_user) will later change to using project_id instead
this is just FYI for now, this change need not be part of the current PR - can be part of a future PR
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, as of now (Thu, 26 Aug, 12:10 PM), the PR that changes storage initialization from using user_id to project_id hasn’t been merged yet, so I can’t incorporate that change in my PR at this moment or in any other PR till that PR is not merged to main
| result = preprocessor.process() | ||
| train_path = result["train_file"] | ||
| test_path = result["test_file"] | ||
| train_path = result["train_jsonl_path"] |
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.
train_path is the local temp path where the training data is saved i think and train_data_url is the s3 path
@nishika26 maybe we can have more descriptive names? like training_data_tmp_file_path and training_data_object_store_url - they are longer for sure, but better represent what the vars mean
| ) | ||
| test_file = f"test_data_{int(self.split_ratio * 100)}_{uuid.uuid4().hex}.jsonl" | ||
| uuids = uuid.uuid4().hex | ||
| pct = int(self.split_ratio * 100) # percentage of data |
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.
+1
|
|
||
| try: | ||
| with open(train_path, "rb") as train_f: | ||
| with open(train_data_temp_filepath, "rb") as train_f: |
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.
temp_train_data_filepath looks cleaner
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.
what's rb?
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.
"rb" means read the file in binary mode, which is required here since we’re uploading the file object to OpenAI’s API — the Files API specifically accepts a binary stream as input.
* Classification: db models and migration script (#305) * db models and migration script * Classification: Fine tuning Initiation and retrieve endpoint (#315) * Fine-tuning core, initiation, and retrieval * seperate session for bg task, and formating fixes * fixing alembic revision * Classification : Model evaluation of fine tuned models (#326) * Model evaluation of fine tuned models * fixing alembic revision * alembic revision fix * Classification : train and test data to s3 (#343) * alembic file for adding and removing columns * train and test s3 url column * updating alembic revision * formatting fix * Classification : retaining prediction and fetching data from s3 for model evaluation (#359) * adding new columns to model eval table * test data and prediction data s3 url changes * single migration file * status enum columns * document seeding * Classification : small fixes and storage related changes (#365) * first commit covering all * changing model name to fine tuned model in model eval * error handling in get cloud storage and document not found error handling * fixing alembic revision * uv lock * new uv lock file * updated uv lock file * coderabbit suggestions and removing unused imports * changes in uv lock file * making csv a supported file format, changing uv lock and pyproject toml
Summary
Target issue is #341
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.Changes Made
app/core/finetune/preprocessing.pyS3 Upload Support
Added upload_csv_to_s3() method to upload train/test CSV splits directly to S3.
Added detailed logging for both successful and failed uploads.
File Naming
Standardized naming for train/test splits using pct (split ratio %) and a unique uuid.
Generates both:
.csv → uploaded to S3
.jsonl → stored locally in temp (for fine-tuning)
Preprocessor class now returns
train_csv_url → S3 path of training split CSV
test_csv_url → S3 path of test split CSV
train_jsonl_path → local path of training JSONL
app/api/routes/fine_tuning.pyRemoved test data upload to OpenAI
Test split will no longer be uploaded to OpenAI.
For evaluation, test data will instead be fetched directly from S3.
DB is now updated with:
train_data_url → S3 URL of the training split.
test_data_url → S3 URL of the test split.