Replace abc.ABC with typing.Protocol in BaseSampler
#5425
Conversation
| "If the study is being used for multi-objective optimization, " | ||
| f"{self.__class__.__name__} cannot be used." | ||
| ) | ||
| def _raise_error_if_multi_objective(sampler: BaseSampler, study: Study) -> None: |
There was a problem hiding this comment.
While _rase_error_if_multi_objective is private, some user-defined samplers use it, and these will not be compatible with Optuna v4.
https://github.com/Yard1/autopipeline/blob/b3c8c325adb969ddf9585d313f89dc53e0f6c5eb/automl/search/samplers/optuna_rf_sampler.py#L694
| @@ -20,7 +27,7 @@ | |||
| from optuna.study import Study | |||
|
|
|||
|
|
|||
| class BaseSampler(abc.ABC): | |||
| class BaseSampler(Protocol): | |||
There was a problem hiding this comment.
Following this update, expressions like isinstance(foo, BaseSampler) and issubclass(bar, BaseSampler) will not work. We can see such usage in some OSS code, and it might be helpful to provide a migration guide for those affected.
https://github.com/volico/mlexp/blob/c07c96de964ea181571663a710bb2fc9a1559b25/mlexp/trainers/_base_trainer.py#L78-L80
There was a problem hiding this comment.
Although typing.runtime_checkable allows us to use isinstance and issubclass with Protocol, it comes with some drawbacks, including impacts on execution speed and code complexity. Therefore, it may not be a good idea to employ it here. https://docs.python.org/3/library/typing.html#typing.runtime_checkable
There was a problem hiding this comment.
@toshihikoyanase
Thank you for your pointing out. Where should I leave a migration guide for this change?
There was a problem hiding this comment.
@porink0424 Maybe we can publish the guide in a GitHub discussion along with other breaking changes. Could you outline the guide here, please?
Honestly, I haven't come up with straightforward ways to migrate the code. In this case, we might simply remove the isinstance assertion and rely on the type checking instead.
So, we might recommend users migrate runtime checks such as isinstance and issubclass to type checking such, for example by using mypy.
What do you think?
There was a problem hiding this comment.
Here is my suggestion for the guide:
Runtime check for BaseSampler is not available
In Optuna v4, the BaseSampler class now inherits typing.Protocol instead of abc.ABC. This change makes it possible to type-check samplers using static type checkers. However, this change makes it no longer possible to dynamically check whether a sampler is an instance of BaseSampler or whether a sampler class is a subclass of BaseSampler.
# Optuna v3.x
isinstance(sampler, optuna.samplers.BaseSampler)
issubclass(MySampler, optuna.samplers.BaseSampler)
# These checks are no longer available in Optuna v4Please use static type checking such as mypy instead of runtime checks in Optuna v4.
There was a problem hiding this comment.
Sorry for the delayed response. Seems good to me. Thank you!
porink0424
force-pushed
the
fix/replace-abc-with-protocol
branch
from
d6efd3e
to
2b45b5e
Compare
There was a problem hiding this comment.
The implementation looks OK.
I think It is required to update some documentations.
For example, the following sentence is no longer exact because duck typing allows you create a sampler without inheriting BaseSampler.
https://optuna.readthedocs.io/en/stable/tutorial/20_recipes/005_user_defined_sampler.html
To create a new sampler, you need to define a class that inherits BaseSampler.
|
@y0z |
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
|
@nabenabe0928 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5425 +/- ##
==========================================
+ Coverage 89.52% 89.72% +0.20%
==========================================
Files 194 194
Lines 12626 12562 -64
==========================================
- Hits 11303 11271 -32
+ Misses 1323 1291 -32 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@porink0424 You may already be aware, but could you please address this comment? #5425 (comment)
|
@porink0424 Thank you for your response. I believe it will also be helpful for developers. @not522 I think my comments have been resolved. Please merge after your review. |
Motivation
In Python, static type checkers are now getting popular, and there are fewer and fewer times to use dynamic ones such as
abc.ABC. This PR suggests replacingabc.ABCwithProtocolinBaseSampler.Description of the changes
abc.ABCwithtyping.ProtocolinBaseSampler.BaseSampler.typing_extensionsdependency when python version is less than 3.8.test_samplers.Note
By merging this PR, dynamic type checks would be no longer available such as
isinstance(obj, BaseSampler),issubclass(cls, BaseSampler). The migration guide will be needed for users who rely on dynamic type checks.