Add docs, tests, and samples for StridedMemoryView/@args_viewable_as_strided_memory

[leofang]

Close #143. Close #236.

• rename viewable to args_viewable_as_strided_memory
• rename the device_accessible data class member to is_device_accessible for better consistency
• fix the device_id data class member to -1 if the pointer is only accessible on CPU
• add docs, tests, and a code sample for StridedMemoryView & @args_viewable_as_strided_memory
• modify .gitattributes to enforce Linux line ending

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[rwgk]

Looks good to me!

[leofang]

Note: The diff renders weirdly because I changed the line ending in commit b5cfdce. The only true change is code added starting at line 44 (related to cffi)

[leofang]

Note: these were removed by Ruff by mistake (#201 (comment)). Fixing them here with noqa.

[leofang]

@rwgk @vzhurba01 This is ready for a final review. I've updated the PR description. I also took the liberty to rename the device_accessible data class member to is_device_accessible, because it is not yet a public API until after this PR. Same reasoning applies to args_viewable_as_strided_memory. (We did not document any of this back in v0.1.0.)

[leofang]

/ok to test

[rwgk]

Could this also be made to work with

def args_viewable_as_strided_memory(*arg_indices):

and then

    @args_viewable_as_strided_memory(1)
    def my_func(arg0, arg1, arg2, stream: Stream):

[leofang]

I did think of this and I thought it was discussed somewhere (internally) but I can't find it. One challenge I see is extensibility: What if we also want to support keyword arguments? Extending the current signature is straightforward:

def args_viewable_as_strided_memory(tuple arg_indices, tuple kwarg_names): ...

then on the call site

@args_viewable_as_strided_memory((1,), ("argB",))
def my_func(arg0, arg1, arg2, *, argA=None, argB=None): ...

But if we change the signature to naive *args, **kwargs can we still support this extension?

[leofang]

/ok to test

[rwgk]

When could this occur? I'd expect that the glob above produces only files that exist.

General experience: Explicitly cleaning up right before running a test is more helpful. That's the most certain way to ensure that the artifacts do not exist when the test starts, and in case the test fails, retaining the artifacts can be very useful for debugging.

If you think that idea could be useful here: git status will show the artifacts. I'd generate them in a subdir that we can .gitignore.

[leofang]

I would like to do the opposite: Ensure the artifacts do not exist after testing (regardless if tests succeed or not). The reason is that: I don't want any artifact to remain after the tests finish. I find it troublesome having to update .gitignore to skip the artifacts (and, depending on where we run the tests, the artifact location could change). Without an RAII-like clean up, they'd still remain on the file system after tests, and a subsequent local run (outside of pytest) could accidentally reuse them, which is considered a side effect that I do not like.

[rwgk]

Sounds good.

I'm still wondering when/why the except FileNotFoundError is needed, but it most likely will not do any harm to have it here.

[leofang]

It could happen if CFFI compilation fails for whatever reason (so that there's no artifact to remove).

[rwgk]

The list of files is the result of glob.glob(os.path.join(os.getcwd(), "_cpu_obj*")), which will only produce what actually exists in the filesystem (it could be empty for example).

>>> import glob
>>> glob.glob("_cpu_obj*")
[]

This could only go wrong if something concurrently deletes the files. (I wouldn't want to mask that.)

[leofang]

This could only go wrong if something concurrently deletes the files. (I wouldn't want to mask that.)

You might be onto something. I wrote this snippet a while back and perhaps back then the tests there were run under pytest-xdist which parallelized the tests.

[rwgk]

FWIW, I asked chatgpt for help with temp files. The answer suggests that it's really easy to avoid filesystem races with pytest:

https://docs.google.com/document/d/1IuhmAgtITcnvrEJY5BTkCb-taWw2Soj2YBmU535Ttoc/edit?usp=sharing

Use tmp_path Fixture:

tmp_path provides a Path object from Python's pathlib, which is modern and more feature-rich.
This is the recommended approach for most use cases as Path is more intuitive and robust.

[leofang]

Yes, we also use it in test_nvjitlink.py. It's great when it works, here it does not work because we have no control since we don't directly generate the artifacts, the sample does (think of it as a subprocess, but worse because we exec() the samples).

[rwgk]

FFI = None

I discovered that because I didn't have cffi when running the tests.

Which brings me to another question: Would it make sense to add cffi to the test dependencies (e.g. extras_require in setup.py)?

[leofang]

Good catch! Fixed in commit 6af4da3.

Would it make sense to add cffi to the test dependencies (e.g. extras_require in setup.py)?

Our test has not yet been enabled in the CI (#124). I would like to revisit this later, as the decision could go either way and I want to see how the CI test infra is set up before deciding. (The story with CFFI is a bit complicated, because we're using its "API mode" here which also needs a C/C++ compiler to present at run time.)

[rwgk]

Sounds good.

I'm still wondering when/why the except FileNotFoundError is needed, but it most likely will not do any harm to have it here.

[leofang]

Thanks, @rwgk (and Satya, who reviewed offline and suggested some comment additions in commit 6af4da3)! Let's give @vzhurba01 some time to review the docs before merging.

[vzhurba01]

I'm done with my review. This was my first proper look into both DLPack and CAI, so the tests and samples helped a lot in trying to understand what's going on.

[leofang]

Thanks, Vlad! Since Ralf has approved, and the last commit is minor, let me merge ahead.