chore(training): correct f-strings; log param counts via utils - #113
Conversation
Reviewer's GuideRefactored extensive logger statements to use f-strings for proper variable interpolation and leveraged the shared count_model_params utility for reporting trainable parameter counts, enhancing log clarity without altering runtime behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@uelkerd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRefactors the emotion-detection training pipeline (dev-mode dataset scaling, helper-driven training flow, removed debug_mode); HF loader cleans unused extracted archives; JWT blacklist stores expirations and prunes expired tokens; security headers middleware expands configuration and enhances UA analysis; adds a model-parameter counting util. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Trainer
participant DataPrep
participant Model
participant Loss
participant Validator
User->>Trainer: train_emotion_detection_model(dev_mode)
Trainer->>DataPrep: prepare_data(dev_mode) -- downsample & adjust batch size if dev
DataPrep-->>Trainer: datasets & dataloaders
loop per epoch
Trainer->>Trainer: _handle_progressive_unfreezing(epoch)
loop per batch
Trainer->>Model: forward(batch)
Model-->>Trainer: logits
Trainer->>Loss: compute(logits, labels)
Loss-->>Trainer: loss
Trainer->>Trainer: _train_single_batch(...) and _log_progress(...)
alt first batch (detailed logs)
Trainer->>Trainer: _log_data_distribution/_log_model_output/_log_loss_analysis
end
Trainer->>Trainer: _log_gradient_stats_before/_log_gradient_stats_after
end
Trainer->>Trainer: _maybe_validate_and_early_stop()
alt validation runs
Trainer->>Validator: evaluate(model, val_loader)
Validator-->>Trainer: metrics
Trainer->>Trainer: save/checkpoint if best
end
end
Trainer-->>User: final metrics & checkpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on minor but important code quality improvements within the emotion detection training pipeline. The primary changes involve refactoring logging statements to use f-strings for enhanced readability and accuracy, and standardizing the way model parameters are counted by leveraging a shared utility function. These updates ensure clearer logging output and promote consistent practices across the codebase without altering the existing behavior of the training pipeline.
Highlights
- Refactored logging statements to use f-strings: Multiple logging statements within the
training_pipeline.pyfile have been updated to use f-strings. This improves the clarity and correctness of log messages by ensuring variables are properly interpolated. - Standardized model parameter counting: The method for counting trainable model parameters has been standardized. Instead of an internal model method, the code now utilizes the
count_model_paramsutility function fromsrc/utils, promoting code reuse and maintainability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes good improvements by correcting f-string formatting in many log messages and switching to a utility function for counting model parameters. This not only fixes broken logs but also makes the parameter count log more accurate by specifying trainable parameters.
While many f-strings were fixed, I noticed there are still several in src/models/emotion_detection/training_pipeline.py that use the old formatting. It would be great to fix these as well for consistency when you get a chance. Overall, a solid cleanup!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/models/emotion_detection/training_pipeline.py (3)
401-403: Complete the f-string migration; several logs still render literal braces (and one filename).A number of log lines (and the non-best checkpoint filename) still use brace placeholders without f-strings, so values won’t interpolate and the checkpoint filename will literally contain “{epoch}”. Apply the patch below.
@@ - logger.info( - "Epoch {epoch}, Batch {batch_idx + 1}/{num_batches}, " - "Loss: {avg_loss:.8f}, LR: {current_lr:.2e}" - ) + logger.info( + f"Epoch {epoch}, Batch {batch_idx + 1}/{num_batches}, " + f"Loss: {avg_loss:.8f}, LR: {current_lr:.2e}" + ) @@ - if avg_loss < 1e-8: - logger.error("❌ CRITICAL: Average loss is suspiciously small: {avg_loss:.8f}") - if avg_loss > 100: - logger.error("❌ CRITICAL: Average loss is suspiciously large: {avg_loss:.8f}") + if avg_loss < 1e-8: + logger.error(f"❌ CRITICAL: Average loss is suspiciously small: {avg_loss:.8f}") + if avg_loss > 100: + logger.error(f"❌ CRITICAL: Average loss is suspiciously large: {avg_loss:.8f}") @@ - if (batch_idx + 1) % val_frequency == 0: - logger.info("🔍 Validating at batch {batch_idx + 1}...") + if (batch_idx + 1) % val_frequency == 0: + logger.info(f"🔍 Validating at batch {batch_idx + 1}...") self.validate(epoch) @@ - if self.should_stop_early(): - logger.info("🛑 Early stopping triggered at batch {batch_idx + 1}") + if self.should_stop_early(): + logger.info(f"🛑 Early stopping triggered at batch {batch_idx + 1}") return { @@ - logger.info("Epoch {epoch} completed - Loss: {avg_loss:.4f}, Time: {epoch_time:.1f}s") + logger.info(f"Epoch {epoch} completed - Loss: {avg_loss:.4f}, Time: {epoch_time:.1f}s") @@ - logger.info("Validating model at epoch {epoch}...") + logger.info(f"Validating model at epoch {epoch}...") @@ - self.save_checkpoint(epoch, val_metrics, is_best=True) - logger.info("New best model saved! Macro F1: {current_score:.4f}") + self.save_checkpoint(epoch, val_metrics, is_best=True) + logger.info(f"New best model saved! Macro F1: {current_score:.4f}") @@ - logger.info( - "No improvement. Patience: {self.patience_counter}/{self.early_stopping_patience}" - ) + logger.info( + f"No improvement. Patience: {self.patience_counter}/{self.early_stopping_patience}" + ) @@ - else: - checkpoint_path = self.output_dir / "checkpoint_epoch_{epoch}.pt" + else: + checkpoint_path = self.output_dir / f"checkpoint_epoch_{epoch}.pt" @@ - torch.save(checkpoint, checkpoint_path) - logger.info("Checkpoint saved: {checkpoint_path}") + torch.save(checkpoint, checkpoint_path) + logger.info(f"Checkpoint saved: {checkpoint_path}") @@ - with Path(history_path).open("w") as f: - json.dump(serializable_history, f, indent=2) - logger.info("Training history saved to {history_path}") + with Path(history_path).open("w") as f: + json.dump(serializable_history, f, indent=2) + logger.info(f"Training history saved to {history_path}") @@ - with Path(history_path).open("w") as f: - json.dump(simplified_history, f, indent=2) - logger.info("Simplified training history saved to {history_path}") + with Path(history_path).open("w") as f: + json.dump(simplified_history, f, indent=2) + logger.info(f"Simplified training history saved to {history_path}")Also applies to: 406-408, 411-416, 434-434, 447-447, 462-467, 501-505, 559-559, 579-579
259-264: Bug:hasattr(self, "model")always true here; useself.model is None.
__init__setsself.model = None, sohasattris always true and the model won’t be initialized ifload_modelis called on a fresh instance, causing an AttributeError when loading state dict.- if not hasattr(self, "model"): + if self.model is None: datasets = self.prepare_data() class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
106-112: Avoid double dataset preparation (dev_mode gets silently overridden).
train_emotion_detection_modelcallstrainer.prepare_data(dev_mode=...)and thentrainer.train(), which unconditionally callsself.prepare_data()again with default args. This both wastes time and overrides dev-mode sampling. Persistclass_weightsand only prepare if dataloaders aren’t set.@@ self.tokenizer = None + self.class_weights = None @@ - datasets = self.data_loader.prepare_datasets() + datasets = self.data_loader.prepare_datasets() + # Persist class weights for downstream initialization if prepare_data() was called externally + self.class_weights = datasets.get("class_weights") @@ - datasets = self.prepare_data() - - class_weights = datasets.get("class_weights") - self.initialize_model(class_weights) + class_weights = None + if self.train_dataloader is None: + datasets = self.prepare_data() + class_weights = datasets.get("class_weights") + else: + class_weights = self.class_weights + self.initialize_model(class_weights)If you prefer to keep
train()unaware, alternatively remove the explicittrainer.prepare_data(...)call fromtrain_emotion_detection_modeland pass adev_modearg intotrain(). I can draft that variant if you want.Also applies to: 134-136, 514-518
🧹 Nitpick comments (2)
src/models/emotion_detection/training_pipeline.py (2)
291-293: Validation cadence likely too sparse on small datasets.
val_frequency = max(500, num_batches // 5)means no mid-epoch validation unless you have ≥500 batches. Consider a floor of 1–5% of the epoch with an upper bound.- val_frequency = max(500, num_batches // 5) + # Validate roughly 5 times per epoch, with a sensible floor and cap + val_frequency = max(1, min(500, max(1, num_batches // 5))) + # Optional: make this configurable
299-365: Gate heavy first-batch diagnostics behind DEBUG level.These logs compute reductions on GPU tensors and run every epoch’s first batch. They’re excellent during debugging but add overhead in production. Recommend logging them at DEBUG and flipping the logger level based on a
debug_modeflag.I can wire the
debug_modeargument (present intrain_emotion_detection_model) through to the trainer and switch theselogger.infocalls tologger.debugwhile settinglogger.setLevel(logging.DEBUG)if enabled.Also applies to: 368-390, 400-416
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(12 hunks)
⏰ 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: Analyze (python)
🔇 Additional comments (1)
src/models/emotion_detection/training_pipeline.py (1)
34-34: Good switch to shared param-count utility.Importing and using
count_model_params(..., only_trainable=True)centralizes logic and prevents drift vs. per-model implementations.Also applies to: 245-246
…o f-strings; correct checkpoint filename formatting
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/emotion_detection/training_pipeline.py (2)
258-266: Bug: hasattr check won’t initialize model when self.model is Noneself.model is defined in init, so hasattr(self, "model") is always True. If load_model is called before initialize_model, this will skip initialization and self.model.load_state_dict will error. Check for None instead.
Apply:
- if not hasattr(self, "model"): + if self.model is None: datasets = self.prepare_data() class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
605-616: Public API change introduces behavioral drift: dev_mode defaults to True and unused debug_mode parameter
- Defaulting dev_mode to True alters the previous behavior (now trains on 5% by default). This contradicts the PR claim that behavior is unchanged and can surprise downstream callers.
- debug_mode is unused throughout the pipeline.
Revert dev_mode default to False and remove debug_mode (or wire it end-to-end in a separate PR).
Apply:
def train_emotion_detection_model( @@ - device: Optional[str] = None, - dev_mode: bool = True, # Enable development mode by default - debug_mode: bool = True, # Enable debugging by default + device: Optional[str] = None, + dev_mode: bool = False, # Development mode opt-in to preserve prior behavior ) -> Dict[str, Any]:And update the docstring below accordingly (remove debug_mode).
♻️ Duplicate comments (1)
src/models/emotion_detection/training_pipeline.py (1)
34-34: Import path consistency and existence of src.utilsThis absolute import deviates from the surrounding relative imports and may fail depending on packaging/runtime (src-layout vs installed package). Confirm the module exists and consider aligning import style for consistency.
Run to verify location and signature:
#!/bin/bash set -euo pipefail echo "Searching for count_model_params definition:" rg -nP 'def\s+count_model_params\s*\(' src || true echo "Listing src/utils locations:" fd -t f utils src | sed -n '1,200p' echo "Grepping for count_model_params usage across repo:" rg -nF 'count_model_params(' || true
🧹 Nitpick comments (5)
src/models/emotion_detection/training_pipeline.py (5)
102-102: PR summary vs implementation: f-strings vs parameterized loggingThe PR description says “convert logger calls to f-strings”, but the code uses parameterized logging (logger.info("...", args)), which is generally preferred. Either update the PR description to reflect parameterized logging or switch calls to f-strings to match the stated objective. Don’t mix styles.
213-222: Downgrade “DEBUG” diagnostics to debug level and/or gate behind a flagThese are verbose diagnostics but logged at INFO. Consider using logger.debug and a guard (e.g., if logger.isEnabledFor(logging.DEBUG):) to reduce noise in normal runs.
Apply this minimal pattern:
- logger.info("🔍 DEBUG: Loss Function Analysis") - logger.info(" Loss function type: %s", type(self.loss_fn).__name__) + if logger.isEnabledFor(logging.DEBUG): + logger.debug("Loss Function Analysis") + logger.debug(" Loss function type: %s", type(self.loss_fn).__name__)If you want a flag, introduce self.debug_mode in init and gate on it as well.
303-367: Verbose training diagnostics at INFO; consider demoting to DEBUG and gatingLarge blocks of per-batch shape/stats logs at INFO will flood logs. Recommend logger.debug with an isEnabledFor(DEBUG) guard and/or a debug_mode flag to explicitly enable them.
Example pattern:
- if batch_idx == 0: - logger.info("🔍 DEBUG: Data Distribution Analysis") - logger.info(" Labels shape: %s", labels.shape) + if batch_idx == 0 and logger.isEnabledFor(logging.DEBUG): + logger.debug("Data Distribution Analysis") + logger.debug(" Labels shape: %s", labels.shape)Repeat for “Model Output Analysis”, “Loss Analysis”, and “Gradient Analysis” sections.
618-628: Docstring still advertises debug_mode and new default behaviorUpdate docstring to remove debug_mode and reflect dev_mode now being opt-in.
Apply:
- device: Device to use for training (auto-detect if None) - dev_mode: Enable development mode with smaller dataset - debug_mode: Enable debugging mode with enhanced logging + device: Device to use for training (auto-detect if None) + dev_mode: If True, use a small subset of data for quicker iterations
649-651: Consider exposing dev_mode in CLI/main or environment instead of defaulting in APItrain_emotion_detection_model is a convenient API; let callers choose dev_mode explicitly. For CLI, pass via flag or env to avoid accidental partial-data training.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(21 hunks)
🔇 Additional comments (3)
src/models/emotion_detection/training_pipeline.py (3)
392-393: Nice fix: robust formatting of clip_grad_norm_ return typeCasting to float avoids TypeError when clip_grad_norm_ returns a Tensor. Good defensive logging.
632-637: Logging copy reflects new defaults; adjust messages after reverting dev_mode defaultIf dev_mode becomes opt-in again, these messages are still fine. If you keep dev_mode default True, this section will fire by default and materially change behavior. Align with the chosen default.
246-251: Param count logging: verify utility existence and formatting approachPlease confirm that the
count_model_paramshelper insrc/utilsis defined, acceptsonly_trainable=True, and returns an integer as expected. Additionally, sincelogger.infosupports lazy interpolation, you may choose to pass the raw integer and let the logger handle formatting, or pre-format for readability.• Confirm that
count_model_paramsis implemented insrc/utilsand its signature includesonly_trainable
• Ensure it returns an integer whenonly_trainable=True
• (Optional) Instead offormat(..., ",d"), consider:
- Passing the raw count to
logger.info("…%d…", count)for lazy formatting- Precomputing a formatted string once, if that improves clarity
…op, clip_grad assignment)
… keep behavior unchanged
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/emotion_detection/training_pipeline.py (2)
263-267: Fix: checkpoint load never initializes model due to hasattr check
self.modelis set to None in init, sohasattr(self, "model")is always True and the init branch is skipped, causingNone.load_state_dict(...)to crash.Apply:
- if not hasattr(self, "model"): + if self.model is None: datasets = self.prepare_data() class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
596-602: Ensure returnedmodel_pathexists even when no “best” checkpoint was savedIf no improvement occurs,
best_model.ptmay never be written; returning a non-existent path can break consumers.- results = { + best_model_path = self.output_dir / "best_model.pt" + if not best_model_path.exists(): + # Fallback to the latest epoch checkpoint if best was never saved + ckpts = sorted(self.output_dir.glob("checkpoint_epoch_*.pt")) + if ckpts: + best_model_path = ckpts[-1] + + results = { "final_test_metrics": test_metrics, "best_validation_score": self.best_score, "training_history": self.training_history, - "model_path": str(self.output_dir / "best_model.pt"), + "model_path": str(best_model_path), "total_epochs": len(self.training_history), }
♻️ Duplicate comments (1)
src/models/emotion_detection/training_pipeline.py (1)
34-34: Import path may be brittle; align with local style and verify utility existsAbsolute
from src.utils ...can break outside a src-layout and is inconsistent with nearby relative imports. Prefer a relative import (adjust the level to match your layout) or ensuresrcis onPYTHONPATH. Also verify the utility actually exists to avoid runtime ImportError.Apply one of the following (adjust dots as needed):
-from src.utils import count_model_params +from ..utils import count_model_params # adjust relative level to your package layoutTo verify presence and path quickly:
#!/bin/bash # Verify utility exists and import path works set -euo pipefail echo "Definitions of count_model_params:" rg -nP '\bdef\s+count_model_params\s*\(' -C1 src || true python - <<'PY' try: from src.utils import count_model_params print("OK: src.utils.count_model_params import works") except Exception as e: print("WARN: src.utils import failed:", e) PY
🧹 Nitpick comments (3)
src/models/emotion_detection/training_pipeline.py (3)
296-297: Validation frequency can skip validation on small datasetsWith
val_frequency = max(500, num_batches // 5), smallnum_batchesyields 500 and no mid-epoch validation. Consider clamping to at least 1 and at most 500.- val_frequency = max(500, num_batches // 5) + # Validate roughly 5x/epoch, but never less than once and never too chatty + val_frequency = max(1, min(500, max(1, num_batches // 5)))
374-385: Avoid.datain gradient norm calculationUse
.detach()(or directlyp.grad.norm(2)) to avoid autograd footguns;.datais discouraged.- param_norm = p.grad.data.norm(2) + param_norm = p.grad.detach().norm(2)
639-645: Usedebug_modeto dial logging verbosityRight now, INFO-level diagnostics are very chatty even when
debug_mode=False. Consider setting the logger level from this flag.- if dev_mode: + # Control verbosity early + logger.setLevel(logging.INFO if debug_mode else logging.WARNING) + if dev_mode: logger.info("🚀 DEVELOPMENT MODE ENABLED: Fast training with reduced dataset") logger.info("🚀 Expected training time: 30-60 minutes instead of 9 hours") else: logger.info("🏭 PRODUCTION MODE: Full dataset training")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(20 hunks)
⏰ 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: Analyze (python)
🔇 Additional comments (11)
src/models/emotion_detection/training_pipeline.py (11)
102-102: Good switch to parameterized logging for device messageLazy formatting via logger args avoids unnecessary string interpolation. LGTM.
161-166: Dev-mode batch-size bump is reasonable and well loggedClear, bounded increase with informative logging. LGTM.
194-196: Dataset size logging converted correctlyInterpolations are now reliable; no stray braces. LGTM.
214-223: Loss/class-weight diagnostics look solidCasts to scalars with
.item()avoid Tensor-format errors. LGTM.
247-252: Param-count logging via utility: OK, minor nitUsing
format(..., ",d")yields a readable, grouped number and is safe with%s. No action needed.
319-325: Per-class positives logging is correct and efficientLimited to first 10 classes and scalarized counts. LGTM.
395-400: Nice robustness onclip_grad_norm_return typeCasting to float avoids formatting errors when a Tensor is returned. LGTM.
445-448: Epoch summary logging fixedInterpolations are now reliable and readable. LGTM.
461-482: Validation and patience logging improvements look goodClear epoch context and patience tracking with safe formatting. LGTM.
516-520: Checkpoint naming and save log are fineExplicit file name and path logging is clear. LGTM.
521-545: Order of operations intrain()is soundData prep → initialize → train/validate loop with early stopping. LGTM.
Co-authored-by: denizcan.uelker <denizcan.uelker@mercedes-benz.com>
Resolved issues in src/models/emotion_detection/training_pipeline.py with DeepSource Autofix
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/emotion_detection/training_pipeline.py (2)
265-269: Bug:hasattr(self, "model")is always True; model may still be None
self.modelis initialized toNonein__init__, sohasattr(self, "model")won’t detect uninitialized state. Loading will crash onself.model.load_state_dict(...).- if not hasattr(self, "model"): + if self.model is None: datasets = self.prepare_data() class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
553-559: Functional bug:prepare_data()is called twice and dev_mode is lost/overridden
train_emotion_detection_modelcallstrainer.prepare_data(dev_mode=dev_mode)and thentrainer.train()callsself.prepare_data()again with the defaultdev_mode=False. This forces a full dataset reload and can unintentionally switch from dev to prod mid-run, potentially causing OOM due to the dev-mode batch size increase.Option A (cleanest): thread
dev_modeintotrain()and callprepare_data()only there. Remove the pre-call.- def train(self) -> Dict[str, Any]: + def train(self, dev_mode: bool = False) -> Dict[str, Any]: ... - datasets = self.prepare_data() + datasets = self.prepare_data(dev_mode=dev_mode)- trainer.prepare_data(dev_mode=dev_mode) - - return trainer.train() + return trainer.train(dev_mode=dev_mode)Option B (minimal): guard in
train()to skip re-prep if already prepared.- datasets = self.prepare_data() + datasets = self.prepare_data() if self.train_dataloader is None else {"class_weights": None}Recommend Option A.
Also applies to: 680-682
♻️ Duplicate comments (2)
src/models/emotion_detection/training_pipeline.py (2)
34-34: Import style + module availability: prefer relative import or provide fallback; verifycount_model_paramsexistsThis absolute import breaks consistency with nearby relative imports and may fail outside a src-layout. Provide a fallback relative import, or standardize on one style. Also verify the utility actually exists in the repo/package.
Apply this safer import pattern:
-from src.utils import count_model_params +try: + # src-layout (editable install) support + from src.utils import count_model_params +except ModuleNotFoundError: + # package-relative fallback for installed distributions + from ..utils import count_model_paramsRun to verify presence/signature (no imports executed):
#!/bin/bash rg -nP 'def\s+count_model_params\s*\(' -n src || { echo "Missing count_model_params utility"; exit 1; } rg -nP '\bcount_model_params\(' -n src/models/emotion_detection/training_pipeline.py
373-377: Fix: boolean comparison on tensors is ambiguousUsing
labels.sum() == 0yields a Tensor;ifon a Tensor raises “boolean value of Tensor is ambiguous”.- if labels.sum() == 0: + total_positives = int(labels.sum().item()) + if total_positives == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif total_positives == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
🧹 Nitpick comments (6)
src/models/emotion_detection/training_pipeline.py (6)
241-247: Clamp warmup steps to avoid scheduler misconfiguration when total_steps is smallIf
total_steps < warmup_stepsthe scheduler can be ill‑configured (and occasionally error depending on HF version). Clamp and log the adjustment.- total_steps = len(self.train_dataloader) * self.num_epochs + total_steps = len(self.train_dataloader) * self.num_epochs + # Guard: avoid warmup > total steps + effective_warmup = min(self.warmup_steps, max(0, total_steps - 1)) ... - self.scheduler = get_linear_schedule_with_warmup( + self.scheduler = get_linear_schedule_with_warmup( self.optimizer, - num_warmup_steps=self.warmup_steps, + num_warmup_steps=effective_warmup, num_training_steps=total_steps, ) + if effective_warmup != self.warmup_steps: + logger.info( + "Adjusted warmup steps from %d to %d to fit total steps=%d", + self.warmup_steps, effective_warmup, total_steps + )
249-254: Param-count logging works; minor nit on pre-formattingWorks as-is. If you want to keep lazy logging benefits, pre-format once and pass as an arg rather than formatting within the call.
- logger.info( - "Model initialized with %s trainable parameters", - format(count_model_params(self.model, only_trainable=True), ",d"), - ) + param_count = count_model_params(self.model, only_trainable=True) + logger.info("Model initialized with %s trainable parameters", f"{param_count:,}")
421-433: Avoid.data; use.detach()for grad norm computationAccessing
.datais discouraged and can lead to subtle autograd issues. Use.grad.detach().- for p in self.model.parameters(): - if p.grad is not None: - param_norm = p.grad.data.norm(2) + for p in self.model.parameters(): + if p.grad is not None: + param_norm = p.grad.detach().norm(2) total_norm += param_norm.item() ** 2 param_count += 1
439-442: Casting clip norm: prefer.item()for tensors
float(tensor)typically works for 0-D tensors, but.item()is explicit and avoids surprises across devices.- clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm + clip_val = clip_norm.item() if isinstance(clip_norm, torch.Tensor) else clip_norm logger.info(" Gradient norm after clipping: %.6f", clip_val)
560-571: Skip duplicate validation when early stop triggers mid-epochWhen
_maybe_validate_and_early_stopreturns early,train()still performs per-epoch validation immediately after, duplicating work. Short-circuit ifearly_stoppedis present.for epoch in range(1, self.num_epochs + 1): train_metrics = self.train_epoch(epoch) if self.evaluation_strategy == "epoch": + if train_metrics.get("early_stopped"): + self.training_history.append(train_metrics) + logger.info("Early stop: skipping end-of-epoch validation at epoch %d", epoch) + break val_metrics = self.validate(epoch)
102-102: Logging style is consistent but diverges from the PR objectiveYou’ve standardized on parameterized logging ("%s"/"%.4f") rather than f-strings. That’s fine (and preferred for lazy formatting), but it conflicts with the PR description claim “convert to f-strings”. Consider updating the PR text to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(17 hunks)
🔇 Additional comments (4)
src/models/emotion_detection/training_pipeline.py (4)
646-647: Breaking API/behavior: default dev_mode switched to False; PR text claims “behavior unchanged”Changing
dev_modedefault from True→False (and logging production-mode by default) alters runtime behavior and expected training time. Either revert the default, or update PR description, docs, and any callers.
- Confirm previous default and intended behavior.
- If keeping the new default, add a short note in the docstring and README/CHANGELOG.
Also applies to: 664-668
214-225: Loss/class-weight debug logging looks solidNice use of
logger.isEnabledFor(logging.DEBUG)gating and.item()extraction for scalars.
354-357: Epoch summary logging: solid and conciseClear, numeric formatting and consistent units.
542-546: Checkpoint naming/logging LGTMThe f-string path and post-save info log are clear and consistent.
…urity_headers.py, jwt_manager.py, training_pipeline.py, and hf_loader.py
…PyTorch parameter counting
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/emotion_detection/hf_loader.py (1)
201-210: Fix path traversal risk in archive extraction (tar/zip).Using tarfile.extractall and ZipFile.extractall on untrusted archives allows path traversal (e.g., files writing outside extract_dir). This is a high-severity security issue given the code downloads from arbitrary URLs.
Apply this diff to replace unsafe extraction with a safe extractor:
- if archive_path.endswith(".tar.gz") or archive_path.endswith(".tgz"): - with tarfile.open(archive_path, "r:gz") as tar: - tar.extractall(path=extract_dir) + if archive_path.endswith(".tar.gz") or archive_path.endswith(".tgz"): + with tarfile.open(archive_path, "r:gz") as tar: + _safe_extract_tar(tar, extract_dir) elif archive_path.endswith(".zip"): - import zipfile - - with zipfile.ZipFile(archive_path, "r") as zf: - zf.extractall(path=extract_dir) + import zipfile + with zipfile.ZipFile(archive_path, "r") as zf: + _safe_extract_zip(zf, extract_dir)Add these helpers (outside the shown range, e.g., near module top):
import logging logger = logging.getLogger(__name__) def _is_within(base: str, target: str) -> bool: base_abs = os.path.abspath(base) target_abs = os.path.abspath(target) return os.path.commonprefix([base_abs + os.sep, target_abs + os.sep]) == base_abs + os.sep def _safe_extract_tar(tar: tarfile.TarFile, path: str) -> None: for member in tar.getmembers(): dest = os.path.join(path, member.name) if not _is_within(path, dest): raise ValueError(f"Unsafe tar member path: {member.name}") tar.extractall(path=path) def _safe_extract_zip(zf, path: str) -> None: for name in zf.namelist(): dest = os.path.join(path, name) if not _is_within(path, dest): raise ValueError(f"Unsafe zip member path: {name}") zf.extractall(path=path)Optionally, log and clean up on errors to avoid leaving partial extracts behind. Would you like me to wire in robust cleanup/logging as well?
Also applies to: 202-209, 209-209
src/models/emotion_detection/training_pipeline.py (1)
566-582: Fix metric key names: use f1_macro and f1_micro.evaluate_emotion_classifier returns keys f1_macro and f1_micro. Accessing macro_f1/micro_f1 will KeyError.
Apply this diff:
- current_score = val_metrics["macro_f1"] + current_score = val_metrics["f1_macro"] @@ - logger.info("New best model saved! Macro F1: %.4f", current_score) + logger.info("New best model saved! Macro F1: %.4f", current_score) @@ - logger.info("Best validation Macro F1: %.4f", self.best_score) - logger.info("Final test Macro F1: %.4f", test_metrics["macro_f1"]) - logger.info("Final test Micro F1: %.4f", test_metrics["micro_f1"]) + logger.info("Best validation Macro F1: %.4f", self.best_score) + logger.info("Final test Macro F1: %.4f", test_metrics["f1_macro"]) + logger.info("Final test Micro F1: %.4f", test_metrics["f1_micro"])Also applies to: 712-716
♻️ Duplicate comments (2)
src/models/emotion_detection/training_pipeline.py (2)
22-22: Import style consistency: consider relative import or keep consistent with rest of file.This absolute import differs from the relative imports used below. Stick to one style for maintainability. If src.utils is a top-level util by design, ignore this.
Would you prefer me to switch to a relative import (e.g., from ...utils import count_model_params) if utils lives under src, or keep absolute across the file?
378-406: Fix ambiguous Tensor booleans in label checks.Using labels.sum() directly in if statements raises “boolean value of Tensor is ambiguous”.
Apply:
- if labels.sum() == 0: + if labels.sum().item() == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif labels.sum().item() == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
🧹 Nitpick comments (13)
src/models/emotion_detection/hf_loader.py (4)
186-194: Replace blanket try/except-pass with debug logging to aid diagnostics.Several branches swallow exceptions silently. At minimum, log at debug with the source that failed so operators can troubleshoot without enabling full tracing.
Proposed minimal changes:
- except Exception: - pass + except Exception as e: + logger.debug("Local dir load failed for %s: %s", local_dir, e)Repeat similarly for the HF Hub direct/snapshot and archive branches. Add
import loggingandlogger = logging.getLogger(__name__)at module top if not present.Also applies to: 214-229
176-183: Pathlib and platform-appropriate cache dirs.
- Prefer pathlib.Path over os.path.* for readability and to satisfy PTH1xx hints.
- Defaults like "/var/tmp/hf-cache" can trip security auditors. Consider using Hugging Face defaults or platform caches (e.g., appdirs.user_cache_dir).
Example:
-from os.path import join, dirname, basename, exists, isdir -from pathlib import Path +from pathlib import Path -cache_base = os.getenv("HF_HOME", "/var/tmp/hf-cache") -snap_dir = snapshot_download(repo_id=model_id, token=token, cache_dir=cache_base) +cache_base = Path(os.getenv("HF_HOME", str(Path.home() / ".cache" / "huggingface")))) +snap_dir = snapshot_download(repo_id=model_id, token=token, cache_dir=str(cache_base))Also applies to: 189-194, 215-217, 218-221
66-77: threshold argument is unused in remote inference.The predict signature exposes threshold, but the value isn’t applied. Either:
- Use it to filter or choose labels (align with HFEmotionDetector), or
- Drop it from the signature for consistency.
Would you like me to wire threshold-based top-K/threshold filtering to match local model behavior?
Also applies to: 84-99
214-221: Candidate discovery could be more selective.Scanning extract_dir plus all first-level subdirs is pragmatic, but consider limiting to directories containing both config.json and either pytorch_model.bin or model.safetensors to cut false positives.
I can propose a small helper that validates a candidate dir before attempting _wrap_local_model.
src/security/jwt_manager.py (3)
49-51: Comment is stale: Token pair is not a plain dict.The code returns a TokenResponse pydantic model, not a plain dict. Update the comment to avoid confusion.
-# Token pair is returned as a plain dict +# Token pair is returned as a TokenResponse model
69-71: Use timezone-aware UTC and consistent numeric timestamps.Naive datetimes (utcnow/fromtimestamp) trigger linter warnings and can cause subtle bugs. Prefer timezone-aware UTC and store numeric seconds in JWTs for consistency.
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone ... - "exp": datetime.utcnow() + timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES), - "iat": datetime.utcnow(), + "exp": datetime.now(timezone.utc) + timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES), + "iat": datetime.now(timezone.utc), ... - "exp": datetime.utcnow() + timedelta(days=REFRESH_TOKEN_EXPIRE_DAYS), - "iat": datetime.utcnow(), + "exp": datetime.now(timezone.utc) + timedelta(days=REFRESH_TOKEN_EXPIRE_DAYS), + "iat": datetime.now(timezone.utc), ... - exp_datetime = ( - datetime.fromtimestamp(exp_timestamp) if exp_timestamp else None - ) + exp_datetime = ( + datetime.fromtimestamp(exp_timestamp, tz=timezone.utc) if exp_timestamp else None + )Alternatively, encode
exp/iatas ints via.timestamp()to avoid tz conversion later.Also applies to: 81-85, 133-136
105-113: Avoid logging token material; prefer jti or a hash.Even partial token logs can be sensitive. Consider logging a hash or a JWT ID (jti) claim instead.
- logger.warning(f"Token expired: {token[:10]}...") + logger.warning("Token expired: %s", hashlib.sha256(token.encode()).hexdigest()[:12])Or add a
jticlaim at creation and log that.src/security_headers.py (5)
2-5: Docstring nits: Punctuation and spacing.Minor style fixes per D205/D415: end the summary with punctuation and leave a blank line before the description.
-"""🛡️ Security Headers Middleware -============================== -Flask middleware for adding security headers and implementing security policies. -""" +"""🛡️ Security Headers Middleware. + +============================== +Flask middleware for adding security headers and implementing security policies. +"""
68-75: Prefer pathlib and configurable config path.Pathlib improves readability, and a configurable security.yaml path (e.g., via Flask config or env) aids deployment across environments.
- config_path = os.path.join( - os.path.dirname(__file__), "../configs/security.yaml" - ) - with open(config_path) as f: + from pathlib import Path + base_dir = Path(__file__).resolve().parent + config_path = Path(os.getenv("SECURITY_CONFIG", base_dir / "../configs/security.yaml")).resolve() + with config_path.open() as f: security_config = yaml.safe_load(f)
92-99: PII/log volume caution for security logs.Logging remote_addr, X-Forwarded-For, and raw UA may be considered PII. Consider:
- Masking IPs (e.g., /24 aggregation) or hashing.
- Sampling logs in high-traffic paths.
- Making individual fields opt-in via SecurityHeadersConfig.
I can add config flags like redact_ips/redact_user_agent and a small helper to redact before logging—want me to draft it?
Also applies to: 220-236, 463-483
246-317: Deduplicate and precompile UA pattern lists.
- "checker" appears twice in low_risk_patterns.
- Consider converting lists to tuples or sets and deduplicating to avoid double counting.
- You can also precompute lowercase sets once for a small perf win.
- low_risk_patterns = [ + low_risk_patterns = [ "indexer", "feed", "rss", "aggregator", "monitor", - "checker", + "checker", "validator", "linter", - "checker", "analyzer", ]If you’d like, I can send a follow-up diff to use sets and avoid repeated scoring.
Also applies to: 318-331
24-44: Config surface duplication is confusing (enable_csp vs enable_content_security_policy, enable_hsts vs enable_strict_transport_security).You expose both names but only use the latter ones. Either:
- Drop the unused duplicates, or
- Wire them as synonyms to reduce confusion.
Happy to prep a cleanup commit mapping legacy names to the new ones to avoid breaking external code.
Also applies to: 487-516
src/models/emotion_detection/training_pipeline.py (1)
479-491: Minor: simplify clip_norm cast.Use a one-liner conditional cast (SIM108). No behavioral change.
- if not isinstance(clip_norm, (int, float)): - clip_val = float(clip_norm) - else: - clip_val = clip_norm + clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/models/emotion_detection/hf_loader.py(7 hunks)src/models/emotion_detection/training_pipeline.py(20 hunks)src/security/jwt_manager.py(9 hunks)src/security_headers.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/models/emotion_detection/training_pipeline.py (2)
src/models/emotion_detection/bert_classifier.py (2)
create_bert_emotion_classifier(385-411)evaluate_emotion_classifier(414-469)src/models/emotion_detection/dataset_loader.py (2)
GoEmotionsDataset(33-79)create_goemotions_loader(321-333)
src/security/jwt_manager.py (2)
src/unified_ai_api.py (1)
refresh_token(1029-1061)tests/unit/test_jwt_manager_extra.py (1)
utcnow(56-58)
🪛 Ruff (0.12.2)
src/models/emotion_detection/hf_loader.py
111-111: Use X | None for type annotations
Convert to X | None
(UP045)
112-112: Use X | None for type annotations
Convert to X | None
(UP045)
131-131: Use X | None for type annotations
Convert to X | None
(UP045)
131-131: Use X | None for type annotations
Convert to X | None
(UP045)
161-162: try-except-pass detected, consider logging the exception
(S110)
170-171: try-except-pass detected, consider logging the exception
(S110)
176-176: Probable insecure usage of temporary file or directory: "/var/tmp/hf-cache"
(S108)
183-184: try-except-pass detected, consider logging the exception
(S110)
189-189: Probable insecure usage of temporary file or directory: "/var/tmp/hf-cache"
(S108)
190-190: os.path.join() should be replaced by Path with / operator
(PTH118)
191-191: os.makedirs() should be replaced by Path.mkdir(parents=True)
(PTH103)
192-192: os.path.basename() should be replaced by Path.name
(PTH119)
193-193: os.path.join() should be replaced by Path with / operator
(PTH118)
209-209: Uses of tarfile.extractall()
(S202)
215-215: os.path.join() should be replaced by Path with / operator
(PTH118)
215-215: Use pathlib.Path.iterdir() instead.
(PTH208)
218-218: os.path.isdir() should be replaced by Path.is_dir()
(PTH112)
218-218: os.path.exists() should be replaced by Path.exists()
(PTH110)
219-219: os.path.join() should be replaced by Path with / operator
(PTH118)
src/security_headers.py
2-5: 1 blank line required between summary line and description
(D205)
2-5: First line should end with a period, question mark, or exclamation point
Add closing punctuation
(D415)
68-68: os.path.join() should be replaced by Path with / operator
(PTH118)
246-246: Too many branches (16 > 12)
(PLR0912)
src/models/emotion_detection/training_pipeline.py
486-489: Use ternary operator clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm instead of if-else-block
Replace if-else-block with clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
(SIM108)
542-542: Undefined name val_frequency
(F821)
551-551: Undefined name start_time
(F821)
src/security/jwt_manager.py
69-69: datetime.datetime.utcnow() used
(DTZ003)
70-70: datetime.datetime.utcnow() used
(DTZ003)
81-81: datetime.datetime.utcnow() used
(DTZ003)
82-82: datetime.datetime.utcnow() used
(DTZ003)
135-135: datetime.datetime.fromtimestamp() called without a tz argument
(DTZ006)
🔇 Additional comments (4)
src/security/jwt_manager.py (1)
156-170: LGTM: Efficient expired-token cleanup loop.Separation of collection and deletion avoids dict mutation during iteration; returns removed count. Solid.
src/security_headers.py (1)
118-126: Headers look correct; nice comprehensive coverage.Good explicit control over all major security headers and sane defaults.
Also applies to: 129-164
src/models/emotion_detection/training_pipeline.py (2)
263-268: Nice: switch to count_model_params utility and formatted logging.This produces clearer parameter counts and centralizes counting logic.
144-168: Dev-mode dataset scaling/logging reads well.The 5%/10% sampling and batch scaling with clear logs are helpful for quick iterations.
Resolved issues in src/models/emotion_detection/training_pipeline.py with DeepSource Autofix
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/models/emotion_detection/training_pipeline.py (2)
700-706: Avoid double dataset prep (dev_mode gets overwritten); threaddev_modethroughtrain.
train_emotion_detection_modelprepares data withdev_mode, thentrain()prepares again without it, effectively discarding dev-mode sampling. Unify the flow by passingdev_modeintotrain()and removing the extra call.-def train(self) -> Dict[str, Any]: +def train(self, dev_mode: bool = False) -> Dict[str, Any]: @@ - Returns: - Dictionary with training results and final metrics + Returns: + Dictionary with training results and final metrics @@ - datasets = self.prepare_data() + datasets = self.prepare_data(dev_mode=dev_mode)And in the convenience function:
- trainer.prepare_data(dev_mode=dev_mode) - - return trainer.train() + return trainer.train(dev_mode=dev_mode)Also applies to: 708-712, 833-835
273-284: Fix checkpoint load path:self.modelexists but isNone;hasattrwon’t initialize.
__init__setsself.model = None, sohasattr(self, "model")is True. You need to check forNone.- if not hasattr(self, "model"): + if self.model is None: datasets = self.prepare_data() class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
♻️ Duplicate comments (4)
src/models/emotion_detection/training_pipeline.py (4)
645-653: Fix metric keys: usef1_macrofrom evaluator.
evaluate_emotion_classifierreturnsf1_macro/f1_micro. Accessingmacro_f1will raise KeyError and break early-stopping/best-model logic.- current_score = val_metrics["macro_f1"] + current_score = val_metrics["f1_macro"] @@ - logger.info("New best model saved! Macro F1: %.4f", current_score) + logger.info("New best model saved! Macro F1: %.4f", current_score)
314-323: Fix NameError and misnamed arg: passval_frequencyandstart_time+ renameavg_loss→total_loss.
_maybe_validate_and_early_stopreferencesval_frequencyandstart_timethat are not in scope, and you’re passingtotal_lossinto a parameter namedavg_loss. This will throw at runtime and also miscomputes train loss.Apply at call site:
- maybe_metrics = self._maybe_validate_and_early_stop( - batch_idx, epoch, num_batches, total_loss, self.scheduler.get_last_lr()[0] - ) + maybe_metrics = self._maybe_validate_and_early_stop( + batch_idx=batch_idx, + epoch=epoch, + num_batches=num_batches, + total_loss=total_loss, + current_lr=self.scheduler.get_last_lr()[0], + val_frequency=val_frequency, + start_time=start_time, + )
593-627: Finalize_maybe_validate_and_early_stopAPI to match call site and fix docstring.Accept
val_frequencyandstart_time, and usetotal_lossto compute average correctly. Also fix the Returns docstring (it returns Optional[Dict], not bool).- def _maybe_validate_and_early_stop( - self, - batch_idx: int, - epoch: int, - num_batches: int, - avg_loss: float, - current_lr: float, - ) -> Optional[Dict[str, Any]]: + def _maybe_validate_and_early_stop( + self, + batch_idx: int, + epoch: int, + num_batches: int, + total_loss: float, + current_lr: float, + val_frequency: int, + start_time: float, + ) -> Optional[Dict[str, Any]]: @@ - Returns: - bool: True if training should stop early, False otherwise + Returns: + Optional[Dict[str, Any]]: Metrics snapshot if early-stopped, else None @@ - if (batch_idx + 1) % val_frequency != 0: + if (batch_idx + 1) % val_frequency != 0: return None @@ - return { + return { "epoch": epoch, - "train_loss": avg_loss / (batch_idx + 1), - "epoch_time": time.time() - start_time, + "train_loss": total_loss / (batch_idx + 1), + "epoch_time": time.time() - start_time, "learning_rate": current_lr, "early_stopped": True, }
456-470: Fix ambiguous Tensor boolean in label sanity checks.
if labels.sum() == 0:and== labels.numel()yield tensor booleans; branching on them raises “boolean value of Tensor is ambiguous”.- if labels.sum() == 0: + pos_sum = int(labels.sum().item()) + if pos_sum == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif pos_sum == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
🧹 Nitpick comments (3)
src/models/emotion_detection/training_pipeline.py (3)
253-260: Guard against warmup > total_steps.If
len(train_dataloader) * num_epochs≤warmup_steps, the scheduler will spend the entire training warming up. Clamp warmup tototal_steps - 1.- self.scheduler = get_linear_schedule_with_warmup( - self.optimizer, - num_warmup_steps=self.warmup_steps, - num_training_steps=total_steps, - ) + self.scheduler = get_linear_schedule_with_warmup( + self.optimizer, + num_warmup_steps=min(self.warmup_steps, max(0, total_steps - 1)), + num_training_steps=total_steps, + )
557-561: One-liner for castingclip_norm(SIM108).Minor style: use a ternary.
- if not isinstance(clip_norm, (int, float)): - clip_val = float(clip_norm) - else: - clip_val = clip_norm + clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
219-235: Debug gating is good; considerlogger.debugwithin debug-only helpers.You guard calls with
logger.isEnabledFor(DEBUG)but inner logs useinfo. Switching todebugkeeps log levels semantically aligned; no functional change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/models/emotion_detection/training_pipeline.py (2)
src/models/emotion_detection/bert_classifier.py (3)
create_bert_emotion_classifier(385-411)evaluate_emotion_classifier(414-469)unfreeze_bert_layers(128-146)src/models/emotion_detection/dataset_loader.py (1)
GoEmotionsDataset(33-79)
🪛 Ruff (0.12.2)
src/models/emotion_detection/training_pipeline.py
557-560: Use ternary operator clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm instead of if-else-block
Replace if-else-block with clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
(SIM108)
613-613: Undefined name val_frequency
(F821)
622-622: Undefined name start_time
(F821)
🔇 Additional comments (1)
src/models/emotion_detection/training_pipeline.py (1)
261-265: I’ve requested a scan for any existing parameter‐count helper undersrc/utils. Once we verify whethercount_model_params(or a similarly named function) exists—and its import path—we can confirm if the inline sum indeed needs replacement, or if the original comment about using a helper is incorrect.[next steps]
- If the helper exists, refactor to use it and update imports accordingly.
- If no helper exists, decide whether to implement
count_model_paramsinsrc/utils(or remove the suggestion).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/models/emotion_detection/training_pipeline.py (3)
278-285:load_model: check forNone, not attribute existence
self.modelis defined in__init__and may beNone.hasattr(self, "model")will be True, leading toNone.load_state_dict(...)and an exception. Check forNoneinstead. Also, avoid re‑preparing data unless needed.- if not hasattr(self, "model"): - datasets = self.prepare_data() - class_weights = datasets.get("class_weights") - self.initialize_model(class_weights) + if self.model is None: + datasets = self.prepare_data() + self.initialize_model(datasets.get("class_weights"))
698-701: Avoid double dataset preparation and dev_mode override
train_emotion_detection_modelalready callsprepare_data(dev_mode=...). Callingself.prepare_data()here redoes work and silently overrides dev-mode settings. Prefer a single entry point.Minimal, cohesive fix: let
train()drive preparation and acceptdev_mode, and remove the prepare step fromtrain_emotion_detection_model.
- Update
train()to acceptdev_modeand prepare data:- def train(self) -> Dict[str, Any]: + def train(self, dev_mode: bool = False) -> Dict[str, Any]: @@ - datasets = self.prepare_data() + datasets = self.prepare_data(dev_mode=dev_mode) class_weights = datasets.get("class_weights") self.initialize_model(class_weights)
- Remove the extra prepare call and forward
dev_mode:- trainer.prepare_data(dev_mode=dev_mode) - - return trainer.train() + return trainer.train(dev_mode=dev_mode)
775-776: Fix final test-metrics keys to match evaluator outputThe
evaluate_emotion_classifierfunction returns its F1 scores under the keysf1_microandf1_macro. Intraining_pipeline.py, the code is currently accessing non-existent keysmacro_f1andmicro_f1, which will raise aKeyError.Locations needing update:
src/models/emotion_detection/training_pipeline.pylines 775–776Suggested diff:
- logger.info(f"Final test Macro F1: {test_metrics['macro_f1']:.4f}") - logger.info(f"Final test Micro F1: {test_metrics['micro_f1']:.4f}") + logger.info(f"Final test Macro F1: {test_metrics['f1_macro']:.4f}") + logger.info(f"Final test Micro F1: {test_metrics['f1_micro']:.4f}")
♻️ Duplicate comments (4)
src/models/emotion_detection/training_pipeline.py (4)
22-25: Import style consistency: prefer one style (relative vs absolute) within this moduleThis file uses relative imports for local modules (Lines 24–25) but an absolute import for
src.utils(Line 22). Pick one style for maintainability; relative would befrom ...utils import count_model_paramsgiven the package layout.-from src.utils import count_model_params +from ...utils import count_model_params
314-317: Undefined names downstream: passval_frequencyandstart_time; also pass the correct loss accumulator
_maybe_validate_and_early_stopreferencesval_frequencyandstart_timebut they’re not in scope inside that method. Also, you passtotal_lossbut name the parameteravg_lossinside the callee. Pass both missing values and align the parameter name.- maybe_metrics = self._maybe_validate_and_early_stop( - batch_idx, epoch, num_batches, total_loss, self.scheduler.get_last_lr()[0] - ) + maybe_metrics = self._maybe_validate_and_early_stop( + batch_idx=batch_idx, + epoch=epoch, + num_batches=num_batches, + total_loss=total_loss, + current_lr=self.scheduler.get_last_lr()[0], + val_frequency=val_frequency, + start_time=start_time, + )
463-467: Fix ambiguous Tensor boolean comparisons
labels.sum() == 0andlabels.sum() == labels.numel()yield Tensor booleans, which are invalid inif. Convert to Python scalars.- if labels.sum() == 0: + total_positives = int(labels.sum().item()) + if total_positives == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif total_positives == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
585-618: Fix_maybe_validate_and_early_stop: undefined names, wrong param naming, and incorrect return docstring
- Uses
val_frequencyandstart_timewithout defining them.- Parameter
avg_lossis actually the runningtotal_loss.- Docstring “Returns: bool …” contradicts actual return type (
Optional[Dict[str, Any]]).Refactor signature and internal usage accordingly.
- def _maybe_validate_and_early_stop( - self, - batch_idx: int, - epoch: int, - num_batches: int, - avg_loss: float, - current_lr: float, - ) -> Optional[Dict[str, Any]]: - """Check if validation should be performed and handle early stopping. + def _maybe_validate_and_early_stop( + self, + batch_idx: int, + epoch: int, + num_batches: int, + total_loss: float, + current_lr: float, + val_frequency: int, + start_time: float, + ) -> Optional[Dict[str, Any]]: + """Check if validation should be performed and handle early stopping. @@ - Returns: - bool: True if training should stop early, False otherwise + Returns: + Optional[Dict[str, Any]]: Metrics snapshot if early-stopped; otherwise None. @@ - if (batch_idx + 1) % val_frequency != 0: + if (batch_idx + 1) % val_frequency != 0: return None logger.info(f"🔍 Validating at batch {batch_idx + 1}...") self.validate(epoch) if self.should_stop_early(): logger.info(f"🛑 Early stopping triggered at batch {batch_idx + 1}") return { "epoch": epoch, - "train_loss": avg_loss / (batch_idx + 1), + "train_loss": total_loss / (batch_idx + 1), "epoch_time": time.time() - start_time, "learning_rate": current_lr, "early_stopped": True, } return None
🧹 Nitpick comments (6)
src/utils.py (1)
5-5: Remove unused import to satisfy Ruff F401
from typing import Unionis unused in this module. Drop it to keep the file clean and pass linting.-from typing import Unionsrc/models/emotion_detection/training_pipeline.py (5)
371-379: Log “before clipping” truly before clippingYou compute
clip_grad_norm_(in‑place) and then log “before” stats. Swap order so diagnostics are accurate.- # Gradient clipping - clip_norm = torch.nn.utils.clip_grad_norm_( - self.model.parameters(), max_norm=1.0 - ) - - # Log gradient stats for first batch - if batch_idx == 0: - self._log_gradient_stats_before() - self._log_gradient_stats_after(clip_norm) + # Log gradient stats for first batch (before clipping) + if batch_idx == 0: + self._log_gradient_stats_before() + + # Gradient clipping + clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0) + + # Log gradient stats for first batch (after clipping) + if batch_idx == 0: + self._log_gradient_stats_after(clip_norm)
535-541: Avoid.datausage on grads; use.detach()or tensor directlyAccessing
p.grad.datais discouraged. Usep.grad(it’s already a Tensor) orp.grad.detach()when computing norms.- if p.grad is not None: - param_norm = p.grad.data.norm(2) + if p.grad is not None: + param_norm = p.grad.detach().norm(2) total_norm += param_norm.item() ** 2
554-559: Micro cleanup: simplifyclip_valassignmentUse a conditional expression as suggested by Ruff SIM108.
- if not isinstance(clip_norm, (int, float)): - clip_val = float(clip_norm) - else: - clip_val = clip_norm + clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
446-462: Level mismatch: debug-gated blocks log at INFOThese helpers early-return unless DEBUG is enabled, yet use
logger.infofor their messages. Consider switching tologger.debugto match intent (applies in_log_data_distribution,_log_model_output,_log_loss_analysis).Example:
- logger.info("🔍 DEBUG: Data Distribution Analysis") + logger.debug("🔍 DEBUG: Data Distribution Analysis")
384-391: Unify periodic progress logging via_log_progressYou already have
_log_progress, but here you inline a different format. Consider delegating to_log_progressfor consistency and easier tuning.- logger.info( - "Epoch %d, Batch %d/%d, Loss: %.8f", - epoch, batch_idx + 1, num_batches, loss.item() - ) + self._log_progress(epoch, batch_idx, num_batches, total_loss=total_loss + 0.0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/models/emotion_detection/training_pipeline.py(18 hunks)src/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/models/emotion_detection/training_pipeline.py (3)
src/utils.py (1)
count_model_params(8-21)src/models/emotion_detection/bert_classifier.py (3)
create_bert_emotion_classifier(385-411)evaluate_emotion_classifier(414-469)unfreeze_bert_layers(128-146)src/models/emotion_detection/dataset_loader.py (2)
GoEmotionsDataset(33-79)create_goemotions_loader(321-333)
🪛 Ruff (0.12.2)
src/utils.py
5-5: typing.Union imported but unused
Remove unused import: typing.Union
(F401)
src/models/emotion_detection/training_pipeline.py
554-557: Use ternary operator clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm instead of if-else-block
Replace if-else-block with clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
(SIM108)
605-605: Undefined name val_frequency
(F821)
614-614: Undefined name start_time
(F821)
🔇 Additional comments (4)
src/utils.py (1)
8-21: Utility looks correct and side‑effect freeImplementation returns exact parameter counts and respects
only_trainable. Clear docstring. No changes needed.src/models/emotion_detection/training_pipeline.py (3)
263-267: Nice improvement: centralized parameter-count loggingUsing
count_model_params(..., only_trainable=True)with thousands separator via f-string is clear and robust.
629-649: Validation flow is clear and logs are actionableGood use of metric gating and best‑model checkpointing. No required changes.
773-777: Overall logging improvements deliver the PR objectiveConverting curly-brace literals to f-strings and using the shared util for param counts significantly improves log clarity.
Resolved issues in src/utils.py with DeepSource Autofix
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/utils.py (2)
5-5: Remove unused import to satisfy linter (F401).
typing.Unionis imported but not used.-from typing import Union
1-1: Shebang is unnecessary for a library module.This file isn’t intended to be executed directly; consider removing the shebang to avoid implying executability.
-#!/usr/bin/env python3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
src/utils.py
5-5: typing.Union imported but unused
Remove unused import: typing.Union
(F401)
🔇 Additional comments (2)
src/utils.py (2)
8-20: Utility implementation LGTM.Straightforward and correct parameter counting; optional trainable-only mode is clear and efficient.
8-20: Allcount_model_paramscalls already useonly_trainable=True
I verified across the codebase that the only external invocation ofcount_model_params(insrc/models/emotion_detection/training_pipeline.pyat line 264) passesonly_trainable=True. There are no single-argument calls (which would default toFalse) nor any calls explicitly settingonly_trainable=False. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/emotion_detection/training_pipeline.py (1)
666-674: Fix metric keys: use f1_macro/f1_micro to match evaluator output
evaluate_emotion_classifierreturns "f1_macro" and "f1_micro", not "macro_f1"/"micro_f1". Current code will KeyError at runtime in both validate() and final summary logging.- current_score = val_metrics["macro_f1"] + current_score = val_metrics["f1_macro"] @@ - logger.info("Final test Macro F1: %.4f", test_metrics["macro_f1"]) - logger.info("Final test Micro F1: %.4f", test_metrics["micro_f1"]) + logger.info("Final test Macro F1: %.4f", test_metrics["f1_macro"]) + logger.info("Final test Micro F1: %.4f", test_metrics["f1_micro"])Cross-check: src/models/emotion_detection/bert_classifier.py returns f1_micro/f1_macro. Ensure any consumers elsewhere also use the same keys.
Also applies to: 805-807
♻️ Duplicate comments (4)
src/models/emotion_detection/training_pipeline.py (4)
22-22: Import style: prefer relative to stay consistent with local importsOther imports in this module use relative paths; switch this one as well for consistency across the file.
-from src.utils import count_model_params +from ...utils import count_model_params
381-390: Log “before clipping” stats before calling clip_grad_norm_
clip_grad_norm_updates gradients in-place. Currently “before” is logged after clipping, which is misleading. Swap the order.- # Gradient clipping - clip_norm = torch.nn.utils.clip_grad_norm_( - self.model.parameters(), max_norm=1.0 - ) - - # Log gradient stats for first batch - if batch_idx == 0: - self._log_gradient_stats_before() - self._log_gradient_stats_after(clip_norm) + # Log gradient stats for first batch (before clipping) + if batch_idx == 0: + self._log_gradient_stats_before() + + # Gradient clipping + clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0) + + # Log gradient stats for first batch (after clipping) + if batch_idx == 0: + self._log_gradient_stats_after(clip_norm)
483-487: Fix ambiguous Tensor boolean checks in logging helper
labels.sum() == 0yields a Tensor; using it inifraises “boolean value of Tensor is ambiguous”. Convert to Python scalars.- if labels.sum() == 0: + if labels.sum().item() == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif labels.sum().item() == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
315-327: Correct loss variable semantics and make call explicitYou pass
total_lossbut the callee parameter is namedavg_lossand then divided again, producing a double-division error in the returned metrics when early stopping triggers. Rename the parameter tototal_lossand compute the average inside; also use keyword args to prevent future ordering bugs.- maybe_metrics = self._maybe_validate_and_early_stop( - batch_idx, - epoch, - num_batches, - total_loss, - self.scheduler.get_last_lr()[0], - val_frequency, - start_time, - ) + maybe_metrics = self._maybe_validate_and_early_stop( + batch_idx=batch_idx, + epoch=epoch, + num_batches=num_batches, + total_loss=total_loss, + current_lr=self.scheduler.get_last_lr()[0], + val_frequency=val_frequency, + start_time=start_time, + ) @@ - def _maybe_validate_and_early_stop( + def _maybe_validate_and_early_stop( self, batch_idx: int, epoch: int, num_batches: int, - avg_loss: float, + total_loss: float, current_lr: float, val_frequency: int, start_time: float, ) -> Optional[Dict[str, Any]]: @@ - avg_loss: Average loss for current epoch + total_loss: Accumulated loss so far in the current epoch @@ - return { + return { "epoch": epoch, - "train_loss": avg_loss / (batch_idx + 1), + "train_loss": total_loss / (batch_idx + 1), "epoch_time": time.time() - start_time, "learning_rate": current_lr, "early_stopped": True, }Also applies to: 610-648
🧹 Nitpick comments (3)
src/models/emotion_detection/training_pipeline.py (3)
574-578: Nit: simplify cast per Ruff SIM108Inline the cast using a ternary for brevity; behavior unchanged.
- if not isinstance(clip_norm, (int, float)): - clip_val = float(clip_norm) - else: - clip_val = clip_norm + clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
307-309: Validation frequency guard: avoid “never validate within epoch” on small datasets
max(500, num_batches // 5)means for typical dev runs with <500 batches you will never hit intra-epoch validation. If you intend step-based validation, considermax(1, num_batches // 5). If you only validate per-epoch (default), consider skipping this block entirely whenevaluation_strategy == "epoch".- val_frequency = max(500, num_batches // 5) + val_frequency = max(1, num_batches // 5)
473-482: Use debug level inside debug-only helperThis helper runs only when DEBUG is enabled; emitting INFO-level lines can add noise if the gate changes later. Prefer
logger.debughere for consistency.- logger.info("🔍 DEBUG: Data Distribution Analysis") - logger.info(" Labels shape: %s", labels.shape) - logger.info(" Labels dtype: %s", labels.dtype) - logger.info(" Labels min: %s", labels.min().item()) - logger.info(" Labels max: %s", labels.max().item()) - logger.info(" Labels mean: %.6f", labels.float().mean().item()) - logger.info(" Labels sum: %s", labels.sum().item()) - logger.info(" Non-zero labels: %s", (labels > 0).sum().item()) - logger.info(" Total labels: %s", labels.numel()) + logger.debug("🔍 DEBUG: Data Distribution Analysis") + logger.debug(" Labels shape: %s", labels.shape) + logger.debug(" Labels dtype: %s", labels.dtype) + logger.debug(" Labels min: %s", labels.min().item()) + logger.debug(" Labels max: %s", labels.max().item()) + logger.debug(" Labels mean: %.6f", labels.float().mean().item()) + logger.debug(" Labels sum: %s", labels.sum().item()) + logger.debug(" Non-zero labels: %s", (labels > 0).sum().item()) + logger.debug(" Total labels: %s", labels.numel())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/models/emotion_detection/training_pipeline.py (3)
src/utils.py (1)
count_model_params(8-20)src/models/emotion_detection/bert_classifier.py (3)
create_bert_emotion_classifier(385-411)evaluate_emotion_classifier(414-469)unfreeze_bert_layers(128-146)src/models/emotion_detection/dataset_loader.py (1)
GoEmotionsDataset(33-79)
🪛 Ruff (0.12.2)
src/models/emotion_detection/training_pipeline.py
574-577: Use ternary operator clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm instead of if-else-block
Replace if-else-block with clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
(SIM108)
🔇 Additional comments (1)
src/models/emotion_detection/training_pipeline.py (1)
263-266: Good switch to shared util for parameter countingUsing the central utility improves consistency and testability. Nice touch formatting the count with thousands separators.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/emotion_detection/training_pipeline.py (1)
658-680: Use correct evaluator keys and ensure checkpoints save when save_best_only=Falseevaluate_emotion_classifier returns f1_macro/f1_micro (see bert_classifier.evaluate_emotion_classifier). Accessing macro_f1 will KeyError. Also, when save_best_only is False, no checkpoints are saved at all.
- current_score = val_metrics["macro_f1"] + current_score = val_metrics["f1_macro"] @@ - if self.save_best_only: - self.save_checkpoint(epoch, val_metrics, is_best=True) + if self.save_best_only: + self.save_checkpoint(epoch, val_metrics, is_best=True) + else: + # Save per-epoch checkpoint even if not "best only" + self.save_checkpoint(epoch, val_metrics, is_best=False)Optional: Consider always saving per-epoch checkpoints and, independently, updating best_model.pt when a new best is found.
♻️ Duplicate comments (4)
src/models/emotion_detection/training_pipeline.py (4)
22-25: Prefer consistent relative import for in-repo modulesTo align with the relative imports used elsewhere in this file (e.g., Lines 24–25), import the utility via a relative path. This avoids mixing import styles and eases refactors if the top-level package name changes.
-from src.utils import count_model_params +from ...utils import count_model_paramsIf the package layout lacks the necessary init.py files, keep the absolute import; otherwise, prefer the relative one for consistency.
381-390: “Before clipping” stats are logged after clipping — swap orderclip_grad_norm_ mutates gradients in-place. Log pre-clip stats before calling it, then log the returned post-clip norm.
- # Gradient clipping - clip_norm = torch.nn.utils.clip_grad_norm_( - self.model.parameters(), max_norm=1.0 - ) - - # Log gradient stats for first batch - if batch_idx == 0: - self._log_gradient_stats_before() - self._log_gradient_stats_after(clip_norm) + # Log gradient stats for first batch (before clipping) + if batch_idx == 0: + self._log_gradient_stats_before() + + # Gradient clipping + clip_norm = torch.nn.utils.clip_grad_norm_(self.model.parameters(), max_norm=1.0) + + # Log gradient stats for first batch (after clipping) + if batch_idx == 0: + self._log_gradient_stats_after(clip_norm)
483-487: Fix ambiguous tensor booleans in label-distribution checkslabels.sum() == 0 and labels.sum() == labels.numel() produce tensor booleans; use .item() to branch with Python booleans.
- if labels.sum() == 0: + sum_pos = int(labels.sum().item()) + if sum_pos == 0: logger.error("❌ CRITICAL: All labels are zero!") - elif labels.sum() == labels.numel(): + elif sum_pos == labels.numel(): logger.error("❌ CRITICAL: All labels are one!")
804-808: Final-summary metric keys must match evaluator outputSwitch to f1_macro and f1_micro to avoid KeyError at runtime.
- logger.info("Final test Macro F1: %.4f", test_metrics["macro_f1"]) - logger.info("Final test Micro F1: %.4f", test_metrics["micro_f1"]) + logger.info("Final test Macro F1: %.4f", test_metrics["f1_macro"]) + logger.info("Final test Micro F1: %.4f", test_metrics["f1_micro"])
🧹 Nitpick comments (3)
src/models/emotion_detection/training_pipeline.py (3)
545-566: Avoid .data in gradient norm analysisUsing .data bypasses autograd semantics; prefer .detach() for safe, read-only tensors.
- if p.grad is not None: - param_norm = p.grad.data.norm(2) + if p.grad is not None: + param_norm = p.grad.detach().norm(2) total_norm += param_norm.item() ** 2
574-579: Simplify clip-norm casting (Ruff SIM108)Tighten the cast to a single expression.
- if not isinstance(clip_norm, (int, float)): - clip_val = float(clip_norm) - else: - clip_val = clip_norm + clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
4-6: Docstring overclaims features not present hereThe module-level docstring mentions “focal loss, temperature scaling, and ensemble methods,” which aren’t implemented in this file. Consider trimming to avoid confusing readers.
-This module provides a comprehensive training pipeline for the BERT-based emotion -detection model with advanced features like focal loss, temperature scaling, and -ensemble methods. +This module provides a training pipeline for a BERT-based emotion +detection model with progressive unfreezing, detailed logging, and evaluation utilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/models/emotion_detection/training_pipeline.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/models/emotion_detection/training_pipeline.py (3)
src/utils.py (1)
count_model_params(8-20)src/models/emotion_detection/bert_classifier.py (3)
create_bert_emotion_classifier(385-411)evaluate_emotion_classifier(414-469)unfreeze_bert_layers(128-146)src/models/emotion_detection/dataset_loader.py (2)
GoEmotionsDataset(33-79)create_goemotions_loader(321-333)
🪛 Ruff (0.12.2)
src/models/emotion_detection/training_pipeline.py
574-577: Use ternary operator clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm instead of if-else-block
Replace if-else-block with clip_val = float(clip_norm) if not isinstance(clip_norm, (int, float)) else clip_norm
(SIM108)
🔇 Additional comments (5)
src/models/emotion_detection/training_pipeline.py (5)
145-168: Dev-mode dataset and batch-size logging reads clearlyNice clarity improvements: the dev-mode banner, reduced sampling, and explicit batch-size change log make runs easy to audit.
198-203: Dataset size summary is precise and cheapGood use of %d placeholders; avoids f-string eager formatting and logs exact sizes.
221-236: Loss/weights debug logs are gated correctlyDEBUG gating avoids noisy logs in INFO runs; numeric summaries (min/mean/max) are helpful for diagnosing class-weight issues.
263-268: Parameter-count logging via utility is correctUsing count_model_params with only_trainable=True and formatting with thousands separators is spot-on.
300-302: Progressive unfreezing hook is clean and well-scopedCalling _handle_progressive_unfreezing early in the epoch is appropriate; the helper logs context and delegates to model.unfreeze_bert_layers.
Also applies to: 331-341
…e_batch and use _log_progress in train_epoch, 2) Fix tensor comparison issues by adding .item() to weights.min() and weights.max(), 3) Implement actual blocking in security headers for high-risk user agents
…g, add explicit return None for PEP8 compliance, and fix long lines
…r count_model_params
chore(training): correct f-strings; log param counts via utils
Summary:
Fixed literal brace logging -> proper f-strings
Switched parameter logging to src/utils.count_model_params
File: src/models/emotion_detection/training_pipeline.py
Behavior unchanged; clearer, accurate logs
Summary by Sourcery
Improve training pipeline logging by converting all logger messages to f-strings for accurate interpolation and leverage the count_model_params utility for parameter count logging
Enhancements:
Summary by CodeRabbit
New Features
Behavior Changes
Refactor