Merged
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
===========================================
+ Coverage 35.95% 54.12% +18.17%
===========================================
Files 21 23 +2
Lines 865 1273 +408
===========================================
+ Hits 311 689 +378
- Misses 554 584 +30 ☔ View full report in Codecov by Sentry. |
This was referenced Feb 5, 2024
Merged
Comment on lines
+89
to
+110
|
|
||
| sys.modules[__name__] = _IntegrationModule(__name__) | ||
|
|
||
| __all__ = [ | ||
| "AllenNLPExecutor", | ||
| "AllenNLPPruningCallback", | ||
| "CatalystPruningCallback", | ||
| "CatBoostPruningCallback", | ||
| "ChainerMNStudy", | ||
| "ChainerPruningExtension", | ||
| "FastAIPruningCallback", | ||
| "FastAIV1PruningCallback", | ||
| "FastAIV2PruningCallback", | ||
| "KerasPruningCallback", | ||
| "MXNetPruningCallback", | ||
| "OptunaSearchCV", | ||
| "ShapleyImportanceEvaluator", | ||
| "SkorchPruningCallback", | ||
| "TensorBoardCallback", | ||
| "TensorFlowPruningHook", | ||
| "TFKerasPruningCallback", | ||
| ] |
Member
There was a problem hiding this comment.
Suggested change
| sys.modules[__name__] = _IntegrationModule(__name__) | |
| __all__ = [ | |
| "AllenNLPExecutor", | |
| "AllenNLPPruningCallback", | |
| "CatalystPruningCallback", | |
| "CatBoostPruningCallback", | |
| "ChainerMNStudy", | |
| "ChainerPruningExtension", | |
| "FastAIPruningCallback", | |
| "FastAIV1PruningCallback", | |
| "FastAIV2PruningCallback", | |
| "KerasPruningCallback", | |
| "MXNetPruningCallback", | |
| "OptunaSearchCV", | |
| "ShapleyImportanceEvaluator", | |
| "SkorchPruningCallback", | |
| "TensorBoardCallback", | |
| "TensorFlowPruningHook", | |
| "TFKerasPruningCallback", | |
| ] | |
| __all__ = [ | |
| "AllenNLPExecutor", | |
| "AllenNLPPruningCallback", | |
| "CatalystPruningCallback", | |
| "CatBoostPruningCallback", | |
| "ChainerMNStudy", | |
| "ChainerPruningExtension", | |
| "FastAIPruningCallback", | |
| "FastAIV1PruningCallback", | |
| "FastAIV2PruningCallback", | |
| "KerasPruningCallback", | |
| "MXNetPruningCallback", | |
| "OptunaSearchCV", | |
| "ShapleyImportanceEvaluator", | |
| "SkorchPruningCallback", | |
| "TensorBoardCallback", | |
| "TensorFlowPruningHook", | |
| "TFKerasPruningCallback", | |
| ] | |
| sys.modules[__name__] = _IntegrationModule(__name__) | |
__all__ should be defined inside _IntegrationModule class, because the module itself will be overwritten.
Member
There was a problem hiding this comment.
I think this can be discussed in a follow-up PR, since the same bug exists in optuna.integration. Let's merge this PR first.
contramundum53
approved these changes
Feb 6, 2024
Member
contramundum53
left a comment
There was a problem hiding this comment.
Otherwise, LGTM!
I checked that all existing modules and symbols inside are accessible directly from optuna_integration module, with the following test:
import pytest
import pkgutil
import importlib
import optuna_integration
exclude_modules = ["version"]
submodule_names = []
for importer, modname, ispkg in pkgutil.iter_modules([optuna_integration.__path__[0]]):
if not modname.startswith("_") and not modname in exclude_modules:
submodule_names.append(modname)
def test_integration_module():
import optuna_integration
import optuna_integration.version
from optuna_integration import version
assert submodule_names != []
@pytest.mark.parametrize("submodule_name", submodule_names)
def test_import_modules(submodule_name: str) -> None:
assert hasattr(optuna_integration, submodule_name)
importlib.import_module(f"optuna_integration.{submodule_name}")
@pytest.mark.parametrize("symbol_name", optuna_integration.__all__)
def test_import_symbols(symbol_name: str) -> None:
assert hasattr(optuna_integration, symbol_name)However, the list in __init__.py must be updated each time a new integration is added. I think we could consider adding this test in CI so that nobody forgets to add it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description of the changes
In this PR, I apply some changes required to migrate the sklearn integration as follows.
optuna_integration.OptunaSearchCVand other modules.optuna_integration, notoptuna.integration(the current reference generation has an inappropriate cycle dependency, i.e., the documents foroptuna_integraitonare generated from the implementations inoptuna.integration).:func:and:class:for components inoptuna.integrationmodules to hyperlinks because references to components of another repository do not work.