Skip to content

fix: reenable flaky tests on test_torch_kmeans.py#7849

Open
cswpy wants to merge 1 commit into
lance-format:mainfrom
cswpy:push-yroqxmxynuqn
Open

fix: reenable flaky tests on test_torch_kmeans.py#7849
cswpy wants to merge 1 commit into
lance-format:mainfrom
cswpy:push-yroqxmxynuqn

Conversation

@cswpy

@cswpy cswpy commented Jul 19, 2026

Copy link
Copy Markdown

Seems like the tests are not flaky anymore and we could re-enable them. Fixes #1683 .

@github-actions github-actions Bot added A-python Python bindings bug Something isn't working labels Jul 19, 2026
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Torch KMeans tests are re-enabled. One test uses explicit duplicate centroids with CPU execution, while the device-acceptance test passes an explicit CPU device.

Changes

Torch KMeans test execution

Layer / File(s) Summary
Deterministic test setup and CPU execution
python/python/tests/torch_tests/test_torch_kmeans.py
The tests are no longer skipped, KMeans uses explicit duplicate centroids, and accelerator training receives torch.device("cpu").

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: re-enabling previously flaky Torch k-means tests.
Description check ✅ Passed The description is directly related to the PR and correctly states that the tests are being re-enabled.
Linked Issues check ✅ Passed The PR addresses #1683 by fixing the broken torch tests in test_torch_kmeans.py and unskipping the affected cases.
Out of Scope Changes check ✅ Passed The changes stay focused on the targeted test file and its flaky test re-enablement.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@cswpy
cswpy marked this pull request as ready for review July 19, 2026 21:52

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
python/python/tests/torch_tests/test_torch_kmeans.py-19-23 (1)

19-23: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Seed the empty-cluster repair path. KMeans._updated_centroids still calls unseeded torch.rand_like when a cluster is empty, so this test can still vary run to run. Seed the CPU RNG locally or make the assertion independent of that perturbation.

🤖 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 `@python/python/tests/torch_tests/test_torch_kmeans.py` around lines 19 - 23,
Make the test around KMeans._updated_centroids deterministic by locally seeding
the CPU Torch RNG before kmeans.fit(arr), or revise its assertions so they do
not depend on the random empty-cluster repair perturbation. Keep the
duplicate-centroid setup and validate the intended empty-cluster recovery
behavior.
🤖 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.

Other comments:
In `@python/python/tests/torch_tests/test_torch_kmeans.py`:
- Around line 19-23: Make the test around KMeans._updated_centroids
deterministic by locally seeding the CPU Torch RNG before kmeans.fit(arr), or
revise its assertions so they do not depend on the random empty-cluster repair
perturbation. Keep the duplicate-centroid setup and validate the intended
empty-cluster recovery behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: e3bd19cf-0237-417e-a256-0e84eccf8d93

📥 Commits

Reviewing files that changed from the base of the PR and between 252d81a and a1a66d0.

📒 Files selected for processing (1)
  • python/python/tests/torch_tests/test_torch_kmeans.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-python Python bindings bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix torch related tests

1 participant