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.

Sign up for GitHub

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

Jump to bottom

feat(nexus): finish artifact saver #6296

Merged
merged 13 commits into from
Oct 4, 2023
Merged

feat(nexus): finish artifact saver #6296

merged 13 commits into from
Oct 4, 2023

Conversation

szymon-piechowicz-wandb
Copy link
Contributor

@szymon-piechowicz-wandb szymon-piechowicz-wandb commented Sep 15, 2023 •
edited

Fixes WB-15188

Description

Nexus docs: https://weightsandbiases.slack.com/archives/C04MNBGJDBN/p1692678447664909
Upload algorithm docs: https://weightsandbiases.slack.com/archives/C04MNBGJDBN/p1693352398534739
Benchmark: https://weightsandbiases.slack.com/archives/C04MNBGJDBN/p1695764535202659

TODO:

  • delete staged files
  • write to cache
  • S3 multipart upload

Test plan

  • Created an artifact, made sure the manifest and files were uploaded correctly:
import wandb

wandb.require("nexus")

artifact = wandb.Artifact("my_artifact", type="tmp")
with artifact.new_file("a.txt") as f:
    f.write("a")
artifact.save()
artifact.wait()

@szymon-piechowicz-wandb szymon-piechowicz-wandb force-pushed the szymon/nexus_upload branch 3 times, most recently from 9773c04 to 50026b5 Compare September 15, 2023 06:41
@codecov
Copy link

codecov bot commented Sep 15, 2023 •
edited

Codecov Report

Merging #6296 (11d53b5) into main (6b9767b) will decrease coverage by 6.41%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6296      +/-   ##
==========================================
- Coverage   77.10%   70.70%   -6.41%     
==========================================
  Files         387      387              
  Lines       44607    44598       -9     
==========================================
- Hits        34393    31531    -2862     
- Misses      10162    13015    +2853     
  Partials       52       52
Flag Coverage Δ
unittest 74.10% <0.00%> (-6.84%) ⬇️

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

Files Coverage Δ
wandb/apis/public.py 67.17% <ø> (-11.93%) ⬇️
wandb/sdk/wandb_init.py 81.02% <0.00%> (-6.01%) ⬇️
nexus/pkg/server/handler.go 0.00% <0.00%> (ø)
nexus/pkg/server/sender.go 16.57% <0.00%> (+0.14%) ⬆️

... and 68 files with indirect coverage changes

Base automatically changed from szymon/nexus_split_file to main September 15, 2023 18:26
@github-actions github-actions bot added cc-feat and removed cc-feat labels Sep 15, 2023
@szymon-piechowicz-wandb szymon-piechowicz-wandb force-pushed the szymon/nexus_upload branch 3 times, most recently from 351060b to 796c8a0 Compare September 26, 2023 06:21
@szymon-piechowicz-wandb szymon-piechowicz-wandb marked this pull request as ready for review September 26, 2023 06:22
@szymon-piechowicz-wandb szymon-piechowicz-wandb force-pushed the szymon/nexus_upload branch 4 times, most recently from 0658cf7 to 100799f Compare September 26, 2023 22:06
@ibindlish ibindlish mentioned this pull request Sep 27, 2023
Draft
2 tasks
@github-actions github-actions bot added cc-feat and removed cc-feat labels Oct 2, 2023
@szymon-piechowicz-wandb szymon-piechowicz-wandb force-pushed the szymon/nexus_upload branch 2 times, most recently from ab50c8b to 6003a11 Compare October 2, 2023 18:39
@szymon-piechowicz-wandb szymon-piechowicz-wandb mentioned this pull request Oct 2, 2023
Merged
Base automatically changed from szymon/nexus_nil_if_zero to main October 4, 2023 19:27
@github-actions github-actions bot added cc-feat and removed cc-feat labels Oct 4, 2023
@dmitryduev dmitryduev changed the title feat(artifacts): finish artifact saver in Nexus feat(nexus): finish artifact saver Oct 4, 2023
@github-actions github-actions bot added cc-feat and removed cc-feat labels Oct 4, 2023
raubitsj
raubitsj reviewed Oct 4, 2023
View reviewed changes
nexus/pkg/artifacts/saver.go
taskResultsChan <- TaskResult{task, name}
},
}
as.UploadManager.AddTask(task)
Copy link
Member

Choose a reason for hiding this comment

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

FYI: i think we still need to add something (in a later PR) to capture uploaded files so we can pass them in filestream as completed files.. See "Uploaded" in pkg/filestream/loop_transmit.go -- and equiv logic in python internal/filestream.go

@dmitryduev dmitryduev merged commit 58544e1 into main Oct 4, 2023
67 of 69 checks passed
@dmitryduev dmitryduev deleted the szymon/nexus_upload branch October 4, 2023 23:47
@szymon-piechowicz-wandb
Copy link
Contributor Author

What happened here? @dmitryduev @kptkin why did you add a bunch of unrelated changes to my PR?

@dmitryduev dmitryduev restored the szymon/nexus_upload branch October 6, 2023 17:38
kptkin
kptkin reviewed Oct 6, 2023
View reviewed changes
nexus/pkg/server/handler.go
@@ -291,7 +291,8 @@ func (h *Handler) handleRequest(record *service.Record) {
shutdown = true
case *service.Request_StopStatus:
case *service.Request_LogArtifact:
h.handleLogArtifact(record, x.LogArtifact, response)
h.handleLogArtifact(record)
response = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return early from this case

nexus/pkg/utils/utils.go Outdated
func NilIfZero[T comparable](x T) *T {
var zero T
if x == zero {
return nil
}
return &x
}

func ComputeB64MD5(data []byte) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere other than artifacts?

nexus/pkg/server/sender.go Show resolved Hide resolved
tests/pytest_tests/system_tests/test_core/test_keras_full.py Show resolved Hide resolved
tests/pytest_tests/system_tests/test_core/test_public_api.py Show resolved Hide resolved
wandb/apis/public.py Show resolved Hide resolved
wandb/sdk/wandb_run.py Show resolved Hide resolved
@szymon-piechowicz-wandb szymon-piechowicz-wandb deleted the szymon/nexus_upload branch October 6, 2023 18:59
@github-actions github-actions bot added cc-feat and removed cc-feat labels Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raubitsj raubitsj raubitsj approved these changes

@kptkin kptkin kptkin left review comments

@moredatarequired moredatarequired Awaiting requested review from moredatarequired

@estellazx estellazx Awaiting requested review from estellazx

@ibindlish ibindlish Awaiting requested review from ibindlish

Assignees
No one assigned
Labels
cc-feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@szymon-piechowicz-wandb @raubitsj @kptkin @dmitryduev