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

Implement download_artifact #5448

Merged
merged 17 commits into from
Jun 12, 2024
Merged

Implement download_artifact #5448

merged 17 commits into from
Jun 12, 2024

Conversation

porink0424
Copy link
Contributor

@porink0424 porink0424 commented May 15, 2024 •
edited

Motivation

Although upload_artifact existed, its counterpart did not exist. This PR introduces a new API download_artifact as the counterpart. In order to have symmetry with upload_artifact, download_artifact has an interface that specifies file_path as an argument.

Description of the changes

  • Added download_artifact
  • Added test for download_artifact

toshihikoyanase
toshihikoyanase previously requested changes May 15, 2024
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for your great proposal. I've actually been waiting for this feature.

Let me share my early comment.

optuna/artifacts/_download.py Outdated Show resolved Hide resolved
tests/artifacts_tests/test_download_artifact.py Outdated Show resolved Hide resolved
@porink0424
Copy link
Contributor Author

porink0424 commented May 17, 2024 •
edited

@toshihikoyanase
Thank you for your comments! I have fixed the points.

toshihikoyanase
toshihikoyanase reviewed May 17, 2024
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thanks for your update!
I added two suggestions and a note.

optuna/artifacts/_download.py Outdated Show resolved Hide resolved
optuna/artifacts/_download.py Outdated Show resolved Hide resolved
tests/artifacts_tests/test_download_artifact.py Show resolved Hide resolved
@nabenabe0928
Copy link
Collaborator

Could you add an explanation of download_artifact to tutorial/20_recipes/012_artifact_tutorial.py?

nabenabe0928
nabenabe0928 requested changes May 21, 2024
View reviewed changes
Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, I left some comments!
Could you please take another look?

optuna/artifacts/_download.py Outdated Show resolved Hide resolved
optuna/artifacts/_download.py Outdated Show resolved Hide resolved
optuna/artifacts/_download.py Outdated Show resolved Hide resolved
optuna/artifacts/_download.py Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented May 24, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.77%. Comparing base (181d65f) to head (6b27239).
Report is 183 commits behind head on master.

❗ Current head 6b27239 differs from pull request most recent head c286409

Please upload reports for the commit c286409 to get more accurate results.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5448      +/-   ##
==========================================
+ Coverage   89.52%   89.77%   +0.24%     
==========================================
  Files         194      196       +2     
  Lines       12626    12582      -44     
==========================================
- Hits        11303    11295       -8     
+ Misses       1323     1287      -36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

porink0424 and others added 4 commits May 29, 2024 16:29
not522
not522 approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM!

@not522 not522 removed their assignment Jun 7, 2024
@c-bata c-bata added the feature Change that does not break compatibility, but affects the public interfaces. label Jun 7, 2024
toshihikoyanase
toshihikoyanase reviewed Jun 7, 2024
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed response. My comments have been resolved.
@nabenabe0928 @c-bata Please merge after your review.

nabenabe0928
nabenabe0928 reviewed Jun 8, 2024
View reviewed changes
Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Thank you for the update!
I left some minor comments!

tutorial/20_recipes/012_artifact_tutorial.py Show resolved Hide resolved
tutorial/20_recipes/012_artifact_tutorial.py Show resolved Hide resolved
tutorial/20_recipes/012_artifact_tutorial.py Outdated Show resolved Hide resolved
tutorial/20_recipes/012_artifact_tutorial.py Outdated Show resolved Hide resolved
@c-bata c-bata dismissed toshihikoyanase’s stale review June 10, 2024 06:41

Since his comment was addressed

nabenabe0928
nabenabe0928 approved these changes Jun 12, 2024
View reviewed changes
Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for the update, LGTM!

@nabenabe0928 nabenabe0928 merged commit 95b65d2 into optuna:master Jun 12, 2024
13 of 14 checks passed
@porink0424 porink0424 deleted the feat/download_artifact branch June 12, 2024 02:21
@nabenabe0928 nabenabe0928 added this to the v4.0.0 milestone Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not522 not522 not522 approved these changes

@toshihikoyanase toshihikoyanase toshihikoyanase left review comments

@c-bata c-bata c-bata left review comments

@nabenabe0928 nabenabe0928 nabenabe0928 approved these changes

Assignees

@nabenabe0928 nabenabe0928

Labels
feature Change that does not break compatibility, but affects the public interfaces.
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@porink0424 @nabenabe0928 @not522 @toshihikoyanase @c-bata