Fix latent bugs surfaced by ty migration#646
Conversation
Resolves 4 `# ty: ignore` comments from PR #585 by fixing the underlying logic (not suppressing). Includes None-division risk in loss.py and masked None flow in optimizer code paths. See: docs/plans/20260518-ty-ignore-resolution-meta.md
The repo's default unit-test pattern is *_test.py (Makefile:29, tests/unit/main.py:8); the original name test_torchrec_utils.py uses the prefix form, so the new regression coverage for apply_sparse_optimizer would be silently skipped by make unit_test_py / CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/all_test |
GiGL Automation@ 17:55:04UTC : 🔄 @ 19:24:40UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:55:05UTC : 🔄 @ 18:04:08UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:55:06UTC : 🔄 @ 17:57:02UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:55:07UTC : 🔄 @ 19:19:34UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:55:07UTC : 🔄 @ 18:04:07UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 17:55:07UTC : 🔄 @ 18:53:11UTC : ✅ Workflow completed successfully. |
| @@ -100,18 +100,27 @@ def forward( | |||
|
|
|||
|
|
|||
| class SoftmaxLoss(nn.Module): | |||
There was a problem hiding this comment.
Side node, this isnt really Softmax loss but we have been calling it that.
cc @yliu2-sc - isnt this just a flavor of InfoNCE ?
There was a problem hiding this comment.
this is not even used in any runs or configs
| @@ -0,0 +1,83 @@ | |||
| """Regression tests for ``apply_sparse_optimizer`` in ``gigl.experimental.knowledge_graph_embedding.common.torchrec.utils``. | |||
There was a problem hiding this comment.
do we need test for this experimental code?
Are these auto generated?
There was a problem hiding this comment.
they were auto-generated, would you rather remove them?
| return { | ||
| CondensedEdgeType(0): BatchScores( | ||
| pos_scores=pos_scores, # ty: ignore[invalid-argument-type] TODO(ty-torch-tensor-specialization): cleared by PR 3 relaxing BatchScores to Tensor. | ||
| hard_neg_scores=hard_neg_scores, # ty: ignore[invalid-argument-type] TODO(ty-torch-tensor-specialization): cleared by PR 3 relaxing BatchScores to Tensor. |
There was a problem hiding this comment.
can we take a manual pass at this test first?
There was a problem hiding this comment.
Hmmm do you mean why are we adding the new type ignores? I have a follow-up pr which fixes these.
- Consolidate SoftmaxLoss regression coverage into the existing loss_test.py (one assertion on the default temperature). - Delete legacy_loss_test.py (was a misnamed parallel file created to avoid clobbering the existing loss_test.py). - Trim torchrec_utils_test.py from 4 tests to 1 — the actual regression path (defaults flowing through to RowWiseAdagrad). - Drop test-side narrative docstrings that re-described what the fix did rather than the contract under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
Resolves 4
# ty: ignorecomments from PR #585 by fixing the underlying logic (not suppressing). Includes None-division risk in loss.py and masked None flow in optimizer code paths.