Skip to content

Conversation

@michaelmckinsey1
Copy link
Collaborator

@michaelmckinsey1 michaelmckinsey1 commented Nov 12, 2024

From the error message in develop, it is hard to tell which index is the duplicate when there are many profiles. In this example the duplicate is 3723623787.

develop
Screenshot from 2024-11-11 20-53-17

This PR
Screenshot from 2024-11-11 20-54-44

@michaelmckinsey1 michaelmckinsey1 self-assigned this Nov 12, 2024
@michaelmckinsey1 michaelmckinsey1 added area-utils Issues and PRs related to Thicket's internal utilities and helpers priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-internal-cleanup PRs or Issues related to the structure of the codebase, directories, and refactors labels Nov 12, 2024
@michaelmckinsey1 michaelmckinsey1 marked this pull request as ready for review November 12, 2024 23:39
@slabasan
Copy link
Collaborator

@michaelmckinsey1 Are you able to amend this PR to check for duplicates earlier? Can we update the error message to report collisions to us?

@slabasan slabasan added this to the 2025.1.0 milestone Feb 13, 2025
Copy link
Collaborator

@slabasan slabasan left a comment

Choose a reason for hiding this comment

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

I can't recall my note about checking for this "earlier", so merging this now and will revisit if it comes up again.

@slabasan slabasan merged commit 90bd510 into llnl:develop Feb 13, 2025
@michaelmckinsey1
Copy link
Collaborator Author

I can't recall my note about checking for this "earlier", so merging this now and will revisit if it comes up again.

I think we only discussed it verbally, but for the future: from what I remember I would basically need to restructure the validation checks to check earlier and add an additional check for hash collisions, which is on my todo list. This PR is strictly improving the way the existing check works.

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

Labels

area-utils Issues and PRs related to Thicket's internal utilities and helpers priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-internal-cleanup PRs or Issues related to the structure of the codebase, directories, and refactors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants