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

handle CTK version specific options in the linker test #371

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ksimpson-work
Copy link
Contributor

This change skips tests when a specific nvjitlink exception is raised for invalid option, which happens if the option is invalid, or if the option is invalid for the given CTK.

I think we should also add cuda 12.4 to the test matrix because <21.3 targets the culink backend, and cuda 12.6 has options which aren't part of 12.4

Copy link
Contributor

copy-pr-bot bot commented Jan 10, 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 ksimpson-work self-assigned this Jan 10, 2025
@ksimpson-work ksimpson-work added P1 Medium priority - Should do cuda.core Everything related to the cuda.core module labels Jan 10, 2025
@ksimpson-work ksimpson-work requested a review from leofang January 10, 2025 00:16
@ksimpson-work
Copy link
Contributor Author

/ok to test

@leofang leofang added the enhancement Any code-related improvements label Jan 12, 2025
@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

@leofang leofang added this to the cuda.core beta 3 milestone Jan 15, 2025
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

Copy link

github-actions bot commented Jan 16, 2025

@leofang leofang requested a review from rwgk January 17, 2025 16:06
@rwgk
Copy link
Collaborator

rwgk commented Jan 17, 2025

I looked. I'll ping @ksimpson-work to chat 1:1.

@ksimpson-work ksimpson-work changed the title Add exception manager to handle options which aren't covered in all CTK versions handle CTK version specific options in the linker test Jan 17, 2025
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work ksimpson-work requested a review from leofang January 21, 2025 22:32
@ksimpson-work
Copy link
Contributor Author

/ok to test

cuda_core/tests/test_linker.py Outdated Show resolved Hide resolved
cuda_core/tests/test_linker.py Outdated Show resolved Hide resolved
@ksimpson-work
Copy link
Contributor Author

/ok to test

rwgk
rwgk previously approved these changes Jan 28, 2025
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 to me. (When I looked before the weekend I saw a CI error but it looked unrelated. Is it gone now?)

]
if not culink_backend:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested for line 22 above:

if culink_backend:
    nvjitlink = None
else:
    from cuda.bindings import nvjitlink

Then here:

if nvjitlink is not None:

That way the import appears only once, and near the top, where it is more expected.

@@ -2,6 +2,7 @@
#
# SPDX-License-Identifier: LicenseRef-NVIDIA-SOFTWARE-LICENSE


Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete the extra empty line again?

@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

1 similar comment
@ksimpson-work
Copy link
Contributor Author

/ok to test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module enhancement Any code-related improvements P1 Medium priority - Should do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants