Try to reduce network usage in cuML tests. #6174
Conversation
| try: | ||
| df = get_boston_data() | ||
| except: # noqa E722 | ||
| pytest.xfail(reason="Error fetching Boston housing dataset") |
There was a problem hiding this comment.
Is there something else we can do here? My feeling is that by xfailing the test we are kinda saying "this will be a test that runs sometimes, but often it won't run. Eventually the test will break but no one will notice because it hardly ever runs. Until one day long after the change that broke the test when it will run and fail. Now someone from the future will have to scratch their head about what the heck just happened to them".
I am not sure what a good solution would be. Here some ideas:
- we now cache the dataset so should run into rate limits less often, so just deal with the occasional failure
- delete all the tests that use boston
- switch all the tests that use boston to some newer dataset (like california?)
- host the boston CSV somewhere we can more reliably fetch it from
There was a problem hiding this comment.
I chose the first (deal with the occasional failure) because that strategy is used elsewhere in the same tests for robustness. However, the other tests will retry network failures three times. I could do that here too if desired but I didn’t want to make the code more complex.
If the team desires something more substantive like a change in test data or hosting, I would recommend adopting that in a larger-scale rewrite of these tests. We must increase the robustness of our CI, even if it comes at a cost of coverage (if a download fails, just move on). It has been over 130 days since nightly CI passed.
There was a problem hiding this comment.
There is an open issue about switching away from the Boston dataset, #5158, so it’d be great if you want to give that a shot!
There was a problem hiding this comment.
Can you remove the pytest.xfail then?
There was a problem hiding this comment.
I think "just deal with the occasional failure" should come at a cost to coverage, and not at a cost to CI pass/fail. CI robustness (signal/noise) is too low to be useful at present, and we need to have occasional passes for nightly CI to be of any value.
There is already an open issue to move away from this long-deprecated dataset, and I would recommend that if the test is xfailing too consistently. In the meantime I can match the same behavior used for other datasets: retry the download 3 times, cache the file locally, and xfail the test if the download does not succeed.
There was a problem hiding this comment.
I added retry logic in 6ed322a. I now expect this test to be equally robust as the native sklearn fetch_* methods (aside from what is probably a different data host, GitHub vs. wherever fetch_* data comes from).
|
I like that we are using pytest machinery to solve this problem vs downloading the datasets at the start via some extra command or some such. LGTM modulo my comment about xfail |
|
/merge |
|
Interesting, this PR also fell into the same all-jobs-were-canceled issue as other PRs I saw. |
@dantegd The logs from earlier (https://github.com/rapidsai/cuml/actions/runs/12278231979/job/34262373197) showed an annotation that says:
I asked @galipremsagar why he canceled the jobs. He was trying to free up CI resources for a UCX release blocker. I asked him to notify the team on Slack next time this occurs -- and maybe only do it as a very-last-resort. As recompense for this annoyance, I asked him to finish up #6078. 😉 I was mostly teasing -- but he agreed to it! |
This PR tries to use
"session"scope pytest fixtures and cached data downloads to reduce network usage in cuML's nightly tests.