PR #587 — Fix XGBoost retrain crash: tz-aware vs naive datetime TypeError in sample_weights#458
Conversation
📝 WalkthroughWalkthroughThe PR fixes a timezone handling bug in XGBoost weekly retraining. The sample-weight recency decay now normalizes both the current timestamp and parsed graded_at timestamp to timezone-naive datetimes before computing days elapsed, preventing type errors from mixing aware and naive datetime values. ChangesSample-weight recency calculation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Learning Topic: Security Misconfiguration (Detected by phrase)Matched on "security misconfiguration"Try a challenge in Secure Code Warrior |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Docker |  |
May 18, 2026 5:42p.m. | Review ↗ |
| JavaScript | |
May 18, 2026 5:42p.m. | Review ↗ |
| Python |  |
May 18, 2026 5:42p.m. | Review ↗ |
| SQL | |
May 18, 2026 5:42p.m. | Review ↗ |
| Secrets | |
May 18, 2026 5:42p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasklets.py`:
- Around line 7997-8000: The current code drops tzinfo naively which can skew
day differences; convert both timestamps to UTC before removing tzinfo: call
_parse_graded_at(row[2]) and convert it to UTC (e.g., via
astimezone(datetime.timezone.utc)) then replace(tzinfo=None) and do the same for
now_utc (or simply use
now_utc.astimezone(datetime.timezone.utc).replace(tzinfo=None)) so that the
computation inside sample_weights (uses _now_naive, _parse_graded_at, rows)
compares timestamps in UTC rather than dropping offsets in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| _now_naive = now_utc.replace(tzinfo=None) # PR #587: fix tz-aware vs naive TypeError | ||
| sample_weights = np.array([ | ||
| np.exp(-0.01 * max((now_utc - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | ||
| np.exp(-0.01 * max((_now_naive - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | ||
| for r in rows |
There was a problem hiding this comment.
Normalize aware timestamps to UTC before dropping tzinfo.
This avoids the TypeError, but replace(tzinfo=None) on an offset-aware graded_at drops the offset without conversion, which can skew .days and sample weights for non-UTC offsets.
Suggested fix
_now_naive = now_utc.replace(tzinfo=None) # PR `#587`: fix tz-aware vs naive TypeError
+ def _to_utc_naive(dt: datetime.datetime) -> datetime.datetime:
+ if dt.tzinfo is not None:
+ return dt.astimezone(datetime.timezone.utc).replace(tzinfo=None)
+ return dt
+
sample_weights = np.array([
- np.exp(-0.01 * max((_now_naive - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0))
+ np.exp(-0.01 * max((_now_naive - _to_utc_naive(_parse_graded_at(r[2]))).days, 0))
for r in rows
], dtype=np.float32)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _now_naive = now_utc.replace(tzinfo=None) # PR #587: fix tz-aware vs naive TypeError | |
| sample_weights = np.array([ | |
| np.exp(-0.01 * max((now_utc - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | |
| np.exp(-0.01 * max((_now_naive - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | |
| for r in rows | |
| _now_naive = now_utc.replace(tzinfo=None) # PR `#587`: fix tz-aware vs naive TypeError | |
| def _to_utc_naive(dt: datetime.datetime) -> datetime.datetime: | |
| if dt.tzinfo is not None: | |
| return dt.astimezone(datetime.timezone.utc).replace(tzinfo=None) | |
| return dt | |
| sample_weights = np.array([ | |
| np.exp(-0.01 * max((_now_naive - _to_utc_naive(_parse_graded_at(r[2]))).days, 0)) | |
| for r in rows |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tasklets.py` around lines 7997 - 8000, The current code drops tzinfo naively
which can skew day differences; convert both timestamps to UTC before removing
tzinfo: call _parse_graded_at(row[2]) and convert it to UTC (e.g., via
astimezone(datetime.timezone.utc)) then replace(tzinfo=None) and do the same for
now_utc (or simply use
now_utc.astimezone(datetime.timezone.utc).replace(tzinfo=None)) so that the
computation inside sample_weights (uses _now_naive, _parse_graded_at, rows)
compares timestamps in UTC rather than dropping offsets in place.
Up to standards ✅🟢 Issues
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a TypeError in tasklets.py by converting the timezone-aware now_utc variable to a naive datetime before calculating sample weights. The reviewer suggests a more robust long-term approach of maintaining timezone awareness throughout the calculation to prevent potential bugs with mixed timezones, providing a code suggestion for a helper function to ensure UTC awareness.
| _now_naive = now_utc.replace(tzinfo=None) # PR #587: fix tz-aware vs naive TypeError | ||
| sample_weights = np.array([ | ||
| np.exp(-0.01 * max((now_utc - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | ||
| np.exp(-0.01 * max((_now_naive - _parse_graded_at(r[2]).replace(tzinfo=None)).days, 0)) | ||
| for r in rows | ||
| ], dtype=np.float32) |
There was a problem hiding this comment.
While this fix is correct and solves the immediate TypeError, a more robust long-term solution is to work with timezone-aware datetimes consistently, rather than stripping timezone information. This prevents subtle bugs if data from different timezones is introduced in the future.
A better approach would be to ensure the graded_at datetime is timezone-aware (in UTC) before subtracting it from the aware now_utc. This makes the code's intent more explicit and safer.
Here's an alternative implementation for consideration, which could be placed inside run_xgboost_tasklet:
def _ensure_utc_aware(dt):
"""Helper to ensure a datetime is timezone-aware and in UTC."""
if dt.tzinfo is None:
# Assume naive datetimes from DB are in UTC.
return dt.replace(tzinfo=datetime.timezone.utc)
# Convert other aware datetimes to UTC.
return dt.astimezone(datetime.timezone.utc)
sample_weights = np.array([
np.exp(-0.01 * max((now_utc - _ensure_utc_aware(_parse_graded_at(r[2]))).days, 0))
for r in rows
], dtype=np.float32)This pattern of ensuring datetimes are aware before use is a good practice for robustness.
1 participant
Root Cause
run_xgboost_tasklet()has been crashing on every nightly 2:30 AM retrain since PR #575 merged. The crash is aTypeError: can't subtract offset-naive and offset-aware datetimesin thesample_weightscomprehension._parse_graded_at(r[2]).replace(tzinfo=None)strips timezone → naive datetime.now_utc - naive_datetime→ TypeError on every row, function crashes beforemodel.fit().This is why
xgb_model_storehas been empty since day 1. No model → base rate ~52-58% → MIN_PROB gate (60%) blocks all legs →eval_none=2182every dispatch → zero picks sent.Fix
One-line fix: strip timezone from
now_utctoo, so both operands are naive.Impact
After this PR merges and Railway redeploys, tonight's 2:30 AM retrain will:
xgb_model_store(INSERT works — verified manually)Note
A manually-trained model was inserted into
xgb_model_store(id=2) as a bridge fix for today's dispatch window. This PR ensures the automated 2:30 AM retrain works correctly going forward.Summary by cubic
Fixes a TypeError in the XGBoost retrain by aligning datetime types in
sample_weights. The nightly 2:30 AM retrain now completes and saves a model toxgb_model_store, enabling dispatch to use it.now_utcto a naive datetime before subtraction (_now_naive = now_utc.replace(tzinfo=None)) to prevent tz-aware vs naive errors in weight calculation.Written for commit 03648dc. Summary will update on new commits. Review in cubic
Summary by CodeRabbit