Make positional args to kwargs in suggest_int by nabenabe0928 · Pull Request #5044 · optuna/optuna

nabenabe0928 · 2023-10-16T06:24:21Z

Motivation

As suggest_float has * before steps and log so that users need to pass them as kwargs, I applied the same to suggest_int.

Description of the changes

I made the following changes:

Change suggest_int(name, low, high, step, log) into suggest_int(name, low, high, *, step, log) to strengthen the future safety, and
Wrap suggest_int with convert_positional_args for now to avoid breaking changes.

nabenabe0928 · 2023-10-16T07:00:25Z

I have a question.
This PR does not improve the safety of the software by itself, but is it still fine?

…ials

codecov · 2023-10-16T08:00:32Z

Codecov Report

Merging #5044 (6a44734) into master (3fcbf88) will increase coverage by 0.01%.
Report is 78 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5044      +/-   ##
==========================================
+ Coverage   89.40%   89.42%   +0.01%     
==========================================
  Files         203      205       +2     
  Lines       15055    15135      +80     
==========================================
+ Hits        13460    13534      +74     
- Misses       1595     1601       +6

Files	Coverage Δ
optuna/_convert_positional_args.py	`92.50% <100.00%> (+2.84%)`	⬆️
optuna/multi_objective/trial.py	`87.42% <100.00%> (+0.23%)`	⬆️
optuna/trial/_base.py	`76.34% <100.00%> (+0.25%)`	⬆️
optuna/trial/_fixed.py	`90.00% <100.00%> (+0.30%)`	⬆️
optuna/trial/_frozen.py	`94.90% <100.00%> (+0.07%)`	⬆️
optuna/trial/_trial.py	`95.74% <100.00%> (+0.06%)`	⬆️

... and 17 files with indirect coverage changes

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

contramundum53 · 2023-10-16T09:00:43Z

@not522 @HideakiImamura Could you review this PR?

HideakiImamura

Thanks for the PR. Basically, LGTM. I have two minor comments. PTAL.

By the way, as you mentioned, this PR does not force anything on the users as it is a change aimed at continuing support for the older API. However, this change is a milestone for the future. It is expected that the older API will no longer be supported in a future major version, and by making these changes in the code, it becomes easier for us to keep motivation.

HideakiImamura · 2023-10-18T01:51:50Z

tests/multi_objective_tests/test_trial.py

The multi_objective module is deprecated, so you should not have to add tests on this module.

Just to make sure, but did you mean "do not have to" or "discouraging" (In other words, should I remove it)?

I mean I discourage to fix the deprecated module.

HideakiImamura · 2023-10-18T01:53:03Z

optuna/multi_objective/trial.py

The multi_objective module is deprecated, so you should not have to fix this module.

not522 · 2023-10-19T07:30:39Z

optuna/trial/_base.py

 from optuna.distributions import CategoricalChoiceType


+_SUGGEST_INT_POSITIONAL_ARGS = ["self", "name", "low", "high", "step", "log"]


Could you make only step and log positional? Currently convert_positional_args does not support partial conversion, but you can use inspect.signature to achieve this.

Hi sorry for the late response:(
The variable name was actually misleading because these are "previous" positional arguments, but not the current positional arguments.
Plus, since all the arguments in suggest_int were positional arguments before this PR, I would like to simply change the variable name and let the convert_positional_args stay as it does.
If necessary, I can also make another PR for this.
How do you like it? (@not522)

Oh, I'm sorry, it's my mistake. I wanted to say "make only step and log keyword-only".

Certainly!
Then I will change the code so that inappropriate args usages will be warned!

HideakiImamura · 2023-10-25T08:10:47Z

@nabenabe0928 The checks CI fails. Could you fix them?

nabenabe0928 · 2023-10-25T09:07:32Z

@nabenabe0928 The checks CI fails. Could you fix them?

Done!

not522

I found unnecessary warnings, for example, suggest_int("x", 0, 9) outputs FutureWarning. I think that the check for the length in converter_wrapper is not correct.

nabenabe0928 · 2023-10-27T06:51:02Z

I found unnecessary warnings, for example, suggest_int("x", 0, 9) outputs FutureWarning. I think that the check for the length in converter_wrapper is not correct.

Now I changed so that the FutureWarning happens only if keyword-only is specified as arguments.
Also the new implementation warns only once rather than raising warnings for each argument.

not522 · 2023-10-31T01:44:09Z

optuna/_convert_positional_args.py

-                    f"{func.__name__}(): Please give all values as keyword arguments."
+                    f"{func.__name__}() got {kwargs_expected} as positional arguments "
+                    "but they were expected to be given as keyword arguments."
                    " See https://github.com/optuna/optuna/issues/3324 for details.",


It's almost LGTM. How about removing this line because this error is not only related to #3324?

Right, because of the last change, #3324 does not relate to this line anymore. (while it still makes sense to use kwargs only, the future warning for it is not the default Python design, so I removed it and stick to your suggestion.)

not522

LGTM!

HideakiImamura

LGTM.

[code-fix] Make positional args to kwargs in suggest_int

418a90c

github-actions bot added the optuna.trial Related to the `optuna.trial` submodule. This is automatically labeled by github-actions. label Oct 16, 2023

nabenabe0928 added 2 commits October 16, 2023 08:37

[fix] Resolve mypy errors

dbe38d6

[test] Add a test for the old-style suggest_int

0f15a0f

nabenabe0928 force-pushed the code-fix/change-args-to-kwargs-in-suggest-int branch from 2dc92a4 to 25d7a0a Compare October 16, 2023 06:58

[test] Add tests for the old-style suggest_int of fixed and frozen tr…

29b63da

…ials

nabenabe0928 force-pushed the code-fix/change-args-to-kwargs-in-suggest-int branch from 25d7a0a to 29b63da Compare October 16, 2023 07:04

[test] Add a test for the old-style suggest_int of MO trial

18c4b3a

contramundum53 assigned not522 and HideakiImamura Oct 16, 2023

HideakiImamura reviewed Oct 18, 2023

View reviewed changes

not522 reviewed Oct 19, 2023

View reviewed changes

Add the future warning for inappropriate positional args usages

92a4c6a

nabenabe0928 requested review from HideakiImamura and not522 October 25, 2023 06:35

Apply black formatting

f02c7fe

not522 reviewed Oct 27, 2023

View reviewed changes

Remove unnecessary warnings based on the comment by not522

070cc61

nabenabe0928 force-pushed the code-fix/change-args-to-kwargs-in-suggest-int branch from 56fe8a9 to 070cc61 Compare October 27, 2023 06:15

Add a test using a simple method

7009e0f

not522 reviewed Oct 31, 2023

View reviewed changes

nabenabe0928 added 2 commits October 31, 2023 06:10

Remove ref to 3324 as it is not related anymore

e763c2c

Change variable names for readability

6a44734

not522 approved these changes Nov 1, 2023

View reviewed changes

not522 removed their assignment Nov 1, 2023

not522 added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Nov 1, 2023

HideakiImamura approved these changes Nov 2, 2023

View reviewed changes

HideakiImamura merged commit 4694831 into optuna:master Nov 2, 2023

HideakiImamura added this to the v3.5.0 milestone Nov 2, 2023

nabenabe0928 deleted the code-fix/change-args-to-kwargs-in-suggest-int branch November 8, 2023 06:59

shmurai mentioned this pull request May 31, 2025

add deprecated/removed version specification to calls of convert_positional_args #6117

Merged

		from optuna.distributions import CategoricalChoiceType


		_SUGGEST_INT_POSITIONAL_ARGS = ["self", "name", "low", "high", "step", "log"]

Uh oh!

Conversation

nabenabe0928 commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description of the changes

Uh oh!

nabenabe0928 commented Oct 16, 2023

Uh oh!

codecov bot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

contramundum53 commented Oct 16, 2023

Uh oh!

HideakiImamura left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabenabe0928 Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HideakiImamura commented Oct 25, 2023

Uh oh!

nabenabe0928 commented Oct 25, 2023

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

nabenabe0928 commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

HideakiImamura left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nabenabe0928 commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

nabenabe0928 Oct 24, 2023 •

edited

Loading