Switch to native traceback in cuml
#6078
Conversation
galipremsagar
added
improvement
|
@galipremsagar those times are for the cuDF pytest suite? It might be worth to check the time savings for cuML, I got a fresh container so can check those times in the next couple of days if so |
|
@dantegd Yes, the timing improvements I posted above are of This PR: ==== 466 skipped, 9 xfailed, 201 xpassed, 25 warnings in 1432.43s (0:23:52) ====link: https://github.com/rapidsai/cuml/actions/runs/11002114744/job/30550052027?pr=6078#step:7:1802 Other most recently merged PR: ==== 466 skipped, 9 xfailed, 201 xpassed, 3 warnings in 1696.39s (0:28:16) =====link: https://github.com/rapidsai/cuml/actions/runs/10999896809/job/30543198516#step:7:1622 |
|
Super nice! I should try this for XGBoost as well. |
|
/merge |
|
@galipremsagar could you please check what's going wrong with the pytests in this PR? I can't quite figure out what the error is exactly. |
|
I merged in the upstream. Dask tests (and maybe others) still appear to be failing without a clear indication of the problem. It will probably be necessary to try a local reproduction of CI with more pytest verbosity. Maybe we can skip this change to Dask tests if it's just Dask tests failing. Still waiting on CI to find out. |
|
I am less excited about saving five minutes on 30minutes than everyone else here. Both those times are basically "infinite" in terms of developer experience, which means my workflow is something like "Make a change, start tests, context switch to something else, eventually come back to it later. Reconstruct context, act". This means you need a lot of information when you return to the test output to be able to reconstruct the context and figure out the failure. I had a quick google around but couldn't find examples of what the default (long?) vs native tracebacks look like in terms of information you get. Do we have some examples for cuml or cudf? The five minute saving is quickly wiped out if you need to re-run the test suite more often because you are missing information. |
|
@betatim We had a conversation about this on Slack and there was consensus that there is effectively no loss of information by making this change. It takes a lot of time for pytest to walk through all the traces and pretty-print things. I’ll link you to the discussions in Slack. See also: rapidsai/cudf#16851 (comment) |
|
I guess it is simple enough to switch back to full tracebacks if it turns out to be annoying. Though it seems the real solution is to not have so many xfailed tests. If anyone wants to tackle those :D |
Keep in mind that unfortunately this isn't really true when CI is so backed up that you come back and it hasn't run yet, which has historically been a fairly common occurrence. I's especially not true when we're up against a release timeline and we can't get things done fast enough, or if we have a global breaking change that requires rerunning CI on all PRs and it takes days to get caught up. |
I made this change in bf65363 and merged in the upstream. Hopefully CI will pass now. |
I tried reproing locally without timeouts but the tests are still running after 8hrs+. If these tests are designed to run under 2hrs on CI and they finish we should be good |
fixes #6194 Wheel tests in this project are emitting tons of warnings like this: > test_random_forest.py:1247 /__w/cuml/cuml/python/cuml/cuml/tests/test_random_forest.py:1247: PytestUnknownMarkWarning: Unknown pytest.mark.memleak - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html @pytest.mark.memleak I think that's because the introduction of a `pytest.ini` file in #6078 resulted in all of the `pytest` options from `pyproject.toml` being ignored. From https://docs.pytest.org/en/stable/reference/customize.html#pytest-ini > pytest.ini files take precedence over other files, even when empty. I think "take precedence" there means that if `pytest` finds a `pytest.ini`, it stops searching for other configuration files. Authors: - James Lamb (https://github.com/jameslamb) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Tim Head (https://github.com/betatim) - Jake Awe (https://github.com/AyodeAwe) - Vyas Ramasubramani (https://github.com/vyasr) URL: #6201
In cudf we have observed a ~10% speed up of pytest suite execution by switching pytest traceback to
--native:This PR makes similar change to
cuml.xref: rapidsai/cudf#16851