Skip to content

Fix test of cudf spilling metrics#8478

Merged
quasiben merged 1 commit into
dask:mainfrom
madsbk:test_cudf_metrics
Feb 2, 2024
Merged

Fix test of cudf spilling metrics#8478
quasiben merged 1 commit into
dask:mainfrom
madsbk:test_cudf_metrics

Conversation

@madsbk

@madsbk madsbk commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

The current implementation of test_cudf_metrics() sometimes fails because it doesn't wait for the worker's metrics to update. Fixed in this PR.

Is it possible to wait on a worker's metrics to update? In test_rmm_metrics() and now in test_cudf_metrics(), we use await asyncio.sleep(1), which isn't ideal.

  • Tests added / passed
  • Passes pre-commit run --all-files

cc. @charlesbluca

@github-actions

Copy link
Copy Markdown
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    27 files  ± 0      27 suites  ±0   9h 56m 16s ⏱️ + 12m 16s
 3 964 tests ± 0   3 851 ✅  -  2    109 💤 ±0  4 ❌ +2 
49 853 runs  +18  47 560 ✅ +22  2 289 💤  - 6  4 ❌ +2 

For more details on these failures, see this check.

Results for commit 2734b0f. ± Comparison against base commit e30a3e4.

@madsbk madsbk marked this pull request as ready for review January 24, 2024 14:25
@madsbk madsbk requested a review from fjetter as a code owner January 24, 2024 14:25
@charlesbluca

Copy link
Copy Markdown
Member

rerun tests

@charlesbluca charlesbluca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @madsbk! Approving for now as I'd like to unblock GPU CI as soon as possible, but worth noting that it does seem like this method has issues in test_rmm_metrics that make it flaky:

https://gpuci.gpuopenanalytics.com/job/dask/job/distributed/job/prb/job/distributed-prb/8309/

@madsbk

madsbk commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

rerun tests

@rjzamora rjzamora left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a good fix. CI failures are unrelated.

@quasiben

quasiben commented Feb 2, 2024

Copy link
Copy Markdown
Member

Planning to merge later this afternoon unless there are concerns

@quasiben quasiben merged commit 4425516 into dask:main Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants