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.

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

Add annotation for distributions #220

Merged
merged 2 commits into from
Apr 24, 2022
Merged

Add annotation for distributions #220

merged 2 commits into from
Apr 24, 2022

Conversation

himkt
Copy link
Member

@himkt himkt commented Apr 24, 2022

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

N/A

What does this implement/fix? Explain your changes.

distributions in test_search_space.py is not compatible with the type annotation of crate_trial.
We observe mypy failure like following when a check is run with Optuna v3.0.0b0.

> mypy python_tests optuna_dashboard                                                                                                                                                                 2022-04-24 16:30:44
python_tests/test_search_space.py:40: error: Argument "distributions" to "create_trial" has incompatible type "Dict[str, UniformDistribution]"; expected "Optional[Dict[str, BaseDistribution]]"
python_tests/test_search_space.py:40: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
python_tests/test_search_space.py:40: note: Consider using "Mapping" instead, which is covariant in the value type
python_tests/test_search_space.py:71: error: Argument "distributions" to "create_trial" has incompatible type "Dict[str, UniformDistribution]"; expected "Optional[Dict[str, BaseDistribution]]"
python_tests/test_search_space.py:71: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
python_tests/test_search_space.py:71: note: Consider using "Mapping" instead, which is covariant in the value type
python_tests/test_search_space.py:108: error: Argument "distributions" to "create_trial" has incompatible type "Dict[str, UniformDistribution]"; expected "Optional[Dict[str, BaseDistribution]]"
python_tests/test_search_space.py:108: note: "Dict" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
python_tests/test_search_space.py:108: note: Consider using "Mapping" instead, which is covariant in the value type
Found 3 errors in 1 file (checked 15 source files)

distributions has a type dict[str, UniformDistribution] that is not compatible with Dict[str, BaseDistribution]. It means the type is not valid in the current implementation even if CIs are all green in the latest master branch.

Optuna v2.10.0 unintentionally hides the information because @experimental decorator crushes a type hint of a method. Optuna v3.0.0 makes create_trial stable (i.e. removes @experimental) and exposes type information to third-party libraries correctly. This is why we observe the mypy failure by upgrading the Optuna.

https://github.com/optuna/optuna/blob/v2.10.0/optuna/trial/_frozen.py#L505-L516

v2.10.0 v3.0.0b0
v2 10 0 3 0 0b0

This PR adds type hints List[Dict[str, BaseDistribution]] for distributions, which is compatible with Optional[Dict[str, BaseDistribution]] of create_trial.

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.

$ mypy python_tests
Success: no issues found in 6 source files

LGTM after passed all CI tests!!

@c-bata c-bata merged commit ac4c8cf into optuna:main Apr 24, 2022
@himkt himkt deleted the mypy-typing branch April 24, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants