Skip to content

[BUG] Fix _get_class_flags dropping Mixin flags under multiple inheritance#540

Merged
fkiraly merged 7 commits intosktime:mainfrom
vedant21-oss:fix/539-mro-slice-drops-mixin-flags
Apr 19, 2026
Merged

[BUG] Fix _get_class_flags dropping Mixin flags under multiple inheritance#540
fkiraly merged 7 commits intosktime:mainfrom
vedant21-oss:fix/539-mro-slice-drops-mixin-flags

Conversation

@vedant21-oss
Copy link
Copy Markdown
Contributor

Summary

Fixes #539 - _FlagManager._get_class_flags dropped flags from user-defined Mixin classes under multiple inheritance due to a hardcoded [:-2] MRO slice.

Root Cause

The previous code assumed the last two MRO entries are always BaseObject and object:

for parent_class in reversed(inspect.getmro(cls)[:-2]):

Under Python's C3 linearisation, class MyObj(BaseObject, MyMixin) produces:

MyObj -> BaseObject -> _FlagManager -> MyMixin -> object

MyMixin sits at position [-2], so the slice permanently drops it and all its flags - silently, with no error.

Fix

Iterate the full MRO and skip only object via an explicit identity check:

for parent_class in reversed(inspect.getmro(cls)):
    if parent_class is object:
        continue

object is always the last entry and can never carry user-defined flags, making it the only safe sentinel to exclude.

Changes

  • skbase/base/_tagmanager.py: Replace [:-2] slice with if parent_class is object: continue. Add detailed comment explaining the root cause so this is never accidentally reverted.
    • skbase/tests/test_meta.py: Add missing _FlagManager import + add test_get_class_flags_preserves_mixin_tags regression test that pins this fix and will fail loudly if the slice returns.

Testing

11 passed in 0.02s

vedant21-oss and others added 2 commits April 4, 2026 20:27
…tance (sktime#539)

The MRO iteration in _FlagManager._get_class_flags previously used:

    inspect.getmro(cls)[:-2]

This assumed the last two MRO entries are always 'BaseObject' and 'object'.
That assumption is broken under multiple inheritance.  Python's C3
linearisation can place a user-defined Mixin class immediately before
'object', making it the second-to-last entry.  The [:-2] slice then
silently drops that Mixin and all flags it defines.

Minimal reproduction:

    class MyMixin:
        _flags = {'mixin_tag': 42}

    class MyObj(BaseObject, MyMixin):
        _flags = {'own_tag': 1}

    # MRO: MyObj -> BaseObject -> _FlagManager -> MyMixin -> object
    # [:-2] produces: MyObj -> BaseObject -> _FlagManager
    # MyMixin (and 'mixin_tag') is permanently dropped.

Fix: iterate the full MRO and skip only 'object' via an explicit identity
check.  'object' is the only class that can never carry user-defined flags,
so it is the only safe sentinel to exclude.

Changes:
- skbase/base/_tagmanager.py: replace [:-2] slice with explicit
  'if parent_class is object: continue' guard; add a detailed comment
  explaining the root cause so the fix is not accidentally reverted.
- skbase/tests/test_meta.py: add missing _FlagManager import (needed by
  the existing MRO assertion) and add test_get_class_flags_preserves_mixin_tags
  as a regression test that pins the fix and will loudly fail if the
  [:-2] slice is reintroduced.
# Slicing with [:-2] drops `MyMixin`, permanently losing any flags it
# defines. The only safe sentinel is `object` itself (which never carries
# user-defined flags), so we skip it explicitly and iterate the rest.
for parent_class in reversed(inspect.getmro(cls)):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we want to make this general, do we not want to drop it entirely?

Also, please do not add comments that explain the previous situation, they do not make sense to a reader of the code base. AIs often add comments that reference a change.

@fkiraly fkiraly added enhancement Adding new functionality AI overuse suspected labels Apr 12, 2026
vedant21-oss and others added 3 commits April 13, 2026 02:00
- Removed the detailed historical inline comment from the MRO fix in `_get_class_flags`
- Added `test_class_tags_match_mro` to `TestAllObjects` to verify the MRO
  invariant holds for all dynamic estimator lookups, preventing future regressions.
- Simplified the standalone bug reproduction test docstring.
@fkiraly fkiraly changed the title [BUG] Fix _get_class_flags dropping Mixin flags under multiple inheritance (#539) [BUG] Fix _get_class_flags dropping Mixin flags under multiple inheritance Apr 19, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 19, 2026

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.03%. Comparing base (306958d) to head (559cc36).
⚠️ Report is 197 commits behind head on main.

Files with missing lines Patch % Lines
skbase/testing/test_all_objects.py 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   85.07%   84.03%   -1.04%     
==========================================
  Files          45       52       +7     
  Lines        3015     3934     +919     
==========================================
+ Hits         2565     3306     +741     
- Misses        450      628     +178     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

I fixed it.

@fkiraly fkiraly merged commit 1ae228a into sktime:main Apr 19, 2026
26 checks passed
@vedant21-oss vedant21-oss deleted the fix/539-mro-slice-drops-mixin-flags branch April 19, 2026 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI overuse suspected enhancement Adding new functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] _get_class_flags dynamically slices the MRO index, permanently dropping tags for Mixins during multiple inheritance

2 participants