Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix _util.device_from_ctx #203

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Fix _util.device_from_ctx #203

merged 9 commits into from
Nov 12, 2024

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Oct 30, 2024

The obvious issue is that both of these call get_device_from_ctx which references the unimported Device(). Importing it would cause a circular dependency. Importing it within the get_device_from_ctx solves that issue. A couple other little issues in the method itself are also addressed.

Expected behaviour is: The tests added, should fail without each of the _utils changes. They should pass with the changes. This was observed locally.

Closes #202

Copy link

copy-pr-bot bot commented Oct 30, 2024

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.

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work ksimpson-work requested a review from leofang October 30, 2024 22:13
@ksimpson-work ksimpson-work changed the title remove the method and its references Remove _util.device_from_ctx and its references Oct 30, 2024
@ksimpson-work ksimpson-work mentioned this pull request Oct 30, 2024
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow. self._device_id could be None if we wrap a foreign stream using Device.create_stream(obj=some_foreign_stream_object). Then, the cuStreamGetCtx -> (set to the stream ctx) ->cuCtxGetDevice -> (set back to original ctx) combo is the only way to reliably retrieve the device ID. get_device_from_ctx is meant to make this combo cleaner.

How did you encounter a circular dependency? It might be an overlook in our test coverage depending on what the trigger path is. It is true that we forgot to import Device in get_device_from_ctx (good catch!), but circular dependency can be easily resolved by importing the object inside the function, as is already done in Stream.device.

@ksimpson-work
Copy link
Contributor Author

Thankyou for pointing out my oversight. I now clearly see that device_id is None when a foreign stream is provided. There are still some issues which I will seek to address, and will switch this from draft once I get it working / identify the problem.

@ksimpson-work
Copy link
Contributor Author

The changes

import Device inside function to avoid circular import
handle -> _handle
compare int(ctx) based on:

cdef class CUcontext:
"""

A regular context handle

Methods
-------
getPtr()
    Get memory address of class instance

"""
def __cinit__(self, void_ptr init_value = 0, void_ptr _ptr = 0):
    if _ptr == 0:
        self._ptr = &self.__val
        self._ptr[0] = <cydriver.CUcontext>init_value
    else:
        self._ptr = <cydriver.CUcontext *>_ptr
def __init__(self, *args, **kwargs):
    pass
def __repr__(self):
    return '<CUcontext ' + str(hex(self.__int__())) + '>'
def __index__(self):
    return self.__int__()
def __int__(self):
    return <void_ptr>self._ptr[0]
def getPtr(self):
    return <void_ptr>self._ptr

@ksimpson-work ksimpson-work marked this pull request as ready for review October 31, 2024 18:21
@ksimpson-work ksimpson-work changed the title Remove _util.device_from_ctx and its references Fix _util.device_from_ctx Oct 31, 2024
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, blocking the merge until we conclude beta 1 QA process.

cuda_core/tests/test_stream.py Outdated Show resolved Hide resolved
@leofang leofang added this to the cuda.core beta 2 milestone Nov 2, 2024
@leofang leofang added bug Something isn't working cuda.core Everything related to the cuda.core module P0 High priority - Must do! labels Nov 2, 2024
@ksimpson-work ksimpson-work merged commit 92aa731 into main Nov 12, 2024
@ksimpson-work ksimpson-work deleted the ksimpson/fix_device_from_ctx branch November 12, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuda.core Everything related to the cuda.core module P0 High priority - Must do!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Stream.device and Stream.context with respect to _util.device.from_ctx
2 participants