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

Opt-out validation for C++ accessor types and dimension #745

Merged
merged 7 commits into from
Jul 1, 2023

Conversation

RAMitchell
Copy link

@RAMitchell RAMitchell commented Jun 5, 2023

Check the type and dimension of C++ accessors. Adds a template parameter to optionally disable checking.

In case the type is not primative, check the size of the type.

It would be nice to also do this for reduce accessors, but reduce operators have a LHS and RHS type and I'm not sure if its correct to test one of these types.

@rapids-bot
Copy link

rapids-bot bot commented Jun 5, 2023

Pull requests from external contributors require approval from a nv-legate organization member with write or admin permissions before CI can begin.

@RAMitchell
Copy link
Author

I would have liked to write some tests for this, but not sure how in the current organisation of tests.

Copy link

@jjwilke jjwilke left a comment

Choose a reason for hiding this comment

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

I like the change. Thanks for putting this together.

I think this changes the existing behavior in a way we wouldn't want? Double check me?

#ifdef DEBUG_LEGATE
check_accessor_dimension(DIM);
#endif
if constexpr (VALIDATE_TYPE) {
Copy link

Choose a reason for hiding this comment

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

IIUC, this changes the existing behavior so that even non-debug builds will check the dimension. Previously dim is only checked for DEBUG_LEGATE.

Should we change this so that VALIDATE_TYPE defaults to true for DEBUG_LEGATE and false otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it wouldn't be too bad to turn it on by default even in release mode, as it'd only introduce a couple of checks

@jjwilke jjwilke added the category:improvement PR introduces an improvement and will be classified as such in release notes label Jun 5, 2023
@RAMitchell
Copy link
Author

RAMitchell commented Jun 5, 2023

It's intended to change behavior to increase type safety. If a type mismatch occurs it seems much more likely to me to be a programmer mistake than intended, hence the opt out instead of opt in behavior.

Copy link

@jjwilke jjwilke left a comment

Choose a reason for hiding this comment

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

I guess the check should be cheap and only done once per task. I'm fine with the change in existing behavior.

@RAMitchell
Copy link
Author

Can this be merged?

@RAMitchell
Copy link
Author

Test failures appear unrelated as no python code was changed.

@RAMitchell RAMitchell closed this Jun 29, 2023
@RAMitchell RAMitchell reopened this Jun 29, 2023
@magnatelee
Copy link
Contributor

@RAMitchell can you rebase this on the latest branch-23.07? I know your changes have nothing to do with those failures, but I want to make sure everything is green after the merge.

@magnatelee
Copy link
Contributor

@RAMitchell looks like all tests are now passing, so feel free to merge this PR. (I believe you're now a member of the group, so you should have permission.)

@RAMitchell RAMitchell merged commit f1fb142 into nv-legate:branch-23.07 Jul 1, 2023
manopapad pushed a commit that referenced this pull request Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants