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

fix(sdk): remove duplicate generate_id functions, replace shortuuid with secrets #4676

Merged
merged 17 commits into from
Jan 4, 2023

Conversation

moredatarequired
Copy link
Contributor

@moredatarequired moredatarequired commented Dec 21, 2022

Addresses WB-7375

Description

  • Replace the dependency on shortuuid and prefer the built-in module secrets for token (run id) generation
  • Consolidate the two nearly identical generate_id functions, move the callers depending on util to lib.runid

Testing

No additional tests.

Checklist

  • Include reference to internal ticket "Fixes WB-NNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
  • Ensure PR title compliance with the conventional commits standards

@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #4676 (4088eee) into main (d3e1e9e) will increase coverage by 40.50%.
The diff coverage is 97.95%.

❗ Current head 4088eee differs from pull request most recent head ce54c9b. Consider uploading reports for the commit ce54c9b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #4676       +/-   ##
===========================================
+ Coverage   42.80%   83.30%   +40.50%     
===========================================
  Files         279      280        +1     
  Lines       34555    34946      +391     
===========================================
+ Hits        14790    29111    +14321     
+ Misses      19765     5835    -13930     
Flag Coverage Δ
functest 55.65% <67.34%> (+14.51%) ⬆️
unittest 73.93% <97.95%> (+47.71%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
wandb/util.py 87.26% <ø> (+40.77%) ⬆️
wandb/data_types.py 82.89% <87.50%> (+51.05%) ⬆️
wandb/apis/public.py 81.90% <100.00%> (+51.65%) ⬆️
wandb/filesync/step_checksum.py 92.42% <100.00%> (-0.12%) ⬇️
wandb/sdk/data_types/base_types/json_metadata.py 93.10% <100.00%> (+43.10%) ⬆️
wandb/sdk/data_types/helper_types/image_mask.py 90.76% <100.00%> (+61.08%) ⬆️
wandb/sdk/data_types/html.py 95.31% <100.00%> (+61.46%) ⬆️
wandb/sdk/data_types/image.py 86.01% <100.00%> (+31.74%) ⬆️
wandb/sdk/data_types/molecule.py 89.15% <100.00%> (+63.54%) ⬆️
wandb/sdk/data_types/object_3d.py 96.15% <100.00%> (+65.08%) ⬆️
... and 229 more

@moredatarequired moredatarequired changed the title refactor(sdk): remove duplicate generate_id function fix(sdk): remove duplicate generate_id functions, replace shortuuid with secrets Dec 21, 2022
@moredatarequired moredatarequired marked this pull request as ready for review December 21, 2022 19:52
@moredatarequired moredatarequired requested a review from a team as a code owner December 21, 2022 19:52
@moredatarequired moredatarequired added this to the sdk-2023-01.1 milestone Dec 22, 2022
Copy link
Member

@dmitryduev dmitryduev left a comment

Choose a reason for hiding this comment

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

very nice, thanks @moredatarequired!

@dmitryduev dmitryduev enabled auto-merge (squash) January 4, 2023 03:05
@dmitryduev dmitryduev merged commit 4f272d3 into main Jan 4, 2023
@dmitryduev dmitryduev deleted the runid/consolidate-generate_id branch January 4, 2023 04:15
@github-actions github-actions bot added cc-fix and removed cc-fix labels Jan 4, 2023
@Linux-cpp-lisp
Copy link

This appears to have been missed in the docs...:
https://docs.wandb.ai/guides/track/advanced/resuming#automatic-and-controlled-resuming

Is it correct for client code following this example from the docs to just replace wandb.util.generate_id() with wandb.sdk.lib.runid.generate_id()? And is that second function available in earlier versions before v0.13.8 --- as in, if I make that change, will my code still be correct running on wandb<0.13.8?

Thanks!

@moredatarequired
Copy link
Contributor Author

@Linux-cpp-lisp , I'm sorry about that! None of the util functions are intended to be used by client code, so that example in our docs shouldn't have used it all along. I'll fix the example and put in a fix PR that restores the original function with a deprecation warning.

In the meantime, I actually suggest replacing the call to generate_id with any code that generates your own id. There's nothing special about our id generation, you just need to create (and save, if following our example) a unique id for each run. I suggest using secrets.token_urlsafe() since that requires no external libraries or implementation code.

@Linux-cpp-lisp
Copy link

Hi @moredatarequired --- thanks for the quick response!

Sounds good, I'll replace generate_id() with secrets.tocken_urlsafe().

... a unique id for each run

Is there any advantage to using a user-readable / provided ID? In our training framework (https://github.com/mir-group/nequip) we do force the user to provide a unique run_name, would there be any advantage of using this instad?

@moredatarequired
Copy link
Contributor Author

Is there any advantage to using a user-readable / provided ID? In our training framework (https://github.com/mir-group/nequip) we do force the user to provide a unique run_name, would there be any advantage of using this instead?

As long as the run id is url-safe and unique it should be fine. My worry about user-provided names would just be a lack of uniqueness, and if that's already enforced then it would be okay. Runs already have names that are more readable (generated names look like rosy-shadow-20), so I don't think there's an advantage to using a readable id, but it certainly won't cause a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants