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

Add random sample to guid #4763

Merged
merged 26 commits into from
Oct 28, 2022

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Oct 25, 2022

The current time resolution, especially on windows, is insufficient to ensure that we always avoid collisions in guids. Since sample names are available within the dataset metadata and the sample part of the guid is cumbersome to use I suggest that we replace that with a random string.

The broad plan is

  • Make guid type configurable
  • Warn if the sample name guid is set to something other than the default value.
  • Update tests that have problems with guid collisions to use the new guid type
  • Eventually change the default to the new type

For now we ami to completely remove the support of setting a sample_id. If actual use shows up we are open to allow this to be enabled as an explicit option

@jenshnielsen
Copy link
Collaborator Author

resolves #4504

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #4763 (95e8cd1) into master (0df4520) will decrease coverage by 0.92%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #4763      +/-   ##
==========================================
- Coverage   68.20%   67.28%   -0.93%     
==========================================
  Files         339      339              
  Lines       32003    32016      +13     
==========================================
- Hits        21828    21541     -287     
- Misses      10175    10475     +300     

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

to the open question: i agree, it might not be worth the effort to have 3 options, my hope is that due to lack of advertizing not many users used the guid customization.

qcodes/configuration/qcodesrc_schema.json Outdated Show resolved Hide resolved
qcodes/configuration/qcodesrc_schema.json Outdated Show resolved Hide resolved
qcodes/dataset/guids.py Outdated Show resolved Hide resolved
qcodes/dataset/guids.py Outdated Show resolved Hide resolved
@jenshnielsen jenshnielsen force-pushed the add_random_sample_to_guid branch 2 times, most recently from 7b5b349 to 851e736 Compare October 26, 2022 13:35
@jenshnielsen jenshnielsen marked this pull request as draft October 26, 2022 13:45
@jenshnielsen
Copy link
Collaborator Author

jenshnielsen commented Oct 27, 2022

Todo

  • Add suitable warnings to loading by sample
  • Check that docs are up to date

@jenshnielsen jenshnielsen marked this pull request as ready for review October 27, 2022 13:28
@jenshnielsen
Copy link
Collaborator Author

@astafan8 should be ready for a real review

@jenshnielsen jenshnielsen merged commit 3dc2755 into microsoft:master Oct 28, 2022
@jenshnielsen jenshnielsen deleted the add_random_sample_to_guid branch October 28, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants