Unite the argument order of artifact APIs#5533
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5533 +/- ##
==========================================
+ Coverage 89.76% 89.79% +0.03%
==========================================
Files 197 197
Lines 12665 12666 +1
==========================================
+ Hits 11369 11374 +5
+ Misses 1296 1292 -4 ☔ View full report in Codecov by Sentry. |
|
I added the unittest for positional args. Do you think this test covers what you concerned? @HideakiImamura |
|
@c-bata Could you review this PR? |
HideakiImamura
left a comment
There was a problem hiding this comment.
Thanks for the PR. Basically, LGTM. I have a minor comment. PTAL.
| with open(file_path, "w") as f: | ||
| f.write("foo") | ||
|
|
||
| artifact_id = upload_artifact(trial, file_path, artifact_store) # type: ignore |
There was a problem hiding this comment.
It would be great to catch the warning from convert_positional_args here.
There was a problem hiding this comment.
I addressed your comment!
|
@porink0424 Would you mind reviewing this PR instead of me? |
|
@HideakiImamura could you merge this PR? |
HideakiImamura
left a comment
There was a problem hiding this comment.
I have an additional comment. PTAL.
| with pytest.warns(FutureWarning): | ||
| artifact_id = upload_artifact(trial, file_path, artifact_store) # type: ignore | ||
| _validate(artifact_id=artifact_id) | ||
| artifact_id = upload_artifact( | ||
| trial, file_path, artifact_store=artifact_store # type: ignore | ||
| ) | ||
| _validate(artifact_id=artifact_id) | ||
| artifact_id = upload_artifact( | ||
| trial, file_path=file_path, artifact_store=artifact_store # type: ignore | ||
| ) | ||
| _validate(artifact_id=artifact_id) |
There was a problem hiding this comment.
It would be better to catch the FutureWarning in each call of upload_artifact, since _validate does not cause any warnings.
Motivation
This PR is to match the argument order of artifact store APIs.
Related PR:
download_artifactto that ofupload_artifact#5529Description of the changes
After this PR, the APIs will look:
Since
upload_artifactis an existing API, I addedconvert_potional_argsas a kind deprecation reminder for users.