Code Review and Working with Existing Codebases Questions

Direct answer

Treat code review as one of several teaching tools, not the only one: use written comments for things that are efficient to explain in text, switch to pairing for anything that needs real back-and-forth, and track whether the same class of comment keeps recurring as the honest signal of whether the mentee is learning, not just complying.

Structured elaboration

Structuring comments
Lead with the "why," not just "change this to that." Separate must-fix from optional or learning-opportunity comments explicitly, so the mentee isn't guessing at severity. Ask a question ("what happens if this list is empty?") instead of dictating the fix when the goal is for them to reason through it themselves; state it directly when time pressure or risk is high enough that the learning moment can wait.

Pair-programming versus commenting
Pair when the concept is genuinely new to them, a new pattern or a new part of the codebase, or when a comment thread has gone back and forth more than twice without converging. Use written comments for anything they've seen before and just need a nudge on, since it's asynchronous and doesn't interrupt their flow.

Targeted learning resources
Link to the specific doc, prior PR (pull request), or style-guide section that addresses the exact gap, not a generic "read the docs on X." Even better: point to a real example already in the codebase that does it well, since it's concrete and has already passed review.

Measuring progress
Track whether the same category of comment (e.g. "add error handling," "extract this function") shows up less often across their later PRs. A good sign is the mentee starting to anticipate the class of feedback you'd give and addressing it before you comment. A bad sign is the same class of feedback repeating PR after PR with no change in how the code arrives.

Worked example

Mentoring a junior engineer over a couple of months: early PRs draw frequent comments on missing error handling and untested edge cases. When they hit an unfamiliar part of the codebase (an async job queue), that becomes a pairing session rather than a comment thread, since it's genuinely new. For a recurring "extract this into a function" pattern, the mentor points them at a specific earlier PR in the codebase that does it well, rather than a generic style guide link. The measurable, honest signal by the third or fourth PR: the error-handling comments mostly stop appearing, and their tests start covering edge cases unprompted, not a fabricated precise percentage.

Trade-offs and pitfalls

Mentoring through review can tip into micromanaging, rewriting their solution in comments instead of letting them arrive at it themselves. It can also become one-sided, where the mentor never learns anything from the mentee's perspective on the code. A common wrong turn is being so gentle that a genuinely blocking issue reads as optional and ships anyway, which helps no one.

In an ML repository, "the tests pass" is not enough evidence that a change is safe, because a change can be functionally correct and still quietly alter what gets trained, how it is measured, or whether the run can be reproduced later. I treat an ML code review as answering three questions in order: does this change touch something that can silently shift results (data, randomness, or configuration), is there a fixed checklist item for that surface, and can I verify the fix with an automated check rather than trusting the diff by eye.

What the checklist covers

• Configuration: every training-affecting parameter lives in a versioned config file (not a hardcoded constant or an environment variable that differs by machine), and any change to a default bumps the config's version.
• Randomness: the random seed is explicitly set and propagated to every library that uses one (NumPy, PyTorch, TensorFlow), and the PR states whether determinism is expected end to end or only approximately, since some GPU operations stay non-deterministic even with a fixed seed.
• Data handling: any change to preprocessing, filtering, or how a dataset is split gets reviewed specifically for leakage (validation examples ending up in training) and for whether it silently changes the composition of an existing split, not just its size.
• Metrics and logs: metric names, units, and log schemas stay backward compatible, or the PR includes a migration note so historical experiment logs remain comparable to new ones.
• Model artifacts: every saved artifact is tagged with the training commit hash, dataset hash, hyperparameters, and framework versions, so a reviewer (or a future you) can tell exactly what produced it.

Worked example

Say a PR changes a training pipeline's split ratio from 80/20 to 75/25 on a 20,000-row dataset, but does not touch the random seed. Under the old split, training got 16,000 rows and validation got 4,000. Under the new split, training gets 15,000 and validation gets 5,000, a 1,000-row shift. The unchanged seed is misleading here: it controls which rows get shuffled, not where the split boundary falls, so the validation set's actual contents change even though nothing about the model architecture did. A reviewer following the checklist catches this because "changes a split" is a flagged data-handling item, and would ask the author to confirm whether the new validation metrics are still comparable to the last logged baseline, since a silently regenerated validation set can look like a model regression when it is really just a measurement change.

Trade-offs and pitfalls

Applying the full checklist to every PR, including ones that only touch a dashboard or a docstring, slows review velocity for no safety benefit, so scope it to PRs that touch the training or evaluation path and keep peripheral changes lightweight. A separate trap is capturing metadata (commit hash, dataset hash) without ever checking it: logging a hash is not the same as a continuous integration (CI) job that re-verifies the hash matches before allowing a merge. Finally, resist trusting a large notebook diff at face value, since notebook outputs and execution order hide state that a linear code diff will not surface; require a script-equivalent or a stripped notebook diff for anything that actually trains a model.

Direct answer

