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

Add optuna study-names cli #5029

Merged
merged 4 commits into from Nov 6, 2023
Merged

Add optuna study-names cli #5029

merged 4 commits into from Nov 6, 2023

Conversation

Alnusjaponica
Copy link
Collaborator

Motivation

Follow up #4898 to add cli of get_all_study_names()

Description of the changes

Add optuna study-names with --format option

@Alnusjaponica Alnusjaponica marked this pull request as ready for review October 12, 2023 08:12
@codecov
Copy link

codecov bot commented Oct 12, 2023 •
edited

Codecov Report

Merging #5029 (e152d69) into master (3895ff8) will decrease coverage by 0.10%.
Report is 92 commits behind head on master.
The diff coverage is 16.00%.

@@            Coverage Diff             @@
##           master    #5029      +/-   ##
==========================================
- Coverage   89.38%   89.28%   -0.10%     
==========================================
  Files         203      205       +2     
  Lines       15055    15135      +80     
==========================================
+ Hits        13457    13514      +57     
- Misses       1598     1621      +23
Files Coverage Δ
optuna/cli.py 22.90% <16.00%> (-0.30%) ⬇️

... and 19 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

c-bata
c-bata reviewed Oct 13, 2023
View reviewed changes
optuna/cli.py Outdated Show resolved Hide resolved
@c-bata c-bata self-assigned this Oct 13, 2023
@c-bata c-bata added the feature Change that does not break compatibility, but affects the public interfaces. label Oct 13, 2023
@c-bata
Copy link
Member

c-bata commented Oct 13, 2023

@nabenabe0928 Could you review this PR?

@nabenabe0928
Copy link
Collaborator

@nabenabe0928 Could you review this PR?

yes

@nabenabe0928
Copy link
Collaborator

looks good to me:)
I also checked the behavior on my Linux machine and it was perfect!

@not522 not522 changed the title Add optuna study-name cli Add optuna study-names cli Oct 13, 2023
nabenabe0928
nabenabe0928 approved these changes Oct 13, 2023
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.

LGTM

@nabenabe0928 nabenabe0928 removed their assignment Oct 13, 2023
@github-actions
Copy link
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Oct 22, 2023
@contramundum53
Copy link
Member

@c-bata ping

c-bata
c-bata previously requested changes Oct 25, 2023
View reviewed changes
Copy link
Member

@c-bata c-bata 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 delay. I left a comment.

optuna/cli.py Outdated
"--format",
type=str,
choices=("json", "table", "yaml"),
default="table",
Copy link
Member

Choose a reason for hiding this comment

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

We discussed the default format and decided to output study names per line like below. Could you update the PR?

$ optuna study-names --storage ...
study_1
study_2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated so that study-names output is default to value format. PTAL

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Oct 25, 2023
@HideakiImamura HideakiImamura assigned not522 and unassigned c-bata Oct 26, 2023
@HideakiImamura
Copy link
Member

@not522 Could you review this PR?

not522
not522 reviewed Oct 27, 2023
View reviewed changes
tests/test_cli.py Outdated Show resolved Hide resolved
not522
not522 approved these changes Nov 6, 2023
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!
Let me merge it since @c-bata's concern has been resolved.

@not522 not522 dismissed c-bata’s stale review November 6, 2023 06:59

Your concern has been resolved. Since you are busy, I just dismiss your review instead of asking you to re-review it.

@not522 not522 merged commit e5835b8 into optuna:master Nov 6, 2023
32 checks passed
@not522 not522 added this to the v3.5.0 milestone Nov 6, 2023
@Alnusjaponica Alnusjaponica deleted the implement-study-name-cli branch November 6, 2023 08:09
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

@nabenabe0928 nabenabe0928 nabenabe0928 approved these changes

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

Assignees

@not522 not522

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@Alnusjaponica @c-bata @nabenabe0928 @contramundum53 @HideakiImamura @not522