Dynamically handle future scikit-learn metadata routing deprecation#22723
Dynamically handle future scikit-learn metadata routing deprecation#22723hertschuh merged 3 commits intokeras-team:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the _routing_enabled utility to handle missing sklearn installations and introduces version-based defaults for metadata routing, specifically enabling it for scikit-learn 1.3+. The review feedback highlights that the docstring is now outdated regarding default values and suggests a more concise implementation for the configuration and version checks.
| Whether metadata routing is enabled. If the config is not set, it | ||
| defaults to False. |
There was a problem hiding this comment.
| if "enable_metadata_routing" in config: | ||
| return config.get("enable_metadata_routing", False) | ||
|
|
||
| try: | ||
| from packaging.version import parse as parse_version | ||
|
|
||
| if parse_version(sklearn.__version__) >= parse_version("1.3"): | ||
| return True | ||
| except ImportError: | ||
| pass | ||
|
|
||
| return False |
There was a problem hiding this comment.
The implementation can be simplified. Since you've already checked if "enable_metadata_routing" is in the config, using .get(..., False) is redundant. The version check can also be more concise while catching potential parsing errors for robustness.
| if "enable_metadata_routing" in config: | |
| return config.get("enable_metadata_routing", False) | |
| try: | |
| from packaging.version import parse as parse_version | |
| if parse_version(sklearn.__version__) >= parse_version("1.3"): | |
| return True | |
| except ImportError: | |
| pass | |
| return False | |
| if "enable_metadata_routing" in config: | |
| return config["enable_metadata_routing"] | |
| try: | |
| from packaging.version import parse | |
| return parse(sklearn.__version__) >= parse("1.3") | |
| except Exception: | |
| return False |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22723 +/- ##
==========================================
- Coverage 83.15% 83.04% -0.12%
==========================================
Files 596 596
Lines 68858 69889 +1031
Branches 10774 10894 +120
==========================================
+ Hits 57259 58037 +778
- Misses 8783 9004 +221
- Partials 2816 2848 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @hertschuh this pr has passes all the tests and is ready for review |
| if parse_version(sklearn.__version__) >= parse_version("1.3"): | ||
| return True |
There was a problem hiding this comment.
Why would it be on all the time for sklearn >= 1.3 ?
hertschuh
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
|
Hi @hertschuh i believe that meta data routing wasnt available until version 1.3 |
If that's the case, shouldn't the logic be this? if parse_version(sklearn.__version__) < parse_version("1.3"):
return False
config = sklearn.get_config()
if "enable_metadata_routing" in config:
return config.get("enable_metadata_routing", False)
return False |
If we use that logic and a future version of scikit-learn (e.g., version 2.0) completely removes the enable_metadata_routing config key, your suggested logic would fall through to the final return False. |
|
| from packaging.version import parse as parse_version | ||
|
|
There was a problem hiding this comment.
Move import to the top and remove try / except
packaging is a required dependency of the pypi wheel.
Oh, I see. I misunderstood what the deprecation was about. |
|
Hi @hertschuh I have done the changes this pr is ready for review |
Description
This PR resolves a code health TODO by future-proofing the scikit-learn metadata routing check in keras/src/wrappers/fixes.py. It dynamically falls back to checking sklearn.version if the config key is removed, ensuring metadata routing correctly defaults to enabled in future scikit-learn versions while preserving backward compatibility.
Contributor Agreement
Please check all boxes below before submitting your PR for review: