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

Make typing_extensions optional. #3990

Merged

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Sep 12, 2022

Motivation

An alternative approach for #3980 to fix typing_extensions related issues.

Description of the changes

Check the typing.TYPE_CHECKING flag to check if type checking is enabled.

@github-actions github-actions bot added the optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. label Sep 12, 2022
@c-bata c-bata force-pushed the make-typing-extensions-optional branch from 646b2b6 to 396d762 Compare September 12, 2022 05:45
@c-bata c-bata marked this pull request as ready for review September 12, 2022 05:54
@codecov-commenter
Copy link

Codecov Report

Merging #3990 (396d762) into master (79f0329) will decrease coverage by 0.10%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master    #3990      +/-   ##
==========================================
- Coverage   90.60%   90.50%   -0.11%     
==========================================
  Files         167      167              
  Lines       13111    13119       +8     
==========================================
- Hits        11879    11873       -6     
- Misses       1232     1246      +14     
Impacted Files Coverage Δ
optuna/_convert_positional_args.py 89.28% <57.14%> (-10.72%) ⬇️
optuna/integration/pytorch_distributed.py 35.91% <57.14%> (-0.96%) ⬇️
optuna/_deprecated.py 94.28% <60.00%> (-5.72%) ⬇️
optuna/_experimental.py 92.45% <60.00%> (-7.55%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@contramundum53 contramundum53 added installation Installation and dependency. needs-discussion Issue/PR which needs discussion. labels Sep 12, 2022
@c-bata c-bata removed the needs-discussion Issue/PR which needs discussion. label Sep 13, 2022
@contramundum53
Copy link
Member

I removed typing_extension and the tests ran successfully.
(Except for importance_tests and sampler_tests/test_samplers.py which use torch internally.)
LGTM.

@c-bata c-bata modified the milestones: v3.0.2, v3.1.0 Sep 13, 2022
@toshihikoyanase
Copy link
Member

toshihikoyanase commented Sep 13, 2022

I don't think this is a blocker of this PR, but I found that typing-extensions was installed along with SQLAlchemy in Python 3.6 and 3.7. Without typing-extensions, users cannot import optuna due to the following errors.

$ python -c "import optuna"
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/importlib_metadata/_compat.py", line 9, in <module>
    from typing import Protocol
ImportError: cannot import name 'Protocol'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
...
  File "/proj/optuna/storages/_rdb/storage.py", line 27, in <module>
    from optuna.storages._rdb.models import TrialValueModel
  File "/proj/optuna/storages/_rdb/models.py", line 8, in <module>
    from sqlalchemy import asc
...
  File "/usr/local/lib/python3.6/site-packages/importlib_metadata/_meta.py", line 1, in <module>
    from ._compat import Protocol
  File "/usr/local/lib/python3.6/site-packages/importlib_metadata/_compat.py", line 11, in <module>
    from typing_extensions import Protocol  # type: ignore
ModuleNotFoundError: No module named 'typing_extensions'

The reproducible code is as follows:

  1. Launch a container of the specific version of the python docker image.
$  git clone https://github.com/c-bata/optuna.git -b make-typing-extensions-optional
$ cd optuna
$ MINOR_VERSION=7 
$ docker run -it --rm -v $(pwd):/proj --workdir=/proj python:3.$MINOR_VERSION bash
  1. Execute following commands
$ python --version
$ pip install -U pip
$ pip install .
$ python -c "import optuna"
$ pip freeze | grep typing
$ pip uninstall typing-extensions
$ python -c "import optuna"

The results are shown in the following table:

Python typing-extensions import optuna without typing-extensions
3.6.15 4.1.1 ImportError
3.7.12 4.3.0 ImportError
3.8.12 not installed ✔️
3.9.13 not installed ✔️
3.10.4 not installed ✔️

@toshihikoyanase
Copy link
Member

(e.g. We cannot install tensorflow=2.5.0 with typing_extensions>=3.10.0.0).

With this PR, we can use tensorflow=2.5.0 and optuna, if we install tensorflow in prior to optuna.

root@fee7c0b17cda:/proj# pip freeze | grep tensorflow
tensorflow==2.5.0
tensorflow-estimator==2.5.0
root@fee7c0b17cda:/proj# pip freeze | grep typing
typing-extensions==3.7.4.3
root@fee7c0b17cda:/proj# python -c "import optuna"

@c-bata
Copy link
Member Author

c-bata commented Sep 14, 2022

@toshihikoyanase Thank you for your review!
It looks like the issue of SQLAlchemy. I checked that typing-extensions are specified in install_requires for each version of SQLAlchemy. The results are as follows:

Even though Optuna's RDBStorage users may bump into this problem, I don't think typing-extensions should be included in Optuna's install_requires. What do you think?

@toshihikoyanase
Copy link
Member

Even though Optuna's RDBStorage users may bump into this problem, I don't think typing-extensions should be included in Optuna's install_requires. What do you think?

I agree with you. With this PR, the typing_extensions will not be a dependency of Optuna, so I don't think we have to keep it in the requirements. In addition, users have workaround to use libraries that requires older version of typing_extensions, so the original issue seems to be resolved. I'll approve this PR after checking the behavior of code suggestion features of IDE (i.e., VS Code).

@toshihikoyanase
Copy link
Member

In the latest release SQLAlchemy v1.4.41, typing_extensions is not listed in install_requires.
https://github.com/sqlalchemy/sqlalchemy/blob/rel_1_4_41/setup.cfg

Actually, it indirectly depends on typing_extensions via importlib-metadata.

And also, PrettyTable required by cliff also depends on importlib-metadata. So, some other libraries might depends on typing_extensions and it may be difficult to remove typing_extensions from the indirect dependencies.

@toshihikoyanase
Copy link
Member

In my environment, the code suggestion of VS Code worked as expected. I cannot understand why, but it worked even if typing_extensions was not installed in the virtual environment.

The code suggestion for the deprecated method (i.e., trial.suggest_uniform) is shown as follows:
image

The code suggestion for a method with @convert_positional_args (i.e., optuna.create_study) is shown as follows:
image

The code suggestion for an experimental method (i.e., optuna.storages.RetryFailedTrialCallback) is shown as follows:
image

Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

LGTM.

@toshihikoyanase toshihikoyanase merged commit 09306ae into optuna:master Sep 14, 2022
@c-bata c-bata deleted the make-typing-extensions-optional branch September 14, 2022 02:57
contramundum53 pushed a commit to contramundum53/optuna that referenced this pull request Sep 15, 2022
contramundum53 added a commit to contramundum53/optuna that referenced this pull request Sep 15, 2022
c-bata pushed a commit to c-bata/optuna that referenced this pull request Sep 15, 2022
contramundum53 added a commit that referenced this pull request Sep 15, 2022
[Backport] Merge pull request #3990 from c-bata/make-typing-extensions-optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation Installation and dependency. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants