Skip to content

Added UINT to assert in toArray#1269

Closed
punAhuja wants to merge 1 commit intorapidsai:branch-25.10from
SearchScale:puneet/to-array-assert-fix
Closed

Added UINT to assert in toArray#1269
punAhuja wants to merge 1 commit intorapidsai:branch-25.10from
SearchScale:puneet/to-array-assert-fix

Conversation

@punAhuja
Copy link
Copy Markdown
Contributor

Added assert for UINT datatype.
As per issue encountered and debugged by @narangvivek10 while working on Lucene HNSW serialization

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Aug 21, 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.

@chatman
Copy link
Copy Markdown
Contributor

chatman commented Aug 21, 2025

@narangvivek10 please add more details on how you encountered this issue. Also, any ideas how we can test for that scenario?

@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Sep 16, 2025
@mythrocks
Copy link
Copy Markdown
Contributor

It would be good to raise a bug indicating the error that @narangvivek10 ran into on this front, and add a "fixes XXXX" in the description of this PR. Maybe include a snippet of how the bug manifests in the PR description.

Copy link
Copy Markdown
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding this!

@ldematte
Copy link
Copy Markdown
Contributor

Also, any ideas how we can test for that scenario?

We are missing tests where the input data is int and the datatype is UINT. Those can be added to CuVSMatrixIT (e.g. testIntDatasetCopy -> also have testUIntDatasetCopy, etc.)

Maybe related/more importantly, we are missing a lot of tests on index operations (build, search, etc.): those are only with a host matrix/heap input (no device matrix) and only for float (no int or byte).
I have a plan to add those in the next week or so, but if anyone wants to do it you'll be more than welcome! :)

@ldematte
Copy link
Copy Markdown
Contributor

ldematte commented Sep 17, 2025

But wait, I believe your branch is just out of date -- I have fixed this already as part of #1232

EDIT: yep, very unfortunate timing: you raised this PR on August 21, and my PR got merged on August 22.
I think this can be closed.

@mythrocks
Copy link
Copy Markdown
Contributor

@punAhuja: Could you please check if the latest commits already address what this PR attempts to solve?

@punAhuja
Copy link
Copy Markdown
Contributor Author

@punAhuja: Could you please check if the latest commits already address what this PR attempts to solve?

Yes, the latest commits already address what this PR attempts to solve. Will close this PR.

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

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants