Add missing `init.py` files #1367

harupy · 2020-06-14T12:37:44Z

Motivation

mypy ignores directories that don't contain __init__.py.

Description of the changes

Add __init__.py files.

I ran the test below to make sure all the directories contain __init__.py

import os


def test_init_exists_in_all_directories() -> None:
    dirs = []
    for top in ["optuna", "tests"]:
        for root, _, files in os.walk(top):
            if any(f.endswith(".py") for f in files) and "__init__.py" not in files:
                dirs.append(root)
    assert len(dirs) == 0, "Found directories that don't contain `__init__.py`: {}".format(dirs)

harupy · 2020-06-15T04:46:33Z

mypy check failed:

tests/multi_objective/test_trial.py:25: error: Unsupported operand types for + ("float" and "None")
tests/multi_objective/test_trial.py:25: error: Unsupported operand types for + ("float" and "str")
tests/multi_objective/test_trial.py:25: note: Right operand is of type "Union[None, bool, int, float, str]"
tests/multi_objective/test_trial.py:35: error: Argument 1 to "report" of "MultiObjectiveTrial" has incompatible type "Tuple[int, int, int]"; expected "Tuple[float]"
tests/multi_objective/test_trial.py:36: error: Argument 1 to "report" of "MultiObjectiveTrial" has incompatible type "Tuple[int, int, int]"; expected "Tuple[float]"
tests/multi_objective/test_trial.py:150: error: Value expression in dictionary comprehension has incompatible type "Optional[float]"; expected type "float"
tests/multi_objective/samplers_tests/test_nsga2.py:41: error: Argument "population_size" to "NSGAIIMultiObjectiveSampler" has incompatible type "float"; expected "int"
optuna/storages/rdb/alembic/env.py:32: error: Function is missing a return type annotation
optuna/storages/rdb/alembic/env.py:32: note: Use "-> None" if function does not return a value
optuna/storages/rdb/alembic/env.py:51: error: Function is missing a return type annotation
optuna/storages/rdb/alembic/env.py:51: note: Use "-> None" if function does not return a value
tests/multi_objective/test_study.py:131: error: Incompatible return value type (got "Tuple[float, float]", expected "List[float]")
Found 9 errors in 4 files (checked 171 source files)

harupy · 2020-06-15T05:14:43Z

tests/multi_objective/test_trial.py:35: error: Argument 1 to "report" of "MultiObjectiveTrial" has incompatible type "Tuple[int, int, int]"; expected "Tuple[float]"
tests/multi_objective/test_trial.py:36: error: Argument 1 to "report" of "MultiObjectiveTrial" has incompatible type "Tuple[int, int, int]"; expected "Tuple[float]"

It seems the type annotation of values in multi_objective should be fixed. It is annotated as Tuple[float] which represents a single-element tuple, but it should be Tuple[float, ...] which represents a variable-length tuple.

optuna/optuna/multi_objective/trial.py

Line 111 in e7afd40

def report(self, values: Tuple[float], step: int) -> None:

hvy · 2020-06-15T11:09:27Z

Good catch! We should address the failures in the same PR. Taking a brief look through the errors, they mostly seem straight forward to address (e.g. by introducing assertions). Do you want to do that here? I otherwise suggest we split up this PR into multiple parts.

toshihikoyanase · 2020-06-15T11:46:40Z

Thank you for finding the missing type check.

I think optuna/storages/rdb/alembic/ is a kind of settings for alembic rather than a part of Optuna's submodules (i.e., Optuna doesn't import it as a module.) It may be controversial to include the alembic submodule only for mypy.

What do you think?

harupy · 2020-06-15T13:15:02Z

@hvy Got it. I'll fix all the errors in this PR.

@toshihikoyanase If so, I'll make mypy ignore files under alembic.

harupy · 2020-06-16T01:47:01Z

tests/multi_objective/test_trial.py

        p0 = trial.suggest_float("p0", -10, 10)
        p1 = trial.suggest_uniform("p1", 3, 5)
        p2 = trial.suggest_loguniform("p2", 0.00001, 0.1)
        p3 = trial.suggest_discrete_uniform("p3", 100, 200, q=5)
        p4 = trial.suggest_int("p4", -20, -15)
-        p5 = trial.suggest_categorical("p5", [7, 1, 100])
+        p5 = cast(int, trial.suggest_categorical("p5", [7, 1, 100]))


https://stackoverflow.com/questions/44739764/how-to-cast-a-typing-union-to-one-of-its-subtypes-in-python

This is just an idea. It'd be nice if we could infer the return type of suggest_categorical in a generic way like "return int if int list given" instead of returning Union of several python primitive types because this enforces users to cast a suggested value to resolve type errors when they use Optuna with a type-checking tool like mypy.

harupy · 2020-06-16T04:26:20Z

The tests in tests/test_cli.py failed.

toshihikoyanase · 2020-06-16T04:29:20Z

