-
Notifications
You must be signed in to change notification settings - Fork 0
Barebones training pipeline #2
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
@@ -0,0 +1,66 @@ | |||
import torch |
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.
Could you write a docstring at the start with the explanation on how to use the training pipeline?
def save(self,path): | ||
torch.save(self.model.state_dict(),path) | ||
|
||
No newline at end of 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.
Could you also write a simple test to make sure the code is working in the test folder?
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.
Pull Request Overview
This PR introduces a basic training pipeline for machine learning models using PyTorch. The implementation provides essential functionality for training, validation, and model saving.
- Implements a TrainingPipeline class with configurable hyperparameters
- Provides methods for training epochs, validation, and model persistence
- Sets up automatic train/validation data splitting and device management
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
self.epochs = epochs | ||
|
||
#train test split(0.8:0.2) | ||
train_size = int(len(data)) |
Copilot
AI
Oct 8, 2025
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 train/test split logic is incorrect. Setting train_size to the full length of data results in val_size being 0, meaning no validation data. For an 80/20 split, train_size should be int(0.8 * len(data))
.
train_size = int(len(data)) | |
train_size = int(0.8 * len(data)) |
Copilot uses AI. Check for mistakes.
for xb,yb in self.train_loader: | ||
xb , yb = xb.to(self.device) , yb.to(self.device) | ||
preds = self.model.predict(xb) | ||
loss = self.criterion(yb,preds) |
Copilot
AI
Oct 8, 2025
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 arguments to CrossEntropyLoss are in the wrong order. It should be self.criterion(preds, yb)
where predictions come first and targets second.
Copilot uses AI. Check for mistakes.
def validate(self): | ||
self.model.eval() | ||
total_loss = 0 | ||
with torch.inference_mode: |
Copilot
AI
Oct 8, 2025
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.
Missing parentheses for the context manager. Should be with torch.inference_mode():
.
with torch.inference_mode: | |
with torch.inference_mode(): |
Copilot uses AI. Check for mistakes.
total_loss = 0 | ||
with torch.inference_mode: | ||
#loops through each batch | ||
for xb,yb in self.val_data: |
Copilot
AI
Oct 8, 2025
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.
Incorrect attribute reference. Should be self.val_loader
instead of self.val_data
to iterate through the validation DataLoader.
for xb,yb in self.val_data: | |
for xb,yb in self.val_loader: |
Copilot uses AI. Check for mistakes.
for xb,yb in self.val_data: | ||
xb , yb = xb.to(self.device) , yb.to(self.device) | ||
preds = self.model.predict(xb) | ||
loss = self.criterion(yb,preds) |
Copilot
AI
Oct 8, 2025
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 arguments to CrossEntropyLoss are in the wrong order. It should be self.criterion(preds, yb)
where predictions come first and targets second.
Copilot uses AI. Check for mistakes.
#loops through each batch | ||
for xb,yb in self.train_loader: | ||
xb , yb = xb.to(self.device) , yb.to(self.device) | ||
preds = self.model.predict(xb) |
Copilot
AI
Oct 8, 2025
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.
Using predict
method is unconventional for PyTorch models. Standard practice is to call the model directly with self.model(xb)
or use self.model.forward(xb)
.
Copilot uses AI. Check for mistakes.
#loops through each batch | ||
for xb,yb in self.val_data: | ||
xb , yb = xb.to(self.device) , yb.to(self.device) | ||
preds = self.model.predict(xb) |
Copilot
AI
Oct 8, 2025
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.
Using predict
method is unconventional for PyTorch models. Standard practice is to call the model directly with self.model(xb)
or use self.model.forward(xb)
.
Copilot uses AI. Check for mistakes.
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.
Some quick small changes
testData = TensorDataset(X,y) | ||
|
||
pipeline = TrainingPipeline(model = testModel,batch_size = 10,data = testData, device = "cpu") | ||
pipeline.training() No newline at end of 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.
Could we move this to a separate test file? (see how the camera module did it)
self.epochs = epochs | ||
|
||
#train test split(0.8:0.2) | ||
train_size = int(0.8* len(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.
suggestion: move the 0.8 to be a default parameter in init in case we want to test different splits
Accept plz