Repository navigation
Closed
Description
After #4888 which removed from distributed.utils_test import cleanup from files, the UCX test now fails as below:
________________________________________________________ ERROR at setup of test_large_cupy[1000000000] _________________________________________________________file /datasets/pentschev/miniconda3/envs/ucx-1.9-112-21.08.210608/lib/python3.8/site-packages/distributed/comm/tests/test_ucx.py, line 228
@pytest.mark.slow
@pytest.mark.asyncio
@pytest.mark.parametrize("n", [int(1e9), int(2.5e9)])
async def test_large_cupy(n, cleanup):
cupy = pytest.importorskip("cupy")
com, serv_com = await get_comm_pair()
arr = cupy.ones(n, dtype="u1") msg = {"op": "ping", "data": to_serialize(arr)}
_, result = await asyncio.gather(com.write(msg), serv_com.read()) data2 = result.pop("data")
assert result["op"] == "ping" assert len(data2) == len(arr)
await com.close()
await serv_com.close() E fixture 'cleanup' not found
> available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, event_loop, monkeypatch, pytestconfig, record_property,
record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory, unused_tcp_port, unused_tcp_port_factory > use 'pytest --fixtures [testpath]' for help on them.
/datasets/pentschev/miniconda3/envs/ucx-1.9-112-21.08.210608/lib/python3.8/site-packages/distributed/comm/tests/test_ucx.py:228This is a file which isn't tested by Distributed's CI currently, which is supposedly the reason the error above wasn't catched. However, it seems that https://github.com/dask/distributed/pull/4888/files#diff-a31c7ed5d35f5ed8233994868c54d625b18e6bacb6794344c4531e62bd9dde59R19-R20 should make the fixture available for all tests, I'm not sure though why that isn't the case for that file in particular.
@fjetter do you happen to have any ideas how to resolve the above without adding the import back? I verified that if I add the import back it then works.
Metadata
Metadata
Assignees
Labels
No labels
Activity
jrbourbeau commentedon Jun 10, 2021
Hmm interesting. Looking at the CI logs from rapidsai/ucx-py#750, it looks like the
rootdirreported bypytestis pointing to a directory within
distributed. Maybe theconftest.pyfile in the root of thedistributedsource, which contains thecleanupfixuture, isn't being picked up bypytest?jrbourbeau commentedon Jun 10, 2021
pytesthas a--rootdir=<path>option to manually specify therootdir. Could you try setting that to the root of thedistributedsource to see if that helps?pentschev commentedon Jun 10, 2021
Indeed you're right, thanks for checking this. I'm not very experienced with more advanced uses of pytest so didn't realize this could be the issue.
Using
--rootdirdoesn't seem to work unfortunately:But assuming it did work, it would still require users to download
distributed's source to run any of the pytests that depend ondistributed.utils_test.cleanupasconftest.pyisn't shipped, so from a packaging perspective this seems like a bug to me (having tests shipped that can't run without the source). Is that a wrong assumption from my part perhaps?jrbourbeau commentedon Jun 10, 2021
Good catch. If we stick with keeping plugins in the root
conftest.py, we should include that file indistributed/MANIFEST.inCan you try commenting out this line locally?
distributed/conftest.py
Line 37 in ee06a44
pentschev commentedon Jun 10, 2021
I see a different error now:
I'm not sure, maybe I'm using it incorrectly? Also, on the link you posted before for
--rootdirthere's no explicit mention toconftest.py, so perhaps it's not intended to be used with that file, but only with config files such aspytest.ini?jrbourbeau commentedon Jun 10, 2021
This feels like a step in the right direction as
pytesthas at least found/datasets/pentschev/src/distributed/conftest.pywhich contains the missingfixtures. The--runslowoption is also specific todistributed, but is added in the same rootconftest.pyfiledistributed/conftest.py
Lines 23 to 24 in ddec3c9
So I'm not sure why we're getting
ValueError: no option named 'runslow'. Perhaps @jakirkham has some thoughts on some good next steps to try?pentschev commentedon Jun 10, 2021
Yes, it has found it, I'm wondering whether it expects to do so or it has found it but isn't supposed to support it, and thus a bug in one direction or another.
fjetter commentedon Jun 11, 2021
I agree but frankly I was always wondering why we were shipping tests. If this is what we intend to do we should also add tests for this. anything wrong with checking out source?
While it looks the same, fixture discovery and plugin discovery works slightly differently even though it's all based in
conftest.py, see https://docs.pytest.org/en/6.2.x/writing_plugins.html#plugin-discovery-order-at-tool-startup and https://docs.pytest.org/en/6.2.x/reference.html#pytest.hookspec.pytest_addoptionI assume your particular way of calling things is now treating the root
conftest.pyas some sort deeply nested file and the docs for addoption already say that this is impossible. There is also a reference in https://docs.pytest.org/en/6.2.x/customize.html#rootdirthat config options are not possible using this but that isn't quite what's happening for us but maybe its related
My preference would be to simply not ship tests but there are probably good reasons for doing so I am not aware of.
Otherwise we could try to move all fixture and plugin code to a proper plugin (see https://docs.pytest.org/en/6.2.x/writing_plugins.html#making-your-plugin-installable-by-others). Effectively registering
distributed.utils_test+ addoptions as a pytest plugin and allow others to call it. That would allow us to remove the asterisk import as wellfjetter commentedon Jun 11, 2021
There it is. See my above comment about "nested conftest" files.
BTW is this something you can reproduce locally or is this only happening on CI?
pentschev commentedon Jun 11, 2021
I could check the source, there's no reason why we can't, it was more of a question as to what is the intended goal. If the tests are shipped, then I believe it should be ensured that they work, otherwise not shipping them at all seems fine.
Either alternative is fine by me, as long as we know what is appropriate for use.
I can reproduce locally, also reproducible with other tests that don't require UCX, e.g.:
jrbourbeau commentedon Jun 11, 2021
That does seem like the simpler option. Perhaps @mrocklin @jcrist or @jakirkham know why we ship tests?
mrocklin commentedon Jun 14, 2021
Historically some projects encouraged users to run tests to make sure that things worked. Something like the following:
This predated widespread use of tools like pytest though.
I think that it's fine to not ship tests.
jrbourbeau commentedon Jun 23, 2021
@pentschev @madsbk it looks like rapidsai/dask-cuda#658 updated
dask-cuda's CI to checkout thedistributedsource. Is it okay to close this issue?jakirkham commentedon Jun 23, 2021
Peter is currently OOTO. Let's wait until he's back
pentschev commentedon Jun 28, 2021
We updated our CI to indeed resolve this by cloning the distributed repo and running tests from there, so from our side the issue was resolved. However, do we have a resolution on what distributed will do moving forward, will it stop shipping tests? I think it would be good to properly deal with the tests (even if that means not shipping them anymore) to prevent future users from seeing the same issue.
jakirkham commentedon Jul 13, 2021
@jrbourbeau do you have any thoughts here?
jrbourbeau commentedon Jul 13, 2021
Yeah, not shipping tests sounds reasonable to me. I opened up #5054 to update our
MANIFEST.inaccordingly. @jakirkham if you get a chance, any feedback you may have on that PR is welcomejakirkham commentedon Jul 22, 2021
Now that PR ( #5054 ) is in, are we good to close? Or is there anything else needed here @pentschev?
jrbourbeau commentedon Jul 22, 2021
I think this is good to close, though @pentschev let me know if there's anything else to do here
pentschev commentedon Jul 22, 2021
Yup, all good from my side. Thanks @jakirkham and @jrbourbeau !
QuLogic commentedon Mar 6, 2022
This is unfortunate, since versioneer is totally busted from GitHub tarballs. Can tests be restored in sdists, but not installed?