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.

Sign up for GitHub

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

Jump to bottom

device properties #409

Merged
merged 23 commits into from
Feb 14, 2025
Merged

device properties #409

merged 23 commits into from
Feb 14, 2025

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Jan 17, 2025 •
edited
Loading

close #210

@copy-pr-bot copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Jan 17, 2025

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 self-assigned this Jan 17, 2025
@ksimpson-work ksimpson-work added enhancement Any code-related improvements cuda.core Everything related to the cuda.core module breaking Breaking changes are introduced labels Jan 17, 2025
@ksimpson-work ksimpson-work added this to the cuda.core beta 3 milestone Jan 17, 2025
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
ksimpson-work
ksimpson-work commented Jan 23, 2025
View reviewed changes
cuda_core/cuda/core/experimental/_device.py
@@ -14,6 +14,1021 @@
_tls_lock = threading.Lock()


# ruff: noqa
class DeviceProperties:

This comment was marked as outdated.

Sign in to view

@ksimpson-work ksimpson-work requested review from rwgk and leofang January 27, 2025 20:37
@ksimpson-work ksimpson-work marked this pull request as ready for review January 27, 2025 20:37
rwgk
rwgk reviewed Jan 27, 2025
View reviewed changes
cuda_core/cuda/core/experimental/_device.py Outdated
def __init__(self):
raise RuntimeError("DeviceProperties should not be instantiated directly")

slots = "_handle"
Copy link
Collaborator

Choose a reason for hiding this comment

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

bug fix: __slots__

(I discovered that via the suggested added test.)

rwgk
rwgk previously approved these changes Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

This looks great!

@ksimpson-work
Copy link
Contributor Author

Thanks for your review @rwgk . Notable improvements :)

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

previous run encounter CI runner issue on one test. re-running the CI.

@ksimpson-work ksimpson-work mentioned this pull request Feb 6, 2025
Merged
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

@leofang this one should be good to go. Ralf has approved but I wanted to make sure you were ok with it

@ksimpson-work ksimpson-work enabled auto-merge (squash) February 12, 2025 19:01
leofang
leofang requested changes Feb 14, 2025
View reviewed changes
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.

there are in total 5 dynamic attributes that we should not cache

@ksimpson-work
Copy link
Contributor Author

/ok to test

rwgk
rwgk approved these changes Feb 14, 2025
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

The new commit looks good to me.

@leofang leofang disabled auto-merge February 14, 2025 19:32
@leofang
Copy link
Member

leofang commented Feb 14, 2025

CI is green, let me merge.

@leofang leofang merged commit af5a64e into NVIDIA:main Feb 14, 2025
69 checks passed
@leofang
Copy link
Member

leofang commented Feb 14, 2025

Thanks a lot, Keenan, Ralf, Daniel!

@leofang leofang added the P0 High priority - Must do! label Feb 14, 2025
@github-actions GitHub Actions
Copy link

Doc Preview CI
Preview removed because the pull request was closed or merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang requested changes

@carterbox carterbox carterbox left review comments

@rwgk rwgk rwgk approved these changes

Assignees

@ksimpson-work ksimpson-work

Labels
breaking Breaking changes are introduced cuda.core Everything related to the cuda.core module enhancement Any code-related improvements P0 High priority - Must do!
Projects
None yet
Milestone
cuda.core beta 3
Development

Successfully merging this pull request may close these issues.

Make device properties prettier
4 participants
@ksimpson-work @leofang @rwgk @carterbox