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

build(sdk): remove dependency on six by modifying vendored libs #4280

Merged
merged 4 commits into from Dec 5, 2022

Conversation

dmitryduev
Copy link
Member

@dmitryduev dmitryduev commented Sep 16, 2022

Fixes WB-NNNN
Fixes #NNNN

Description

This PR removes dependency on six by modifying vendored libs gql and graphql-core.

Concretely what I did was:

  • Downloaded the vendored versions of the libraries from PyPI (tar balls)
  • Initialized git in the uncompressed directories, applied our wandb-vendor diff's (manually)
  • Identified all files that used six with git grep " six" (to catch import six and from six import ...)
  • Ran pyupgrade --py36-plus <filename> on each identified file and manually made sure the result was good.
  • Generated new diffs with git diff > wandb-vendor.diff, ensured they looked good
  • Copied stuff over to the SDK and checked the diffs once again.

Testing

CI!

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

@dmitryduev dmitryduev requested a review from a team September 16, 2022 07:09
@dmitryduev dmitryduev added this to the sdk-2022-11.1 milestone Oct 3, 2022
@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #4280 (dca841e) into main (202de65) will increase coverage by 0.04%.
The diff coverage is n/a.

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4280      +/-   ##
==========================================
+ Coverage   83.07%   83.12%   +0.04%     
==========================================
  Files         267      267              
  Lines       33739    33739              
==========================================
+ Hits        28030    28046      +16     
+ Misses       5709     5693      -16     
Flag Coverage Δ
functest 55.84% <ø> (+0.04%) ⬆️
unittest 73.46% <ø> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
wandb/integration/tensorboard/monkeypatch.py 91.35% <0.00%> (-2.47%) ⬇️
wandb/sdk/lib/git.py 79.01% <0.00%> (ø)
wandb/sdk/wandb_run.py 90.89% <0.00%> (+0.05%) ⬆️
wandb/sdk/wandb_setup.py 88.94% <0.00%> (+0.50%) ⬆️
wandb/sdk/internal/internal_api.py 89.79% <0.00%> (+0.51%) ⬆️
wandb/sdk/lib/printer.py 89.24% <0.00%> (+0.63%) ⬆️
wandb/sdk/internal/file_stream.py 89.86% <0.00%> (+1.01%) ⬆️
wandb/filesync/step_prepare.py 94.93% <0.00%> (+1.26%) ⬆️
wandb/sdk/internal/system/system_info.py 91.55% <0.00%> (+3.24%) ⬆️
wandb/errors/term.py 91.30% <0.00%> (+4.34%) ⬆️

Copy link
Member

@raubitsj raubitsj left a comment

Choose a reason for hiding this comment

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

I think updating the wandb-vendor.md files (3 of them) with the procedure mentioned in the PR (or one line summary of it so people can find the change from the PR), and optionally a separate diff for the pyupgrade stuff (rather than merging them into one diff) makes sense to me.
The diff is bonus... the main thing we want to know is the steps done on the base vendored code so we can reproduce them if needed.. technically the diff can be found in git so if it isnt totally necessary.
also need to resolve the conflict from the watchdog prefix vendor that just went in.

@raubitsj raubitsj modified the milestones: sdk-2022-11.1, sdk-2022-12.1 Nov 3, 2022
@dmitryduev
Copy link
Member Author

dmitryduev commented Dec 5, 2022

@raubitsj cleaned this up and addressed your comment (mainly, rm'ed all the non-six-relevant pyupgrade stuff).

@dmitryduev dmitryduev enabled auto-merge (squash) December 5, 2022 23:02
@dmitryduev dmitryduev merged commit 6f71775 into main Dec 5, 2022
@dmitryduev dmitryduev deleted the sixnomore branch December 5, 2022 23:16
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