-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add upload study artifact api #674
Add upload study artifact api #674
Conversation
Let me read this PR for my study. I will not merge the PR with my approval. |
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.
Thanks for the PR. I have some early comments. PTAL.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
==========================================
+ Coverage 62.88% 63.42% +0.53%
==========================================
Files 35 35
Lines 2250 2280 +30
==========================================
+ Hits 1415 1446 +31
+ Misses 835 834 -1 ☔ View full report in Codecov by Sentry. |
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.
Sorry for being late. I just checked python API part, and left comments. Regarding python API parts, it looks almost good to me other than my comments.
If you divide this PR into python API change and adding new UI, I can merge the PR for python API change immediately.
return {"reason": "Cannot access to the artifacts."} | ||
artifact_store.remove(artifact_id) | ||
|
||
# The artifact's metadata is stored in one of the following two locations: |
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.
[question] As study artifact is introduced in this PR, the metadata is always stored in ARTIFACTS_ATTR_PREFIX + artifact_id
?
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.
Yes, this behavior is the same as optuna/artifacts/_upload.py
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.
In that case, can we remove those lines?
storage.set_study_system_attr(
study_id, _dashboard_artifact_prefix(study_id) + artifact_id, json.dumps(None)
)
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.
It sounds good. I think I can remove that line from delete_study_artifact
. However, I would not remove it from delete_trial_artifact
due to backward compatibility.
It is not relevant to this PR, but should we also need to delete meta data in |
Also, it is not relevant to this PR or optunda-dashboard, but should we prepare API for deleting data and meta together like |
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.
Thanks for the update. I added some additional comments. PTAL
Good catch, that's because you did not pass the |
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!
Thank you for working on it.
Contributor License Agreement
This repository (
optuna-dashboard
) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
None.
What does this implement/fix? Explain your changes.
upload_study_artifact
api.StudyHistory
page.