Conversation
There was a problem hiding this comment.
Pull request overview
Updates SASRec transformer unit tests to reflect changed recommendation ordering/output, likely as a consequence of dependency version changes.
Changes:
- Adjust expected
Columns.Itemsequences in SASRec recommendation tests (test_u2i_with_key_and_attn_masks,test_u2i_with_item_features).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pd.DataFrame( | ||
| { | ||
| Columns.User: [30, 30, 30, 40, 40, 40], | ||
| Columns.Item: [11, 13, 17, 17, 13, 11], | ||
| Columns.Item: [11, 13, 15, 17, 13, 11], | ||
| Columns.Rank: [1, 2, 3, 1, 2, 3], |
There was a problem hiding this comment.
The PR title/description indicate only dependency/lockfile bumps, but this hunk changes the expected recommendation output (items/ranks). Please update the PR description (and/or title) to explain the behavioral change and which dependency bump caused it, so reviewers can assess whether the new ranking is an intended/acceptable change rather than masking a regression.
| Columns.User: [30, 30, 30, 40, 40, 40], | ||
| Columns.Item: [11, 13, 17, 17, 13, 11], | ||
| Columns.Item: [11, 13, 15, 17, 13, 11], | ||
| Columns.Rank: [1, 2, 3, 1, 2, 3], |
There was a problem hiding this comment.
These tests assert an exact item ordering. In the recommendation stack, ranking is produced via torch.topk(..., sorted=True) (see TorchRanker.rank), which does not guarantee a deterministic order for ties and can change across torch versions / BLAS kernels even with deterministic flags. To reduce future lockfile-bump churn and flakiness, consider enforcing a deterministic tie-break (e.g., secondary sort by item id) in the ranker/recommendation code, or relax the test to assert an ordering rule (score desc + item asc) rather than a hardcoded list that may vary when scores are equal/near-equal.
| pd.DataFrame( | ||
| { | ||
| Columns.User: [30, 30, 30, 40, 40, 40], | ||
| Columns.Item: [11, 13, 12, 13, 14, 12], | ||
| Columns.Item: [11, 12, 13, 13, 14, 12], | ||
| Columns.Rank: [1, 2, 3, 1, 2, 3], |
There was a problem hiding this comment.
Same concern as above: this expected ordering change looks like it could be driven by tie/near-tie score ordering differences after dependency updates. To make this test robust across torch/pytorch-lightning bumps, prefer a deterministic tie-break in ranking (or assert ordering via an explicit secondary key) instead of hardcoding an order that can legitimately vary.
1 participant
Description
Type of change
How Has This Been Tested?
Before submitting a PR, please check yourself against the following list. It would save us quite a lot of time.