Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isinstance(ConfigDict(...), collections.abc.Mapping) returns False #47

Open
DrPlantabyte opened this issue Feb 10, 2025 · 3 comments
Open
Labels
bug Something isn't working

Comments

@DrPlantabyte
Copy link

DrPlantabyte commented Feb 10, 2025

Describe the bug
isinstance(config_dict_obj, collections.abc.Mapping) returns False even though it is a "dictionary" type

This is because class ml_collections.config_dict.ConfigDict does not inherit Python built-in abstract base class collections.abc.Mapping, despite being a "dictionary" type, producing unexpected behavior when using reflection or duck-typing with isinstance(x, collections.abc.Mapping). This is especially problematic for serializing and logging code where it is common to apply different formatting to different collection types.

See PR #46 for proposed solution.

To Reproduce
The following code snippet reproduces and illustrates the bug

import collections.abc
from typing import Any
from ml_collections import config_dict


def safe_print(obj: Any):
    """Print object with masked values to avoid leaking API keys in the logs"""
    if isinstance(obj, collections.abc.Mapping):
        for k, v in obj.items():
            print(f"{k}={'*' * len(str(v))}")
    elif isinstance(obj, collections.abc.Sequence):
        print(f"[ ... ({len(obj)} items)]")
    else:
        raise ValueError(f"Unsupported type: {type(obj)}")


safe_print({"a": 1})
>>> a=*

conf = config_dict.ConfigDict({"a": 1})
safe_print(conf)
>>> Traceback (most recent call last):
>>>   File "<stdin>", line 1, in <module>
>>>   File "<stdin>", line 8, in safe_print
>>> ValueError: Unsupported type: <class 'ml_collections.config_dict.config_dict.ConfigDict'>

Expected behavior
isinstance(config_dict_obj, collections.abc.Mapping) should return True

Environment:

  • OS: Linux (Pop_OS/Ubuntu)
  • OS Version: 6.9.3/22.04
  • Python: Python 3.10

Additional context
Correctly inheriting collections.abc.Mapping would also improve Python static type analysis tooling (eg Ruff)

@Conchylicultor
Copy link
Member

Rather than inheritance which might accidentally change the runtime behavior, using Mapping.register(ConfigDict) would make isinstance(xx, Mapping) returns True without changing the inheritance.

@DrPlantabyte
Copy link
Author

Inheriting the built-in ABC class looks fairly benign (see https://github.com/python/cpython/blob/b8b4b713c5f8ec0958c7ef8d29d6711889bc94ab/Lib/_collections_abc.py#L804 and https://github.com/python/cpython/blob/b8b4b713c5f8ec0958c7ef8d29d6711889bc94ab/Lib/_collections_abc.py#L936 ), but I'd be happy enough just to have Mapping.register(ConfigDict)

@DrPlantabyte
Copy link
Author

Rather than inheritance which might accidentally change the runtime behavior, using Mapping.register(ConfigDict) would make isinstance(xx, Mapping) returns True without changing the inheritance.

Please consider this PR that does exactly that: #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants