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 typehint for deprecated and experimental #3575

Merged
merged 20 commits into from Jun 7, 2022
Merged

Add typehint for deprecated and experimental #3575

merged 20 commits into from Jun 7, 2022

Conversation

himkt
Copy link
Member

@himkt himkt commented May 20, 2022 •
edited

For #3572.

The main idea comes from #3270 (microsoft/pyright#774 (comment) for the original context)
I use @overload to enable type inference for both function and class.

Motivation

This PR aims to enable type inference for deprecated or experimental APIs.

Description of the changes

himkt
himkt commented May 20, 2022
View reviewed changes
optuna/_deprecated.py
@@ -134,7 +150,7 @@ def wrapped_init(self, *args, **kwargs) -> None: # type: ignore

_original_init(self, *args, **kwargs)

cls.__init__ = wrapped_init
setattr(cls, "__init__", wrapped_init)
Copy link
Member Author

Choose a reason for hiding this comment

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

python/mypy#2427 (comment)
(But some linter is not happy with the hack: python/mypy#2427 (comment), I'm not sure whether it is preferred or not)

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2022 •
edited

Codecov Report

Merging #3575 (cfbdfe5) into master (6349240) will increase coverage by 0.01%.
The diff coverage is 99.00%.

@@            Coverage Diff             @@
##           master    #3575      +/-   ##
==========================================
+ Coverage   90.96%   90.98%   +0.01%     
==========================================
  Files         160      160              
  Lines       12287    12309      +22     
==========================================
+ Hits        11177    11199      +22     
  Misses       1110     1110
Impacted Files Coverage Δ
optuna/integration/fastaiv1.py 0.00% <0.00%> (ø)
optuna/_callbacks.py 97.29% <100.00%> (+0.07%) ⬆️
optuna/_deprecated.py 100.00% <100.00%> (ø)
optuna/_experimental.py 100.00% <100.00%> (ø)
optuna/distributions.py 97.57% <100.00%> (ø)
optuna/integration/botorch.py 98.68% <100.00%> (+<0.01%) ⬆️
optuna/integration/catalyst.py 84.61% <100.00%> (ø)
optuna/integration/catboost.py 92.10% <100.00%> (ø)
optuna/integration/chainermn.py 93.93% <100.00%> (ø)
optuna/integration/cma.py 99.15% <100.00%> (ø)
... and 42 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@himkt
Copy link
Member Author

himkt commented May 20, 2022 

optuna/_deprecated.py:171: error: Argument 1 to "_deprecated_class" has incompatible type "Type[Any]"; expected "CT"
optuna/_deprecated.py:173: error: Argument 1 to "_deprecated_func" has incompatible type "Union[Callable[FP, FT], CT]"; expected "Callable[FP, FT]"

optuna/optuna/_experimental.py

Line 123 in f98a6de

return _experimental_class(f) if inspect.isclass(f) else _experimental_func(f)

I think it's difficult to capture TypeVar in the codebase. Because we need to utilize typing feature, I'm thinking of dividing deprecated and experimental into ones for a function and others for a class.

@himkt
Copy link
Member Author

himkt commented May 21, 2022

File changes is extremely huge since this PR divides experiment and deprecated into _func and _class. If someone know more sophisticated approach (i.e. experiment and deprecated can be left with type inference), please open a PR and let me know (I'll close it on response).

@contramundum53 contramundum53 removed their assignment May 25, 2022
@HideakiImamura HideakiImamura self-assigned this May 25, 2022
not522
not522 reviewed May 31, 2022
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.

Sorry for my late response. I agree to divide the decorators for functions and classes, which clarify the types. I will check the implementation detail later.

not522
not522 approved these changes Jun 2, 2022
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 2, 2022
@HideakiImamura
Copy link
Member

@himkt Could you resolve a conflict?

HideakiImamura
HideakiImamura approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@HideakiImamura HideakiImamura 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 late. LGTM except a very minor comment.

optuna/_deprecated.py Outdated Show resolved Hide resolved
@HideakiImamura
Copy link
Member

I'd like to merge this PR after the conflict is resolved.

himkt and others added 4 commits June 7, 2022 15:15
@himkt
Copy link
Member Author

himkt commented Jun 7, 2022

I'm sorry I found the additional docstring error. 616dff2 fixes them.

@HideakiImamura
Copy link
Member

Let me merge this PR. Thanks for the great work!

@HideakiImamura HideakiImamura merged commit 07a8193 into optuna:master Jun 7, 2022
@HideakiImamura HideakiImamura added this to the v3.0.0-rc0 milestone Jun 7, 2022
@HideakiImamura HideakiImamura added the code-fix Change that does not change the behavior, such as code refactoring. label Jun 7, 2022
@HideakiImamura HideakiImamura mentioned this pull request Jun 27, 2022
Closed
2 tasks
blazespinnaker
blazespinnaker reviewed Aug 18, 2022
View reviewed changes
optuna/_deprecated.py
import warnings

from packaging import version
from typing_extensions import ParamSpec
Copy link

Choose a reason for hiding this comment

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

does this break for older versions of typing_extensions, see https://stackoverflow.com/questions/71944041/using-python-code-typed-with-paramspec-on-older-versions-of-python @himkt

@himkt himkt deleted the decorator-typehint branch January 6, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blazespinnaker blazespinnaker blazespinnaker left review comments

@nzw0301 nzw0301 nzw0301 left review comments

@not522 not522 not522 approved these changes

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@HideakiImamura HideakiImamura

Labels
code-fix Change that does not change the behavior, such as code refactoring.
Projects
None yet
Milestone
v3.0.0-rc0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@himkt @codecov-commenter @HideakiImamura @not522 @blazespinnaker @nzw0301 @contramundum53