A rollback-safe rename plus backfill never touches the old column or existing readers directly. It adds the new column alongside the old one, writes to both while backfilling the new one in batches, verifies the backfilled data matches, only then switches reads over behind a flag, and keeps the old column around for a retention window so any step can be reversed just by flipping the flag back, not by undoing a destructive change.

Structured elaboration

Each phase below names what code review should specifically confirm before approving it, as a "Review check," plus what to monitor once it ships.

1. Schema change. Add the new column as nullable, with no constraints yet:

ALTER TABLE events ADD COLUMN new_name text NULL;

Review check: confirm this specific statement is additive only and backward-compatible, meaning every existing reader and writer keeps working unmodified the moment this ships, with zero application changes required yet.

2. Dual-write. Deploy an application change, behind a feature flag, that writes both the old and new column on every write to a row. Review check: is the write to both columns transactional or otherwise guaranteed consistent (not "write old, then separately and non-atomically write new"), and is the flag off by default so this ships dormant before anything depends on it?

3. Backfill. A batched, idempotent job fills in the new column for existing rows, only where it's still NULL, ordered by primary key, with a checkpoint so it can resume after an interruption instead of restarting from row one:

UPDATE events
SET new_name = old_name
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_name IS NULL;

Review check: is progress persisted somewhere durable (not just in the running process's memory), and does re-running an already-completed batch do nothing (true idempotence), not create incorrect data? Monitoring: track batches completed, rows backfilled, replication lag, and write error rate on the table for the duration of the backfill, and pause automatically if replication lag crosses an agreed threshold.

4. Verification. Before trusting the backfill, sample a random set of rows and confirm new_name matches what old_name implies for each. Review check: is the sample size and comparison method actually specified in the PR, not just asserted as "we verified it"?

5. Read cutover. Only after verification passes, flip the flag so reads prefer the new column, falling back to the old one if the new one is somehow still empty for a given row. Review check: is the fallback logic actually tested, not just written? Monitoring: application error rate and query latency on this table specifically, right after the flag flips, since a regression here is the trigger for the rollback shown in the diagram below.

6. Cleanup. Once reads have run on the new column successfully for a defined retention window, make it NOT NULL, add any index it needs, and only then drop the old column, in a separate, later PR. Review check: is dropping the old column genuinely a separate step from everything above, so it can never accidentally ship bundled with a change that hasn't been verified yet?

flowchart LR
  A[Step 1: add new_name column, nullable] --> B[Step 2: dual write old_name plus new_name]
  B --> C[Step 3: backfill new_name in batches where NULL]
  C --> D{Verification: sampled row values match}
  D -- mismatch found --> C
  D -- fully verified --> E[Step 4: flip reads to new_name behind a flag]
  E --> F{Error rate normal after cutover}
  F -- regression --> G[Rollback: flip flag back to old_name, dual write stays intact]
  F -- healthy --> H[Step 5: make new_name NOT NULL, add index concurrently]
  H --> I[Step 6: drop old_name after a retention window]

Worked example

Renaming user_email to primary_email on a table with 40 million rows, backfilled in batches of 5,000 rows: that's 40,000,000 / 5,000 = 8,000 batches total. With a short pause between batches to keep replication lag bounded, the job runs as a background process over however long it takes to work through all 8,000 batches, checkpointing its position after each one so a restart resumes from the last completed batch instead of row one. Verification samples 10,000 random rows after the backfill reports complete and confirms primary_email equals user_email for every one of them before the flag is ever flipped to prefer reads from the new column.

Trade-offs and pitfalls

Every step here is reversible specifically because the old column and old read path stay intact until the very last, separate cleanup step, which is exactly what makes this slower and more code than a single rename statement; that trade is worth it for a heavily-used column and wrong for a rarely-touched internal table, where a single migration with a maintenance window might be simpler and perfectly safe. The most dangerous version of this pattern to review is one that quietly combines two of these steps, most often shipping the read cutover and the old-column drop in the same change, which collapses the rollback safety the whole design exists to provide.

Direct answer

When a PR (pull request) touches how user data is stored or retrieved, review for four concrete things, what's collected, who can access it, how it's protected, and how it can be deleted, and insist on evidence for each one in the diff or PR description, since a comment claiming "it's handled" is exactly the gap that causes real incidents.

Structured elaboration

Worked example

A PR adds a "recently viewed items" feature that stores a user's view history in a new table. Review pass: data minimization, does the feature need the full page content, or just an item ID and timestamp? Access control, is the table filtered by the requesting user's ID at the query level, not just hidden in the UI? Logging, a new debug log line dumps the entire view-history payload including the user's session token; flagged and removed. Retention, the existing account-deletion job doesn't touch this new table yet; raised as a blocking comment before merge.

Trade-offs and pitfalls

Treating this checklist as something only a dedicated security or privacy reviewer does means it gets skipped on every "small" PR by everyone else, and most real incidents come from exactly this kind of small, unglamorous change. Blocking every PR pending a full compliance review for genuinely low-risk data is its own failure mode, it trains people to route around the review entirely. Calibrate scrutiny to the actual sensitivity of the specific field, rather than treating all user data as equally high-risk.

Direct answer

Each line does one job on the way to a clean, deduplicated, recent-events dataset: drop duplicate user-and-date rows, convert the date column from text to a real datetime type so it can be compared correctly, then filter down to events from 2023 onward. Walking a colleague through it means naming each line's input, output, and assumption, not just what the syntax does.

Structured elaboration

Line by line

1. df = df.drop_duplicates(subset=['user_id', 'event_date']). Input: the full dataframe (pandas' table-like data structure). Output: a dataframe with one row per unique (user_id, event_date) pair, keeping the first occurrence by default. Intent: treat repeated rows for the same user on the same date as duplicates, for example from a retried API call. Worth naming to a junior colleague: "first occurrence" is arbitrary unless the rows were sorted meaningfully first; if which duplicate survives matters, sort before deduplicating.
2. df['event_date'] = pd.to_datetime(df['event_date']). Input: the event_date column as text. Output: the same column as an actual datetime type. Intent: text can't be compared or filtered as a date correctly for every format; converting first makes the next line's comparison mean what it looks like it means.
3. df = df[df['event_date'] >= '2023-01-01']. Input: the dataframe with a real datetime column. Output: only rows on or after January 1, 2023. Intent: restrict analysis to a defined recent window; because the column is now a real datetime type, pandas correctly parses the string on the right for the comparison, rather than doing a plain text comparison.

Pitfalls

• to_datetime on an inconsistent or malformed date string raises an error by default rather than silently producing a wrong date, which is safer than it sounds but means the pipeline breaks hard on one bad row unless you explicitly decide how to handle it, for example converting unparseable values into a missing-date marker and then deciding whether to drop or flag them.
• Deduplicating before the date conversion means "duplicate" is judged on the raw string form of the date. Two rows representing the same date but formatted differently would NOT be caught as duplicates, since the strings differ. Converting to datetime before deduplicating avoids that trap.
• The final filter creates a view that's a slice of the original; further chained assignment on it can trigger pandas' copy-versus-view warning if not handled with an explicit copy, worth flagging to a junior colleague since it's a very common pandas gotcha.

Worked example

I ran this exact three-line snippet against a small sample dataset to confirm the behavior:

import pandas as pd

df = pd.DataFrame({
    'user_id': [101, 101, 102, 103, 103, 103],
    'event_date': ['2022-11-05', '2022-11-05', '2023-01-15', '2023-02-01', '2023-02-01', '2023-03-10'],
    'event_type': ['click', 'click', 'purchase', 'click', 'click', 'purchase'],
})
print("rows before:", len(df))

df = df.drop_duplicates(subset=['user_id', 'event_date'])
print("rows after dedup:", len(df))

df['event_date'] = pd.to_datetime(df['event_date'])
print("dtype after conversion:", df['event_date'].dtype)

df = df[df['event_date'] >= '2023-01-01']
print("rows after date filter:", len(df))
print(df)

Actual output:

rows before: 6
rows after dedup: 4
dtype after conversion: datetime64[us]
rows after date filter: 3
   user_id event_date event_type
2      102 2023-01-15   purchase
3      103 2023-02-01      click
5      103 2023-03-10   purchase

Six rows go to four after deduplication (the two identical 101, 2022-11-05 rows collapse to one), then to three after the date filter drops the pre-2023 row. I separately confirmed the pitfall above is real: feeding pd.to_datetime a list containing 'not_a_date' raises a ValueError naming the exact string that failed to parse, rather than silently producing a wrong date; passing errors='coerce' instead turns that value into a missing-date marker (one row) rather than raising.

One unit test to add

def test_dedup_and_filter_drops_pre_2023_and_duplicates():
    df = pd.DataFrame({
        'user_id': [1, 1, 2],
        'event_date': ['2022-12-31', '2022-12-31', '2023-01-01'],
    })
    df = df.drop_duplicates(subset=['user_id', 'event_date'])
    df['event_date'] = pd.to_datetime(df['event_date'])
    result = df[df['event_date'] >= '2023-01-01']
    assert len(result) == 1
    assert result['user_id'].iloc[0] == 2

This pins both behaviors together (the duplicate collapsed, the pre-2023 row excluded) against a known, hand-checkable expected result, so a future change to either line that breaks either behavior fails loudly instead of silently changing downstream numbers.

Trade-offs and pitfalls

• Silently dropping unparseable dates trades a hard failure for a soft one; only do this if you also log or count how many rows were affected, otherwise you lose visibility into a real data quality problem
• Deduplicating before type conversion is a subtle trap because it looks correct and usually is correct on clean data; it only breaks on inconsistently formatted date strings, exactly the kind of bug that survives review and shows up later as a mysterious discrepancy
• Comparing against a plain string relies on pandas correctly parsing it during the comparison; an explicit timestamp object is marginally more robust and easier for a junior engineer to trust at a glance