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

feat(artifacts): add new_draft method to modify and log saved artifacts as new version #5524

Merged
merged 1 commit into from May 12, 2023

Conversation

szymon-piechowicz-wandb
Copy link
Contributor

@szymon-piechowicz-wandb szymon-piechowicz-wandb commented May 10, 2023

Fixes WB-12549

Description

Added the ability to create a new draft artifact bootstrapped with an existing artifact.

Note on API design:
I decided to create a new Artifact instance instead of modifying an existing instance. Changing the identity of an existing instance would be error prone. Note that we keep references to Artifact instances in various places (e.g. here and here) and assume their identity doesn't change over time.

Test plan

Untitled 1 2

@moredatarequired
Copy link
Contributor

moredatarequired commented May 10, 2023

How do you feel about making this a method on the artifact being cloned? I.e. draft_artifact = committed_artifact.new_draft() instead of draft_artifact = wandb.Artifact.from_artifact(committed_artifact)? (new_draft might not be the best name, but something along those lines)

@szymon-piechowicz-wandb
Copy link
Contributor Author

I had it that way at first, and then changed it.

  • The advantage of draft_artifact = wandb.Artifact.from_artifact(committed_artifact) is that it's clear you're creating a new instance.
  • The advantage of draft_artifact = committed_artifact.new_draft() is that it's more concise.

I don't feel very strongly about this though, I can change it back. What do you think?

@moredatarequired
Copy link
Contributor

I don't feel very strongly about this though, I can change it back. What do you think?

Also no strong opinions. I favor the more concise version. Is there a better name than new_draft() that would make it more obvious you're returning a new instance? What had you called it originally?

I'm happy with whatever you decide you want to go with, I just wanted to bring up the question and make it an explicit decision.

@szymon-piechowicz-wandb
Copy link
Contributor Author

I thought about artifact.draft() and artifact.new_draft(). The problem with the former is that it looks like it's modifying the state of the artifact in place. The problem with the latter is that it's similar to artifact.new_file() and it might look like it adds some "draft" to the artifact, similar to how artifact.new_file() adds a new file.

Alright, I'll change to artifact.new_draft().

wandb/apis/public.py Outdated Show resolved Hide resolved
wandb/apis/public.py Show resolved Hide resolved
@szymon-piechowicz-wandb szymon-piechowicz-wandb merged commit c617710 into main May 12, 2023
51 of 52 checks passed
@szymon-piechowicz-wandb szymon-piechowicz-wandb deleted the szymon/from_artifact branch May 12, 2023 15:14
@dmitryduev
Copy link
Member

/fix-title

@github-actions github-actions bot changed the title feat(artifacts): Artifacts: allow saved artifacts to be modified and logged as a new version feat(artifacts): add new_draft method to modify and log saved artifacts as new version Jun 2, 2023
@kptkin kptkin added this to the sdk-2023-06.1 milestone Jun 3, 2023
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

4 participants