DM-37547: HSC exposure ID generator produces invalid IDs in 2023 #46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ktlim
reviewed
Jan 12, 2023
| self.assertEqual(last_group, int(20110101) * 100_000) | ||
|
|
||
| def test_exposure_id_hsc(self): | ||
| group = "2023011100026" |
There was a problem hiding this comment.
Why not just a number, avoiding the later int()?
There was a problem hiding this comment.
We get a lot of bugs related to integers being assigned as groups and then passed around like that, so I've insisted on other reviews that we always be strict with the types there. I could hardly make an exception for myself, especially since part of the problem is that we tend to think of group IDs as integers when formally they're not.
Broad queries for camera now always fail, so I picked the skymap as another "simple" dataset from the test data.
This table can be used by exposure ID generation algorithms to ensure that their output IDs are always valid.
kfindeisen
force-pushed
the
tickets/DM-37547
branch
from
493072e to
c501677
Compare
|
@ktlim, I implemented the revised generator (thanks for catching the 1.75-year caveat!). I'm still testing, but so far output looks reasonable: Can you take another look and let me know if the code meets with your approval? |
ktlim
approved these changes
Jan 12, 2023
The new ID generation algorithm is guaranteed to return an HSC ID in the correct range (0-21,474,800), and is less likely to have collisions from over-uploading. It is only valid until September 2024, but we hope that will be enough.
kfindeisen
force-pushed
the
tickets/DM-37547
branch
from
c501677 to
1d4b3d6
Compare
While we shouldn't need refresh for the local repo, because there is only one Butler for it, the central repo has one Butler per worker, and any individual worker may be long- or short-lived. While it's not a proper synchronization API, call `central_butler.registry.refresh` before operations that read from or write to the central repo.
kfindeisen
force-pushed
the
tickets/DM-37547
branch
from
1e267d3 to
9d972c8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a regression test for the bug where HSC exposure IDs were being generated with values of 30,000,000+, and rewrites the ID generation algorithm to confine them in the allowed range.