-
Notifications
You must be signed in to change notification settings - Fork 621
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(launch): overwrite partial job metadata instead of building a new job version #7651
fix(launch): overwrite partial job metadata instead of building a new job version #7651
Conversation
b83616d
to
1cd559c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7651 +/- ##
==========================================
+ Coverage 74.26% 75.78% +1.51%
==========================================
Files 500 500
Lines 55803 54133 -1670
==========================================
- Hits 41444 41026 -418
+ Misses 13946 12691 -1255
- Partials 413 416 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1cd559c
to
f42e9fa
Compare
e391f54
to
1c377af
Compare
f42e9fa
to
4e34d5d
Compare
ffff3c5
to
7ec97ff
Compare
7ec97ff
to
e10eea2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a (system) test for the added logic?
56fca2a
to
0a93a41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall logic seems correct to me. However I'd prefer to refactor some of the inherenet job builder logic into job builder methods to keep sender cleaner
b4b22c4
to
5dd1db7
Compare
b8be30a
to
2e5f554
Compare
2e5f554
to
1ba01d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, but mostly LGTM.
1ba01d9
to
6df8fc6
Compare
6df8fc6
to
30e0cbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but codecov is saying there are no tests covering many of the new methods added. Should be pretty straight forward to get tests in tests/pytest_tests/unit_tests/test_job_builder.py
30e0cbb
to
e333d42
Compare
cfb33a6
to
9cd99e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple nits
9cd99e1
to
d99e51a
Compare
…#7676) Co-authored-by: Dmitry Duev <dmitryduev@users.noreply.github.com>
d99e51a
to
0f5ac92
Compare
Description
This PR modifies the core and legacy job builder to never create a new job version when a partial job is used.
Partial jobs are created by the
wandb job create
orwandb launch -u
commands. They include source information but no input or output type info. When a run created from a partial job finishes, the job builder constructs a new, non-partial job version with the input and output info saved into thewandb-job.json
file.Jobs now support having i/o types stored in artifact version metadata, which can be mutated without producing a new job version. Partial jobs now write
_partial
key to the metadata that can be used to identify partial jobs. If a partial job is detected the job builder will overwrite the artifact metadata with the i/o types.Testing
How was this PR tested?