test(bindings): Fix unsupported FS check in cufile#2233
Conversation
This is a follow-up from making cufile tests use temporary directories as noted by Leo I don't think QA is a problem, because the previous xfail was guarded by having `CI` in the environment variables. However, the `isSupportedFilesystem` was using the wrong directory now as we are now running the test in the temporary directory. My suspicion is that there is some additional check that would be strictly needed (e.g. to check that it isn't just ext4 but also directly mounted on a local nvme device) but I have not figured out a check for that.
github-actions
Bot
added
the
cuda.bindings
seberg
added
test
leofang
left a comment
leofang
left a comment
There was a problem hiding this comment.
A few items — main concern is fixture execution ordering (inline below): usefixtures order is not an execution-order guarantee, so driver can set up before the skip resolves and we'd hit cufile.driver_open() on tests we're about to skip (or error in driver setup on an unsupported FS instead of cleanly skipping). Docstring inversion and # noqa colon are quick fixes. Two minor nits.
Doesn't address #1307 itself — fine, that's a separate root-cause question we can keep open.
-- Leo's bot
|
|
||
| @pytest.mark.skipif(not isSupportedFilesystem(), reason="cuFile handle_register requires ext4 or xfs filesystem") | ||
| @pytest.mark.usefixtures("driver") | ||
| @pytest.mark.usefixtures("driver", "skipIfUnsupportedFilesystem") |
There was a problem hiding this comment.
usefixtures string order is not an execution-order guarantee — pytest orders fixtures by scope and dependency graph, not by position in the marker. With ("driver", "skipIfUnsupportedFilesystem") driver can (and likely will) set up first, so we'll run cufile.driver_open() on every test we then skip, and if driver setup itself touches the FS on an unsupported mount it'll error instead of cleanly skipping.
Suggest making the skip a dependency of driver (and stats) so it always resolves first:
@pytest.fixture
def driver(ctx, skipIfUnsupportedFilesystem):
...
@pytest.fixture
def stats(..., skipIfUnsupportedFilesystem):
...Then skipIfUnsupportedFilesystem can be dropped from each test's usefixtures list — the dependency carries it transitively.
There was a problem hiding this comment.
This seems like the bot is not thinking it through?
driver is used in basically all tests, this just skips some. If it depended on it, sure we skip some setup work but we would stop running tests that can run just fine?!
| This uses `findmnt` so the kernel's mount table logic owns the decoding of the filesystem type. | ||
| """ | ||
| fs_type = subprocess.check_output(["findmnt", "-no", "FSTYPE", "-T", os.getcwd()], text=True).strip() # noqa: S603, S607 | ||
| cmd = ["findmnt", "-no", "FSTYPE", "-T", tmpdir_factory.getbasetemp()] |
There was a problem hiding this comment.
Nit: tmpdir_factory.getbasetemp() returns py.path.local. subprocess coerces via os.fspath, so it works today, but explicit str(...) is more defensive against future py deprecation.
| cmd = ["findmnt", "-no", "FSTYPE", "-T", tmpdir_factory.getbasetemp()] | |
| cmd = ["findmnt", "-no", "FSTYPE", "-T", str(tmpdir_factory.getbasetemp())] |
There was a problem hiding this comment.
while I don't care, I really don't see why it makes sense to be "defensive" about this here.
| logging.info(f"Current filesystem type (findmnt): {fs_type}") | ||
| return fs_type in ("ext4", "xfs") | ||
| if fs_type not in ("ext4", "xfs"): | ||
| pytest.skip("cuFile handle_register requires ext4 or xfs filesystem") |
There was a problem hiding this comment.
Nit: worth a sentence in the PR body or docstring that on dev machines where the pytest basetemp lands on tmpfs (a common default for /tmp), the cuFile suite will now silently skip — previously it would attempt to run from CWD. Correct behavior, but a noticeable change for local devs that's worth flagging so nobody is puzzled by 100% skipped tests.
There was a problem hiding this comment.
Hmmmm, but this is a point because the tests did pass, but it might be that they are now indeed skipped (sometimes/always?).
I.e. things are working (maybe in cufile compat mode) here and skipping them would be just because of the FS check not being very good.
EDIT: Sorry, let me just run and see what is the actual situation and then rethink...
Co-authored-by: Leo Fang <leo80042@gmail.com>
2 participants
This is a follow-up from making cufile tests use temporary directories as noted by Leo
I don't think QA is a problem, because the previous xfail was guarded by having
CIin the environment variables.However, the
isSupportedFilesystemwas using the wrong directory now as we are now running the test in the temporary directory.My suspicion is that there is some additional check that would be strictly needed (e.g. to check that it isn't just ext4 but also directly mounted on a local nvme device) but I have not figured out a check for that.
(I don't love
pytest.skip()use, but no good way around as we need the tempdir.)