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

IQSS/10814 Improve dataset version differencing #10818

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Aug 29, 2024

What this PR does / why we need it: The change in this PR makes differencing during the UpdateDatasetVersionCommand for a dataset with 10K files go from taking ~ 8 seconds to <4 seconds (cold start) or even 17 ms (when the info in the versions has been read before). Presumably it improves the Version table on the dataset as well though I haven't tested that at scale.

The PR also includes some cleanup of the terms comparison - basically folding the possible null TermsOfUseAndAccess on the original or new version cases into one case by creating an empty TermsOfUseAndAccess for whichever one is missing, and adding a method to reduce repeated code.

Which issue(s) this PR closes:

Special notes for your reviewer: The test currently tests against the original algorithm and against a hardcoded expected value in the test. There's no real reason to test against the original algorithm except to validate that the hardcoded expected values in the test are correct, so I've marked the test code doing the check against the original as @deprecated.

The current tests don't do anything to test scaling (they basically try adding one file in each category (changed metadata, changed variable-level metadata, added file, deleted file, replaced file) and see if they differences are calculated as expected/as before. If someone wants to add a loop with ~1K files, we could probably start seeing the difference.

Suggestions on how to test this: QA should probably both confirm that the differencing appears to work for the Unit test cases (add a version or two and change metadata, add a file, delete a file, replace a file between versions) and make sure the difference makes sense. Checking the changed variable-level metadata probably requires testing with a tab file and perhaps editing with the DataCurator (Borealis tool for editing variable metadata) or editing in the db.

Scalability can be estimated by finding/creating a published dataset with several K files and checking the time to load the version table. (Maybe there's an API call?) I think all we need is to verify this is much faster/ worth the effort. (I added logging statements to print the start/stop times before/after the new/old algorithms and then ran the code on a dataset with 10K files. I've left a timing statement for the new algorithm in for fine logging, but the original code doesn't have any.)

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?: included - not sure it is needed, but perhaps as part of a general improved scalability note for multiple PRs.

Additional documentation:

@qqmyers qqmyers added GDCC: DANS related to GDCC work for DANS GDCC: QDR of interest to QDR labels Aug 29, 2024
@coveralls
Copy link

coveralls commented Aug 29, 2024

Coverage Status

coverage: 22.31% (+0.5%) from 21.856%
when pulling 89892a4 on GlobalDataverseCommunityConsortium:IQSS/10814-Improve_dataset_version_differencing
into b28812b on IQSS:develop.

@qqmyers qqmyers added the Size: 3 A percentage of a sprint. 2.1 hours. label Aug 29, 2024
@qqmyers qqmyers added the Consider For Next Release A simple change (eg bug fix) that would be good to prioritize since it has been seen in the wild label Sep 25, 2024
@cmbz cmbz added this to the 6.5 milestone Sep 30, 2024
@pdurbin pdurbin added the Type: Feature a feature request label Oct 9, 2024
@cmbz cmbz added the FY25 Sprint 10 FY25 Sprint 10 (2024-11-06 - 2024-11-20) label Nov 7, 2024
@sekmiller sekmiller self-assigned this Nov 12, 2024
Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

OK, this looks good. thanks for the typo fix.

@sekmiller sekmiller removed their assignment Nov 13, 2024
@ofahimIQSS ofahimIQSS self-assigned this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Consider For Next Release A simple change (eg bug fix) that would be good to prioritize since it has been seen in the wild FY25 Sprint 10 FY25 Sprint 10 (2024-11-06 - 2024-11-20) GDCC: DANS related to GDCC work for DANS GDCC: QDR of interest to QDR Size: 3 A percentage of a sprint. 2.1 hours. Type: Feature a feature request
Projects
Status: QA ✅
Development

Successfully merging this pull request may close these issues.

Feature Request: Improve Dataset version differencing performance
6 participants