fix(config): keep QlibConfig.registered safe in joblib worker subprocesses#2212
Open
genisis0x wants to merge 1 commit into
Open
fix(config): keep QlibConfig.registered safe in joblib worker subprocesses#2212genisis0x wants to merge 1 commit into
genisis0x wants to merge 1 commit into
Conversation
…esses
QlibConfig.registered is implemented as a property that reads
self._registered. Because the overridden __setattr__ routes attribute
writes through self.__dict__["_config"], "_registered" lives inside
self._config rather than on the instance __dict__. If self._config has
been rebuilt from _default_config (which does not contain "_registered")
-- for example in a freshly spawned joblib worker that re-imports qlib,
or after Config.reset() inside QlibConfig.__init__ before QlibConfig sets
self._registered -- the property's inner self._registered access raises
AttributeError, Python falls back to Config.__getattr__ for the original
name ("registered"), and the worker crashes with:
AttributeError: No such ``registered`` in self._config
This terminates backtests that spawn joblib workers along the
PortfolioMetrics._cal_benchmark -> get_higher_eq_freq_feature ->
D.features -> ParallelExt -> inst_calculator -> register_from_C path
(issue microsoft#2038).
Make the property defensive: read "_registered" out of
self.__dict__["_config"] with a safe ``get`` chain so a missing key
returns False instead of triggering the property -> __getattr__ cascade.
Also harden register_from_C with ``getattr(C, "registered", False)`` so
the second top-level caller is double-protected.
Add regression tests for:
- registered returns False when "_registered" key is absent from _config,
- registered returns False on a partially-initialised QlibConfig
(mirrors unpickle / fresh-import state seen in workers),
- ``getattr(C, "registered", False)`` does not raise when the key is
missing on the global C,
- registered returns the assigned value on the happy path so the fix
does not regress normal behaviour.
Fixes microsoft#2038
Author
|
Read through the CLA — all good. @microsoft-github-policy-service agree |
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
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.
Summary
Fixes Backtest/joblib worker crash: AttributeError: No such registered in self._config #2038.
QlibConfig.registeredis implemented as a@propertythat returnsself._registered, but because the overridden__setattr__routes attribute writes throughself.__dict__["_config"], the_registeredflag actually lives inside the_configdict. If_configis rebuilt from_default_configwithout the_registeredkey (e.g. inside a freshly-spawned joblib worker that re-importsqlib, or transiently insideConfig.reset()beforeQlibConfig.__init__re-sets the flag), the property's innerself._registeredaccess raisesAttributeError, Python falls back toConfig.__getattr__for the original attribute nameregistered, and the worker dies with:This terminates backtests along
PortfolioMetrics._cal_benchmark->get_higher_eq_freq_feature->D.features->ParallelExt->inst_calculator->register_from_C.Make the property defensive: read
_registeredout ofself.__dict__["_config"]via a safe.get()chain so a missing key returnsFalseinstead of triggering the property ->__getattr__cascade.Harden
register_from_Cwithgetattr(C, "registered", False)so the second top-level caller (qlib/config.py:115) is double-protected against the same failure mode.Validation
uv run pytest tests/misc/test_config_registered.py-- 4 passed.AttributeError: No such ``registered`` in self._config) ontest_registered_property_returns_false_when_key_missingandtest_registered_property_returns_false_when_config_missing; reapplying the patch turns both green.uv run black qlib/config.py tests/misc/test_config_registered.py -l 120 --check --diff-- clean.uv run flake8 --ignore=E501,F541,E266,E402,W503,E731,E203 qlib/config.py-- clean.Tests added
test_registered_property_returns_false_when_key_missing-- worker scenario where_configlacks the_registeredkey.test_registered_property_returns_false_when_config_missing-- partially-constructed instance (e.g. mid-unpickle) where even_configis absent.test_register_from_C_does_not_raise_when_registered_missing-- exact guarded boolean from the original bug report does not raise on the globalCwhen the key is removed.test_registered_property_after_normal_construction-- happy path still reflects the assigned value.Fixes #2038