diff --git a/optuna/_convert_positional_args.py b/optuna/_convert_positional_args.py index caf9d80bcfd..d496a288526 100644 --- a/optuna/_convert_positional_args.py +++ b/optuna/_convert_positional_args.py @@ -3,6 +3,7 @@ from collections.abc import Callable from collections.abc import Sequence from functools import wraps +from inspect import Parameter from inspect import signature from typing import Any from typing import TYPE_CHECKING @@ -17,6 +18,21 @@ _T = TypeVar("_T") +def _get_positional_arg_names(func: "Callable[_P, _T]") -> list[str]: + params = signature(func).parameters + positional_arg_names = [ + name + for name, p in params.items() + if p.default == Parameter.empty and p.kind == p.POSITIONAL_OR_KEYWORD + ] + return positional_arg_names + + +def _infer_kwargs(previous_positional_arg_names: Sequence[str], *args: Any) -> dict[str, Any]: + inferred_kwargs = {arg_name: val for val, arg_name in zip(args, previous_positional_arg_names)} + return inferred_kwargs + + def convert_positional_args( *, previous_positional_arg_names: Sequence[str], @@ -37,10 +53,13 @@ def converter_decorator(func: "Callable[_P, _T]") -> "Callable[_P, _T]": @wraps(func) def converter_wrapper(*args: Any, **kwargs: Any) -> "_T": - if len(args) >= 1: + positional_arg_names = _get_positional_arg_names(func) + inferred_kwargs = _infer_kwargs(previous_positional_arg_names, *args) + if len(inferred_kwargs) > len(positional_arg_names): + expected_kwds = set(inferred_kwargs) - set(positional_arg_names) warnings.warn( - f"{func.__name__}(): Please give all values as keyword arguments." - " See https://github.com/optuna/optuna/issues/3324 for details.", + f"{func.__name__}() got {expected_kwds} as positional arguments " + "but they were expected to be given as keyword arguments.", FutureWarning, stacklevel=warning_stacklevel, ) @@ -50,15 +69,16 @@ def converter_wrapper(*args: Any, **kwargs: Any) -> "_T": f" arguments but {len(args)} were given." ) - for val, arg_name in zip(args, previous_positional_arg_names): - # When specifying a positional argument that is not located at the end of args as - # a keyword argument, raise TypeError as follows by imitating the Python standard - # behavior. - if arg_name in kwargs: - raise TypeError( - f"{func.__name__}() got multiple values for argument '{arg_name}'." - ) - kwargs[arg_name] = val + duplicated_kwds = set(kwargs).intersection(inferred_kwargs) + if len(duplicated_kwds): + # When specifying positional arguments that are not located at the end of args as + # keyword arguments, raise TypeError as follows by imitating the Python standard + # behavior + raise TypeError( + f"{func.__name__}() got multiple values for arguments {duplicated_kwds}." + ) + + kwargs.update(inferred_kwargs) return func(**kwargs) diff --git a/optuna/multi_objective/trial.py b/optuna/multi_objective/trial.py index 3fc37cead61..928eab56056 100644 --- a/optuna/multi_objective/trial.py +++ b/optuna/multi_objective/trial.py @@ -8,12 +8,14 @@ from typing import Union from optuna import multi_objective +from optuna._convert_positional_args import convert_positional_args from optuna._deprecated import deprecated_class from optuna.distributions import BaseDistribution from optuna.study._study_direction import StudyDirection from optuna.trial import FrozenTrial from optuna.trial import Trial from optuna.trial import TrialState +from optuna.trial._base import _SUGGEST_INT_POSITIONAL_ARGS CategoricalChoiceType = Union[None, bool, int, float, str] @@ -87,7 +89,10 @@ def suggest_discrete_uniform(self, name: str, low: float, high: float, q: float) return self._trial.suggest_discrete_uniform(name, low, high, q) - def suggest_int(self, name: str, low: int, high: int, step: int = 1, log: bool = False) -> int: + @convert_positional_args(previous_positional_arg_names=_SUGGEST_INT_POSITIONAL_ARGS) + def suggest_int( + self, name: str, low: int, high: int, *, step: int = 1, log: bool = False + ) -> int: """Suggest a value for the integer parameter. Please refer to the documentation of :func:`optuna.trial.Trial.suggest_int` diff --git a/optuna/trial/_base.py b/optuna/trial/_base.py index 84fb22e6ccb..96e2dec8e6a 100644 --- a/optuna/trial/_base.py +++ b/optuna/trial/_base.py @@ -11,6 +11,9 @@ from optuna.distributions import CategoricalChoiceType +_SUGGEST_INT_POSITIONAL_ARGS = ["self", "name", "low", "high", "step", "log"] + + class BaseTrial(abc.ABC): """Base class for trials. @@ -45,7 +48,9 @@ def suggest_discrete_uniform(self, name: str, low: float, high: float, q: float) raise NotImplementedError @abc.abstractmethod - def suggest_int(self, name: str, low: int, high: int, step: int = 1, log: bool = False) -> int: + def suggest_int( + self, name: str, low: int, high: int, *, step: int = 1, log: bool = False + ) -> int: raise NotImplementedError @overload diff --git a/optuna/trial/_fixed.py b/optuna/trial/_fixed.py index aae0539934b..5db23b0d5ea 100644 --- a/optuna/trial/_fixed.py +++ b/optuna/trial/_fixed.py @@ -7,12 +7,14 @@ import warnings from optuna import distributions +from optuna._convert_positional_args import convert_positional_args from optuna._deprecated import deprecated_func from optuna.distributions import BaseDistribution from optuna.distributions import CategoricalChoiceType from optuna.distributions import CategoricalDistribution from optuna.distributions import FloatDistribution from optuna.distributions import IntDistribution +from optuna.trial._base import _SUGGEST_INT_POSITIONAL_ARGS from optuna.trial._base import BaseTrial @@ -89,7 +91,10 @@ def suggest_loguniform(self, name: str, low: float, high: float) -> float: def suggest_discrete_uniform(self, name: str, low: float, high: float, q: float) -> float: return self.suggest_float(name, low, high, step=q) - def suggest_int(self, name: str, low: int, high: int, step: int = 1, log: bool = False) -> int: + @convert_positional_args(previous_positional_arg_names=_SUGGEST_INT_POSITIONAL_ARGS) + def suggest_int( + self, name: str, low: int, high: int, *, step: int = 1, log: bool = False + ) -> int: return int(self._suggest(name, IntDistribution(low, high, log=log, step=step))) @overload diff --git a/optuna/trial/_frozen.py b/optuna/trial/_frozen.py index 5b946e73538..309b91accfe 100644 --- a/optuna/trial/_frozen.py +++ b/optuna/trial/_frozen.py @@ -11,6 +11,7 @@ from optuna import distributions from optuna import logging +from optuna._convert_positional_args import convert_positional_args from optuna._deprecated import deprecated_func from optuna._typing import JSONSerializable from optuna.distributions import _convert_old_distribution_to_new_distribution @@ -19,6 +20,7 @@ from optuna.distributions import CategoricalDistribution from optuna.distributions import FloatDistribution from optuna.distributions import IntDistribution +from optuna.trial._base import _SUGGEST_INT_POSITIONAL_ARGS from optuna.trial._base import BaseTrial from optuna.trial._state import TrialState @@ -225,7 +227,10 @@ def suggest_loguniform(self, name: str, low: float, high: float) -> float: def suggest_discrete_uniform(self, name: str, low: float, high: float, q: float) -> float: return self.suggest_float(name, low, high, step=q) - def suggest_int(self, name: str, low: int, high: int, step: int = 1, log: bool = False) -> int: + @convert_positional_args(previous_positional_arg_names=_SUGGEST_INT_POSITIONAL_ARGS) + def suggest_int( + self, name: str, low: int, high: int, *, step: int = 1, log: bool = False + ) -> int: return int(self._suggest(name, IntDistribution(low, high, log=log, step=step))) @overload diff --git a/optuna/trial/_trial.py b/optuna/trial/_trial.py index 8d2bdacb9e6..fad8e8f267a 100644 --- a/optuna/trial/_trial.py +++ b/optuna/trial/_trial.py @@ -14,6 +14,7 @@ from optuna import distributions from optuna import logging from optuna import pruners +from optuna._convert_positional_args import convert_positional_args from optuna._deprecated import deprecated_func from optuna.distributions import BaseDistribution from optuna.distributions import CategoricalChoiceType @@ -21,6 +22,7 @@ from optuna.distributions import FloatDistribution from optuna.distributions import IntDistribution from optuna.trial import FrozenTrial +from optuna.trial._base import _SUGGEST_INT_POSITIONAL_ARGS from optuna.trial._base import BaseTrial @@ -235,7 +237,10 @@ def suggest_discrete_uniform(self, name: str, low: float, high: float, q: float) return self.suggest_float(name, low, high, step=q) - def suggest_int(self, name: str, low: int, high: int, step: int = 1, log: bool = False) -> int: + @convert_positional_args(previous_positional_arg_names=_SUGGEST_INT_POSITIONAL_ARGS) + def suggest_int( + self, name: str, low: int, high: int, *, step: int = 1, log: bool = False + ) -> int: """Suggest a value for the integer parameter. The value is sampled from the integers in :math:`[\\mathsf{low}, \\mathsf{high}]`. diff --git a/tests/multi_objective_tests/test_trial.py b/tests/multi_objective_tests/test_trial.py index f5ed15bddb0..17d2f90135e 100644 --- a/tests/multi_objective_tests/test_trial.py +++ b/tests/multi_objective_tests/test_trial.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import datetime from typing import List from typing import Tuple @@ -205,3 +207,13 @@ def create_trial( else: # If `t1` isn't COMPLETE, it doesn't dominate others. assert not t1._dominates(t0, directions) + + +@pytest.mark.parametrize("positional_args_names", [[], ["step"], ["step", "log"]]) +def test_suggest_int_positional_args(positional_args_names: list[str]) -> None: + # If log is specified as positional, step must also be provided as positional. + study = optuna.multi_objective.create_study(["maximize"]) + kwargs = dict(step=1, log=False) + args = [kwargs[name] for name in positional_args_names] + # No error should not be raised even if the coding style is old. + study.optimize(lambda trial: [trial.suggest_int("x", -1, 1, *args)], n_trials=1) diff --git a/tests/test_convert_positional_args.py b/tests/test_convert_positional_args.py index 3a104f10d63..442cce22b0c 100644 --- a/tests/test_convert_positional_args.py +++ b/tests/test_convert_positional_args.py @@ -10,6 +10,12 @@ def _sample_func(*, a: int, b: int, c: int) -> int: return a + b + c +class _SimpleClass: + @convert_positional_args(previous_positional_arg_names=["self", "a", "b"]) + def simple_method(self, a: int, *, b: int, c: int = 1) -> None: + pass + + def test_convert_positional_args_decorator() -> None: previous_positional_arg_names: List[str] = [] decorator_converter = convert_positional_args( @@ -20,6 +26,19 @@ def test_convert_positional_args_decorator() -> None: assert decorated_func.__name__ == _sample_func.__name__ +def test_convert_positional_args_future_warning_for_methods() -> None: + simple_class = _SimpleClass() + with pytest.warns(FutureWarning) as record: + simple_class.simple_method(1, 2, c=3) # type: ignore + simple_class.simple_method(1, b=2, c=3) # No warning. + simple_class.simple_method(a=1, b=2, c=3) # No warning. + + assert len(record) == 1 + for warn in record.list: + assert isinstance(warn.message, FutureWarning) + assert "simple_method" in str(warn.message) + + def test_convert_positional_args_future_warning() -> None: previous_positional_arg_names: List[str] = ["a", "b"] decorator_converter = convert_positional_args( @@ -105,4 +124,4 @@ def test_convert_positional_args_invalid_positional_args() -> None: with pytest.raises(TypeError) as record: decorated_func(1, 3, b=2) # type: ignore - assert str(record.value) == "_sample_func() got multiple values for argument 'b'." + assert str(record.value) == "_sample_func() got multiple values for arguments {'b'}." diff --git a/tests/trial_tests/test_fixed.py b/tests/trial_tests/test_fixed.py index 6d0a46cca30..0c0c0c02243 100644 --- a/tests/trial_tests/test_fixed.py +++ b/tests/trial_tests/test_fixed.py @@ -1,3 +1,7 @@ +from __future__ import annotations + +import pytest + from optuna.trial import FixedTrial @@ -10,6 +14,17 @@ def test_params() -> None: assert trial.params == params +@pytest.mark.parametrize("positional_args_names", [[], ["step"], ["step", "log"]]) +def test_suggest_int_positional_args(positional_args_names: list[str]) -> None: + # If log is specified as positional, step must also be provided as positional. + params = {"x": 1} + trial = FixedTrial(params) + kwargs = dict(step=1, log=False) + args = [kwargs[name] for name in positional_args_names] + # No error should not be raised even if the coding style is old. + trial.suggest_int("x", -1, 1, *args) + + def test_number() -> None: params = {"x": 1} trial = FixedTrial(params, 2) diff --git a/tests/trial_tests/test_frozen.py b/tests/trial_tests/test_frozen.py index 925c151e87c..7d479c10768 100644 --- a/tests/trial_tests/test_frozen.py +++ b/tests/trial_tests/test_frozen.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import copy import datetime from typing import Any @@ -11,6 +13,7 @@ from optuna import create_study from optuna.distributions import BaseDistribution from optuna.distributions import FloatDistribution +from optuna.distributions import IntDistribution from optuna.testing.storages import STORAGE_MODES from optuna.testing.storages import StorageSupplier import optuna.trial @@ -385,3 +388,26 @@ def test_create_trial_distribution_conversion_noop() -> None: # Check fixed_distributions doesn't change. assert trial.distributions == fixed_distributions + + +@pytest.mark.parametrize("positional_args_names", [[], ["step"], ["step", "log"]]) +def test_suggest_int_positional_args(positional_args_names: list[str]) -> None: + # If log is specified as positional, step must also be provided as positional. + trial = FrozenTrial( + number=0, + trial_id=0, + state=TrialState.COMPLETE, + value=0.0, + values=None, + datetime_start=datetime.datetime.now(), + datetime_complete=datetime.datetime.now(), + params={"x": 1}, + distributions={"x": IntDistribution(-1, 1)}, + user_attrs={}, + system_attrs={}, + intermediate_values={}, + ) + kwargs = dict(step=1, log=False) + args = [kwargs[name] for name in positional_args_names] + # No error should not be raised even if the coding style is old. + trial.suggest_int("x", -1, 1, *args) diff --git a/tests/trial_tests/test_trial.py b/tests/trial_tests/test_trial.py index 9a9dfa1f7fe..88de52e1a2b 100644 --- a/tests/trial_tests/test_trial.py +++ b/tests/trial_tests/test_trial.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import datetime import math from typing import Any @@ -128,11 +130,11 @@ def test_check_distribution_suggest_discrete_uniform(storage_mode: str) -> None: assert len([r for r in record if r.category != FutureWarning]) == 1 with pytest.raises(ValueError): - trial.suggest_int("x", 10, 20, 2) + trial.suggest_int("x", 10, 20, step=2) trial = Trial(study, study._storage.create_new_trial(study._study_id)) with pytest.raises(ValueError): - trial.suggest_int("x", 10, 20, 2) + trial.suggest_int("x", 10, 20, step=2) @pytest.mark.parametrize("storage_mode", STORAGE_MODES) @@ -704,3 +706,14 @@ def test_lazy_trial_system_attrs(storage_mode: str) -> None: system_attrs = _LazyTrialSystemAttrs(trial._trial_id, storage) assert set(system_attrs.items()) == {("int", 0), ("str", "A")} assert set(system_attrs.items()) == {("int", 0), ("str", "A")} + + +@pytest.mark.parametrize("positional_args_names", [[], ["step"], ["step", "log"]]) +def test_suggest_int_positional_args(positional_args_names: list[str]) -> None: + # If log is specified as positional, step must also be provided as positional. + study = optuna.create_study() + trial = study.ask() + kwargs = dict(step=1, log=False) + args = [kwargs[name] for name in positional_args_names] + # No error should not be raised even if the coding style is old. + trial.suggest_int("x", -1, 1, *args)