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
Convert all positional arguments to keyword-only #3270
Convert all positional arguments to keyword-only #3270
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3270 +/- ##
==========================================
- Coverage 91.76% 91.73% -0.04%
==========================================
Files 146 155 +9
Lines 12091 12167 +76
==========================================
+ Hits 11095 11161 +66
- Misses 996 1006 +10
Continue to review full report at Codecov.
|
Can we add tests for this decorator? |
optuna/study/study.py
Outdated
f"{func.__name__}: Positional arguments are deprecated." | ||
" Please give all values as keyword arguments." | ||
) | ||
for val, arg_name in zip(args, list(signature(func).parameters)): |
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.
Given this order-dependent logic, it might be a good idea to leave a code comment (somewhere in _convert_positional_args
?) for maintainers that the order of arguments (although keyword-only) may not be changed at the decorated functions.
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.
Actually, how about having _convert_positional_args
take a sequence of argument names instead of an integer count? Although it'd make the caller side more verbose, it'll be more robust to these types of, possibly unintentional, changes.
@_convert_positional_args(
previous_positional_arg_names=[
"storage",
"sampler",
"pruner",
"study_name",
"direction",
"load_if_exists"
]
)
def create_study(
*,
storage: Optional[Union[str, storages.BaseStorage]] = None,
sampler: Optional["samplers.BaseSampler"] = None,
pruner: Optional[pruners.BasePruner] = None,
study_name: Optional[str] = None,
direction: Optional[Union[str, StudyDirection]] = None,
load_if_exists: bool = False,
directions: Optional[Sequence[Union[str, StudyDirection]]] = None,
) -> Study:
...
Implemented something like this. Note that this logic does not require the inspect
module.
I also included some changes from AssertionError
to TypeError
. These errors are user-facing so we should probably try sticking to the latter.
def _convert_positional_args(
*, previous_positional_arg_names: Sequence[str],
) -> Callable[[Callable[..., _T]], Callable[..., _T]]:
def decorator(func: Callable[..., _T]) -> Callable[..., _T]:
@wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> _T:
# Warn the first thing we do.
if len(args) >= 1:
warnings.warn(
f"{func.__name__}: Positional arguments are deprecated."
" Please give all values as keyword arguments."
)
# Raise TypeErrors mimicking Python standard behavior.
if len(args) > len(previous_positional_arg_names):
raise TypeError(
f"{func.__name__}() takes {len(previous_positional_arg_names)} positional"
f" arguments but {len(args)} were given."
)
for val, arg_name in zip(args, previous_positional_arg_names):
if arg_name in kwargs:
raise TypeError(
f"{func.__name__}() got multiple values for argument '{arg_name}'."
)
kwargs[arg_name] = val
return func(**kwargs)
return wrapper
return decorator
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.
Looks good! I'll replace assertions with TypeError
which can inform users more than them.
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.
Was taking a quick look at the CI failure, it seemed unrelated to the change in this PR. 🤔 |
The CI failures
https://github.com/optuna/optuna/runs/4995573671?check_suite_focus=true are caused by the changes in this PR, sorry. Not entire understood the details but using Something along import typing
F = typing.TypeVar('F', bound=Callable[..., Any])
@typing.overload
def _convert_positional_args(__func: F) -> F: ...
@typing.overload
def _convert_positional_args(*, n_positional_args: int = 0) -> Callable[[F], F]: ...
def _convert_positional_args(__func: Callable[..., Any] = None, *, n_positional_args: int = 0):
def decorator(func: Callable[..., Any]):
@wraps(func)
def wrapper(*args, **kwargs) -> Any:
assert len(args) <= n_positional_args, "Too many positional arguments."
if len(args) >= 1:
warnings.warn(
f"{func.__name__}: Positional arguments are deprecated."
" Please give all values as keyword arguments."
)
for val, arg_name in zip(args, list(signature(func).parameters)):
assert arg_name not in kwargs
kwargs[arg_name] = val
return func(**kwargs)
return wrapper
if __func is not None:
return decorator(__func)
else:
return decorator Edit: |
@nzw0301 Could you continue the review, please? |
In commit 7282913, the most important change is here. - def wrapper(*args: Any, **kwargs: Any) -> Any:
+ def wrapper(*args: Any, **kwargs: Any) -> _T: The typevar |
I tried the hvy-san's suggestion using
|
For the remaining feedbacks, I'll respond after tomorrow. |
Ah, that makes sense. Thanks. 🙇 |
optuna/study/study.py
Outdated
def decorator(func: Callable[..., _T]) -> Callable[..., _T]: | ||
@wraps(func) | ||
def wrapper(*args: Any, **kwargs: Any) -> _T: | ||
assert len(previous_positional_arg_names) <= len(signature(func).parameters), ( |
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.
To check previous_positional_arg_names
list is valid, I add signature info. of function and compare them here. This helps developers to keep mapping previous signature to new signature, but my concern is that this is just a runtime overhead for users...
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.
Sounds good. With this logic, my understanding is that there's a runtime overhead in each invocation to the decorator function, but we only need to perform this check once, when applying the decorator, i.e. during module import? We can do this moving this assertion to an outer scope, right below def decorator(func ...):
.
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! Fixed in 9e6b9ad and it works as intended.
I have added decorator tests. Please take a look. |
Left a single comment but logic seems good otherwise. Cc. @himkt, did you want to verify this with VSCode? |
Sorry, wasn't entire sure what you meant with "blocked" here. Would you mind explaining a bit more? 🙇 |
Sorry for the ambiguous comment @hvy. |
Just got clarified by @himkt that the current behavior fine and that it's not blocked in any sense. |
I have introduced 8510858 is a fix for argument mismatch caused by the change of XXX_study(). |
According to https://github.com/optuna/optuna/runs/5075685882, we have to add |
Thank you @higucheese for investigating and improving typing stuff!
If we add typing_extensions to checking extras, it needs to use
We don't have the explicit rule for that and carefully consider for each case. log:
|
@himkt could you jump in this PR as a reviewer instead of me? |
Sure, assigned! |
For the suggestion, I am going to do the move by myself if it's no trouble (This PR is not the kind of thing that needs to be merged in a hurry, is it?). |
Do we need any comments (or printing info.) to lead users to the issue? |
Thank you so much, no extra rush. 👍 |
I moved |
Sounds good, I think including the URL to #2911 in the warning message could be user friendly. It'll go against moving this decorator to |
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 a lot for the quick updates!
"to_storage", | ||
"to_study_name", | ||
], | ||
warning_stacklevel=3, |
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.
👍
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.
👍
[memo]
- warning_stacklevel == 2 (default)
> python sam.py 2022-02-22 10:49:18
[I 2022-02-22 10:49:23,077] A new study created in RDB with name: test
/Users/himkt/work/github.com/himkt/optuna/sam.py:5: ExperimentalWarning: copy_study is experimental (supported from v2.8.0). The interface can change in the future.
optuna.copy_study("test", "sqlite:////tmp/tmp.db", "sqlite:////tmp/tmp2.db")
/Users/himkt/work/github.com/himkt/optuna/optuna/_experimental.py:68: FutureWarning: copy_study(): Please give all values as keyword arguments. See https://github.com/optuna/optuna/issues/3324 for details.
return func(*args, **kwargs) # type: ignore
[I 2022-02-22 10:49:23,135] A new study created in RDB with name: test
/Users/himkt/work/github.com/himkt/optuna/optuna/study/study.py:1446: ExperimentalWarning: add_trials is experimental (supported from v2.5.0). The interface can change in the future.
to_study.add_trials(from_study.get_trials(deepcopy=False))
- warning_stacklevel == 3
> python sam.py 2022-02-22 10:49:34
[I 2022-02-22 10:49:37,185] A new study created in RDB with name: test
/Users/himkt/work/github.com/himkt/optuna/sam.py:5: ExperimentalWarning: copy_study is experimental (supported from v2.8.0). The interface can change in the future.
optuna.copy_study("test", "sqlite:////tmp/tmp.db", "sqlite:////tmp/tmp2.db")
/Users/himkt/work/github.com/himkt/optuna/sam.py:5: FutureWarning: copy_study(): Please give all values as keyword arguments. See https://github.com/optuna/optuna/issues/3324 for details.
optuna.copy_study("test", "sqlite:////tmp/tmp.db", "sqlite:////tmp/tmp2.db")
[I 2022-02-22 10:49:37,249] A new study created in RDB with name: test
/Users/himkt/work/github.com/himkt/optuna/optuna/study/study.py:1447: ExperimentalWarning: add_trials is experimental (supported from v2.5.0). The interface can change in the future.
to_study.add_trials(from_study.get_trials(deepcopy=False))
Sounds good. We can summarize our purpose and historical reasons of this change in the new issue. |
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.
Thank a lot for the long running PR, LGTM!
Again, I think including the issue url to the warning message could be friendly, it's not a strong opinion though.
Sorry for my late responding. I created #3324 to describe the purpose of these changes and updated the warning comment. |
Thank you for the update! I'll check it tomorrow. |
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.
Thank you so much @higucheese! LGTM.
I just found warning message is repeatedly displayed if stacklevel is 2 (it doesn't happen on an interactive environment). I was concerning the problem of the massive warning as in #3301. But I confirm it doesn't affect other places so I think it is acceptable.
- warn_stacklevel == 2 (default)
> python main.py 2022-02-22 10:57:33
/Users/himkt/work/github.com/himkt/optuna/main.py:8: FutureWarning: create_study(): Please give all values as keyword arguments. See https://github.com/optuna/optuna/issues/3324 for details.
study = optuna.create_study("sqlite:////tmp/tmp.db", study_name="test1")
[I 2022-02-22 10:57:35,472] A new study created in RDB with name: test1
/Users/himkt/work/github.com/himkt/optuna/main.py:9: FutureWarning: create_study(): Please give all values as keyword arguments. See https://github.com/optuna/optuna/issues/3324 for details.
study = optuna.create_study("sqlite:////tmp/tmp.db", study_name="test2")
[I 2022-02-22 10:57:35,492] A new study created in RDB with name: test2
/Users/himkt/work/github.com/himkt/optuna/optuna/trial/_trial.py:169: FutureWarning: UniformDistribution has been deprecated in v3.0.0. This feature will be removed in v6.0.0. See https://github.com/optuna/optuna/releases/tag/v3.0.0. Use :class:`~optuna.distributions.FloatDistribution` instead.
distribution = UniformDistribution(low=low, high=high)
/Users/himkt/work/github.com/himkt/optuna/optuna/distributions.py:561: FutureWarning: UniformDistribution has been deprecated in v3.0.0. This feature will be removed in v6.0.0. See https://github.com/optuna/optuna/releases/tag/v3.0.0. Use :class:`~optuna.distributions.FloatDistribution` instead.
return cls(**json_dict["attributes"])
[I 2022-02-22 10:57:35,523] Trial 0 finished with value: 5.472289124011194 and parameters: {'v': 5.472289124011194}. Best is trial 0 with value: 5.472289124011194.
[I 2022-02-22 10:57:35,540] Trial 1 finished with value: 1.8239470731739338 and parameters: {'v': 1.8239470731739338}. Best is trial 1 with value: 1.8239470731739338.
[I 2022-02-22 10:57:35,556] Trial 2 finished with value: 3.1063082890049643 and parameters: {'v': 3.1063082890049643}. Best is trial 1 with value: 1.8239470731739338.
- warn_stacklevel == 1
> python main.py 2022-02-22 10:59:12
/Users/himkt/work/github.com/himkt/optuna/optuna/_convert_positional_args.py:40: FutureWarning: create_study(): Please give all values as keyword arguments. See https://github.com/optuna/optuna/issues/3324 for details.
warnings.warn(
[I 2022-02-22 10:59:16,198] A new study created in RDB with name: test1
[I 2022-02-22 10:59:16,221] A new study created in RDB with name: test2
/Users/himkt/work/github.com/himkt/optuna/optuna/trial/_trial.py:169: FutureWarning: UniformDistribution has been deprecated in v3.0.0. This feature will be removed in v6.0.0. See https://github.com/optuna/optuna/releases/tag/v3.0.0. Use :class:`~optuna.distributions.FloatDistribution` instead.
distribution = UniformDistribution(low=low, high=high)
/Users/himkt/work/github.com/himkt/optuna/optuna/distributions.py:561: FutureWarning: UniformDistribution has been deprecated in v3.0.0. This feature will be removed in v6.0.0. See https://github.com/optuna/optuna/releases/tag/v3.0.0. Use :class:`~optuna.distributions.FloatDistribution` instead.
return cls(**json_dict["attributes"])
[I 2022-02-22 10:59:16,259] Trial 0 finished with value: 1.4824433429684192 and parameters: {'v': 1.4824433429684192}. Best is trial 0 with value: 1.4824433429684192.
[I 2022-02-22 10:59:16,278] Trial 1 finished with value: 4.64190930455168 and parameters: {'v': 4.64190930455168}. Best is trial 0 with value: 1.4824433429684192.
[I 2022-02-22 10:59:16,297] Trial 2 finished with value: 0.16982925381998415 and parameters: {'v': 0.16982925381998415}. Best is trial 2 with value: 0.16982925381998415.
diff --git a/optuna/_convert_positional_args.py b/optuna/_convert_positional_args.py
index cfcd6b4b4..f9936e93b 100644
--- a/optuna/_convert_positional_args.py
+++ b/optuna/_convert_positional_args.py
@@ -36,6 +36,7 @@ def convert_positional_args(
def converter_wrapper(*args: Any, **kwargs: Any) -> _T:
if len(args) >= 1:
+ warning_stacklevel = 1
warnings.warn(
f"{func.__name__}(): Please give all values as keyword arguments."
" See https://github.com/optuna/optuna/issues/3324 for details.",
Thanks @himkt for the review, and again @higucheese for the PR. |
Motivation
This PR is based on #2911, but simply changing the order of positional arguments of
create_study
orload_study
can confuse users. So this aims to encourage them to use keyword-only arguments first without any changes to the interface. Once users get used to keyword-only arguments, it gets much easier to align their arguments.Description of the changes
{create,load,delete,copy}_study()
to keyword-only arguments.{create,load,delete,copy}_study()
sets values as positional arguments, the decorator_convert_positional_args
converts them to keyword arguments according to the signature of the decorated function and sets a warning.