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

Add the options data class to program #237

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

Add the options data class to program #237

wants to merge 4 commits into from

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Nov 13, 2024 •
edited by leofang
Loading

closes #221

Add the options class to Program.

TODO modify the test so that it compiles the program with options once the CI is in place. I was running into compile issues that seemed related to my environment.

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

copy-pr-bot bot commented Nov 13, 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 ksimpson-work mentioned this pull request Nov 13, 2024
Open
@ksimpson-work ksimpson-work marked this pull request as ready for review November 13, 2024 00:39
warsawnv
warsawnv reviewed Nov 13, 2024
View reviewed changes
Copy link

@warsawnv warsawnv left a comment

Choose a reason for hiding this comment

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

Just some drive-by observations. Feel free to ignore!

cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
@leofang leofang added P1 Medium priority - Should do cuda.core Everything related to the cuda.core module enhancement Any code-related improvements labels Nov 14, 2024
@leofang leofang added this to the cuda.core beta 2 milestone Nov 14, 2024
@leofang
Copy link
Member

leofang commented Nov 14, 2024

Just some drive-by observations. Feel free to ignore!

Thanks, Berry 🙏 We welcome feedbacks and won't ignore anyone!

@leofang
Copy link
Member

leofang commented Nov 14, 2024

Another TODO: Let's also update the code samples and show the best practice!

@warsawnv
Copy link

Another TODO: Let's also update the code samples and show the best practice!

Is there a way to add these as doctests so that the code samples must always be kept in working order?

@ksimpson-work
Copy link
Contributor Author

@warsawnv Totally. I'm actively working on getting github actions working, and the examples are invoked as part of the testsuite.

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

Other than the default values for some program options, I believe all the comments and issues have been addressed. Thanks for your feedback @warsawnv and @leofang

leofang
leofang requested changes Nov 26, 2024
View reviewed changes
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_utils.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_program.py Show resolved Hide resolved
@ksimpson-work
Copy link
Contributor Author

Very good point

@leofang leofang mentioned this pull request Nov 27, 2024
Open
leofang
leofang reviewed Nov 28, 2024
View reviewed changes
cuda_core/docs/source/release/0.2.0-notes.md
Copy link
Member

@leofang leofang Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FYI it'd be 0.1.1 not 0.2.0

Copy link
Member

Choose a reason for hiding this comment

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

No, sorry I take It back. This PR introduces a breaking change so it cannot be part of v0.1.1. Keeping it as v0.2.0 is correct (meaning we won't merge this PR in this cycle).

Copy link
Member

Choose a reason for hiding this comment

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

(Let's discuss after the holidays.)

@leofang leofang removed the P1 Medium priority - Should do label Nov 28, 2024
@leofang leofang added P0 High priority - Must do! breaking Breaking changes are introduced labels Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@warsawnv warsawnv warsawnv left review comments

@leofang leofang leofang requested changes

@vzhurba01 vzhurba01 Awaiting requested review from vzhurba01

Requested changes must be addressed to merge this pull request.

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.

Program.compile() options parameter is not handled properly
3 participants
@ksimpson-work @leofang @warsawnv