@harupy I found similar issues in other PRs (e.g., https://circleci.com/gh/optuna/optuna/45221?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link) and daily CI (https://github.com/optuna/optuna/actions/runs/136123282). I'm sorry but I'm under investigation.

codecov-commenter · 2020-06-16T09:10:35Z

Codecov Report

Merging #1367 into master will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
+ Coverage   87.92%   88.06%   +0.14%     
==========================================
  Files          98       97       -1     
  Lines        7376     7297      -79     
==========================================
- Hits         6485     6426      -59     
+ Misses        891      871      -20

Impacted Files	Coverage Δ
optuna/multi_objective/study.py	`97.36% <100.00%> (+0.02%)`	⬆️
optuna/multi_objective/trial.py	`91.21% <100.00%> (-0.06%)`	⬇️
optuna/trial/_base.py	`56.36% <0.00%> (-2.26%)`	⬇️
optuna/integration/lightgbm_tuner/optimize.py	`91.82% <0.00%> (-0.26%)`	⬇️
optuna/study.py	`94.71% <0.00%> (-0.17%)`	⬇️
optuna/_study_summary.py	`100.00% <0.00%> (ø)`
optuna/samplers/cmaes.py
optuna/storages/redis.py
optuna/storages/base.py
optuna/visualization/param_importances.py
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5940731...943792c. Read the comment docs.

hvy

Why don't we allow both tuples and lists (sequence) giving the user some flexibility in how to design their objective functions? It's cumbersome having to convert lists to tuples. This is not a strong opinion though.

harupy · 2020-06-16T12:14:03Z

That makes sense. I'll fix the type annotaions.

harupy · 2020-06-22T00:33:51Z

@hvy I have fixed the type annotations to use Sequence instead of Tuple. Could you review the code again?

hvy

Sorry for the delayed response, and thanks for fixing the details. LGTM!

hvy · 2020-06-22T00:43:46Z

@toshihikoyanase could you cont. for the second approval?

toshihikoyanase

LGTM. Thank you for your contribution!

@hvy Thank you for letting me know.

harupy added 2 commits June 14, 2020 21:33

Add missing __init__.py

33edffe

Merge branch 'master' into add-missing-init

62bc6a5

hvy added the code-fix Change that does not change the behavior, such as code refactoring. label Jun 15, 2020

hvy self-assigned this Jun 15, 2020

toshihikoyanase self-assigned this Jun 15, 2020

harupy added 7 commits June 15, 2020 22:26

Fix objective func type for multi_objective

1c955d2

Fix errors on suggest_categorical

5f5685c

Remove unnecessary Optional

012f789

Ignore intentional type error

df91166

Remove __init__.py under alembic

6bc1cf8

Fix type error in test_callbacks

9a770ba

Remove unused import

7d95775

harupy commented Jun 16, 2020

View reviewed changes

run again

8d4f11e

hvy reviewed Jun 16, 2020

View reviewed changes

harupy added 2 commits June 16, 2020 21:16

Use Sequence instead of Tuple

8989e69

Remove unused import

943792c

hvy approved these changes Jun 22, 2020

View reviewed changes

toshihikoyanase approved these changes Jun 22, 2020

View reviewed changes

toshihikoyanase merged commit 35a8d7e into optuna:master Jun 22, 2020

toshihikoyanase added this to the v2.0.0 milestone Jun 22, 2020

harupy deleted the add-missing-init branch June 22, 2020 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing `init.py` files #1367

Add missing `init.py` files #1367

harupy commented Jun 14, 2020 •

edited

harupy commented Jun 15, 2020 •

edited

harupy commented Jun 15, 2020 •

edited

hvy commented Jun 15, 2020

toshihikoyanase commented Jun 15, 2020

harupy commented Jun 15, 2020

harupy Jun 16, 2020

harupy Jun 22, 2020 •

edited

harupy commented Jun 16, 2020

toshihikoyanase commented Jun 16, 2020

codecov-commenter commented Jun 16, 2020 •

edited

hvy left a comment

harupy commented Jun 16, 2020

harupy commented Jun 22, 2020

hvy left a comment

hvy commented Jun 22, 2020

toshihikoyanase left a comment

Add missing __init__.py files #1367

Add missing __init__.py files #1367

Conversation

harupy commented Jun 14, 2020 • edited

Motivation

Description of the changes

harupy commented Jun 15, 2020 • edited

harupy commented Jun 15, 2020 • edited

hvy commented Jun 15, 2020

toshihikoyanase commented Jun 15, 2020

harupy commented Jun 15, 2020

harupy Jun 16, 2020

Choose a reason for hiding this comment

harupy Jun 22, 2020 • edited

Choose a reason for hiding this comment

harupy commented Jun 16, 2020

toshihikoyanase commented Jun 16, 2020

codecov-commenter commented Jun 16, 2020 • edited

Codecov Report

hvy left a comment

Choose a reason for hiding this comment

harupy commented Jun 16, 2020

harupy commented Jun 22, 2020

hvy left a comment

Choose a reason for hiding this comment

hvy commented Jun 22, 2020

toshihikoyanase left a comment

Choose a reason for hiding this comment

Add missing `init.py` files #1367

Add missing `init.py` files #1367

harupy commented Jun 14, 2020 •

edited

harupy commented Jun 15, 2020 •

edited

harupy commented Jun 15, 2020 •

edited

harupy Jun 22, 2020 •

edited

codecov-commenter commented Jun 16, 2020 •

edited