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(public-api): change artifactSequence to artifactCollection in public GQL requests #4531

Merged
merged 11 commits into from Nov 30, 2022

Conversation

tssweeney
Copy link
Contributor

@tssweeney tssweeney commented Nov 23, 2022

https://wandb.atlassian.net/browse/WB-11598

This is some old tech debt from model reg work. ArtifactCollections should always use the *Collection style not *Sequence.

@tssweeney tssweeney requested a review from vwrj November 23, 2022 21:27
@tssweeney tssweeney changed the title fix(public): Changed artifactSequence to artifactCollection in public GQL requests fix(public-api): Changed artifactSequence to artifactCollection in public GQL requests Nov 23, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #4531 (6e3372d) into main (948e0c1) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4531      +/-   ##
==========================================
- Coverage   83.12%   83.11%   -0.01%     
==========================================
  Files         259      259              
  Lines       32984    32984              
==========================================
- Hits        27418    27416       -2     
- Misses       5566     5568       +2     
Flag Coverage Δ
functest 56.95% <0.00%> (+0.02%) ⬆️
unittest 73.27% <75.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
wandb/apis/public.py 82.13% <75.00%> (ø)
wandb/integration/tensorboard/monkeypatch.py 91.35% <0.00%> (-2.47%) ⬇️
wandb/sdk/internal/internal_api.py 89.17% <0.00%> (-0.52%) ⬇️
wandb/sdk/wandb_setup.py 88.44% <0.00%> (-0.51%) ⬇️
wandb/sdk/lib/git.py 79.01% <0.00%> (ø)
wandb/sdk/internal/system/system_info.py 91.55% <0.00%> (+3.24%) ⬆️

@github-actions github-actions bot added cc-fix and removed cc-fix labels Nov 23, 2022
Copy link
Contributor

@vwrj vwrj left a comment

Choose a reason for hiding this comment

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

Looks great. Can we add a quick test to make sure that this successfully pulls from a portfolio?

@tssweeney tssweeney requested a review from vwrj November 29, 2022 03:16
Copy link
Contributor

@vwrj vwrj left a comment

Choose a reason for hiding this comment

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

Nice.

@vwrj
Copy link
Contributor

vwrj commented Nov 29, 2022

When this is merged, let's respond to this Github issue here: #4519. We should

  • test that the user's code snippet (pulling api.artifact_versions works as a sanity check)
  • tell them this will be available in the next SDK release
  • create a branch off the currently released version + cherry pick this change on top so that they can be unblocked for their current work until the next release.

There's 2 users who have responded that are using the API to pull artifact versions for the model registry so there must be more out there who have experienced this issue. So I'd like to unblock them as soon as possible.

@tssweeney
Copy link
Contributor Author

When this is merged, let's respond to this Github issue here: #4519. We should

  • test that the user's code snippet (pulling api.artifact_versions works as a sanity check)
  • tell them this will be available in the next SDK release
  • create a branch off the currently released version + cherry pick this change on top so that they can be unblocked for their current work until the next release.

There's 2 users who have responded that are using the API to pull artifact versions for the model registry so there must be more out there who have experienced this issue. So I'd like to unblock them as soon as possible.

Great - i tested in notebook and it works:
Screen Shot 2022-11-29 at 8 08 54 AM
Screen Shot 2022-11-29 at 8 11 48 AM

Note: the Artifact::name field refers to the "home" artifact sequence, not the collection in question. To fix this we need to add a new GQL edge that lets us go from artifact version to the membership for a collection.

@tssweeney
Copy link
Contributor Author

Created a followup PR to fix the artifact version name issue: #4547

@tssweeney tssweeney merged commit 8535615 into main Nov 30, 2022
@tssweeney tssweeney deleted the model_reg/artifact_collection_support_in_public_api branch November 30, 2022 04:18
@kptkin kptkin changed the title fix(public-api): Changed artifactSequence to artifactCollection in public GQL requests fix(public-api): change artifactSequence to artifactCollection in public GQL requests Dec 6, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Dec 6, 2022
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

2 participants