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

Discriminator not respected by JSONEncoder when it works for to_json. #239

Closed
JackUrb opened this issue Jul 24, 2024 · 6 comments
Closed

Comments

@JackUrb
Copy link

JackUrb commented Jul 24, 2024

  • mashumaro version: 3.11
  • Python version: 3.11.9
  • Operating System: Unix

Description

Thanks for the library! It enables some really cool functionality that I've become a fan of.

Unfortunately, as discovered in flyteorg/flyte#5588, it was surprising to find that the behavior differs between the use of mashumaro's JSONEncoder and the to_json method of a class extending DataclassJSONMixin. JSONEncoder does not appear to respect any Discriminator configuration, preventing correct serialization/deserialization when using JSONEncoder for this purpose on classes that use the Discriminator pattern.

Is this to be expected that JSONEncoder (and likely decoder) supports a subset of the features that to_json does?

What I Did

from dataclasses import dataclass
from mashumaro.mixins.json import DataClassJSONMixin
from mashumaro.codecs.json import JSONEncoder, JSONDecoder
from mashumaro.config import BaseConfig
from mashumaro.types import Discriminator

from enum import StrEnum, auto

class SubclassTypes(StrEnum):
    BASE = auto()
    CLASS_A = auto()
    CLASS_B = auto()

@dataclass(kw_only=True)
class BaseClass(DataClassJSONMixin):
    class Config(BaseConfig):
        discriminator = Discriminator(
            field="subclass_type",
            include_subtypes=True,
        )

    subclass_type: SubclassTypes = SubclassTypes.BASE
    base_attribute: int


@dataclass(kw_only=True)
class ClassA(BaseClass):
    subclass_type: SubclassTypes = SubclassTypes.CLASS_A
    class_a_attribute: str


@dataclass(kw_only=True)
class ClassB(BaseClass):
    subclass_type: SubclassTypes = SubclassTypes.CLASS_B
    class_b_attribute: float


if __name__ == "__main__":
    class_a = ClassA(
        base_attribute=1,
        class_a_attribute="a"
    )
    class_b = ClassB(
        base_attribute=2,
        class_b_attribute=2.0
    )
    print("Encoder(a)", JSONEncoder(BaseClass).encode(class_a))
    print("a.to_json()", class_a.to_json())

    print("Encoder(b)", JSONEncoder(BaseClass).encode(class_b))
    print("b.to_json()", class_b.to_json())

I would expect these to be equivalent, but they are not:

Encoder(a) {"subclass_type": "class_a", "base_attribute": 1}
a.to_json() {"subclass_type": "class_a", "base_attribute": 1, "class_a_attribute": "a"}
Encoder(b) {"subclass_type": "class_b", "base_attribute": 2}
b.to_json() {"subclass_type": "class_b", "base_attribute": 2, "class_b_attribute": 2.0}
@Future-Outlier
Copy link

@Fatal1ty, can you take a look?
Thank you so much!

@Fatal1ty
Copy link
Owner

Fatal1ty commented Sep 4, 2024

@Future-Outlier Sorry for taking so long. I'll try to find time soon to dive into the problem. At first glance it looks similar to #225

@Future-Outlier
Copy link

@Future-Outlier Sorry for taking so long. I'll try to find time soon to dive into the problem. At first glance it looks similar to #225

No problem, thank you so much!

@JackUrb
Copy link
Author

JackUrb commented Sep 4, 2024

@Fatal1ty I see how this is related to #225, do you have a pointer to the optimization you made and mention there? Happy to help digging + ideating from there. The example you provided is at least somewhat different as in this case it's possible to tell from BaseClass that it has a discriminator, while your subclass example is not explicit on the subclass relationship, making it much harder to know what to do with.

@Fatal1ty
Copy link
Owner

Fatal1ty commented Sep 8, 2024

@JackUrb Just to be clear, the discriminator is not and was never intended to be used as additional information for serialization (encoding) process. Its only purpose is to create conditions for selecting a class when decoding from raw data, because we have nothing but the data. When you have a dataclass instance, you can tell which dataclass it belongs to and theoretically this information can be useful on encoding. However, this is not happening in your case when a separate JSONEncoder is used because the encoder was built with the BaseClass. The current implementation of encoders uses only the information that is available at the time the encoder is being created. Consider the following scenario:

@dataclass(kw_only=True)
class BaseClass(DataClassJSONMixin):
    class Config(BaseConfig):
        discriminator = Discriminator(
            field="subclass_type",
            include_subtypes=True,
        )

    subclass_type: SubclassTypes = SubclassTypes.BASE
    base_attribute: int

encoder = JSONEncoder(BaseClass)

@dataclass(kw_only=True)
class ClassA(BaseClass):
    subclass_type: SubclassTypes = SubclassTypes.CLASS_A
    class_a_attribute: str

class_a = ClassA(base_attribute=1, class_a_attribute="a")
encoder.encode(class_a)

I moved definition of ClassA after definition of the encoder for the base class. In order to force the encoder to know the fields added to ClassA, it must carry out introspection of the class at runtime, which is not consistent with the main ideology of the library - to compile the code in advance for those structures that are used.

On the other hand, using to_json() method works because, roughly speaking, the DataClassJSONMixin compiles and injects the specific for ClassA code that is used when you call to_json on an instance of that class. This specific code is not used when you use a separate JSONEncoder because you can have multiple encoders with different options for the same dataclass. In case of encoders no code is injected to the dataclasses — everything is stored in the encoder instance itself.

And now we are coming to the point of figuring out what to do in this situation. The good news is that it looks like in your case you intend to combine the mixins and the encoders together. In such a case, you can use the dialect to customize the serialization of the BaseClass base class that was told to the json encoder:

from dataclasses import dataclass

from mashumaro.dialect import Dialect
from mashumaro.mixins.json import DataClassJSONMixin
from mashumaro.codecs.json import JSONEncoder
from mashumaro.config import BaseConfig
from mashumaro.types import Discriminator

from enum import StrEnum, auto


class SubclassTypes(StrEnum):
    BASE = auto()
    CLASS_A = auto()
    CLASS_B = auto()


@dataclass(kw_only=True)
class BaseClass(DataClassJSONMixin):
    class Config(BaseConfig):
        discriminator = Discriminator(
            field="subclass_type",
            include_subtypes=True,
        )

    subclass_type: SubclassTypes = SubclassTypes.BASE
    base_attribute: int


@dataclass(kw_only=True)
class ClassA(BaseClass):
    subclass_type: SubclassTypes = SubclassTypes.CLASS_A
    class_a_attribute: str


@dataclass(kw_only=True)
class ClassB(BaseClass):
    subclass_type: SubclassTypes = SubclassTypes.CLASS_B
    class_b_attribute: float


class BaseClassDialect(Dialect):
    serialization_strategy = {BaseClass: {"serialize": lambda x: x.to_dict()}}


base_class_encoder = JSONEncoder(BaseClass, default_dialect=BaseClassDialect)

class_a = ClassA(base_attribute=1, class_a_attribute="a")
class_b = ClassB(base_attribute=2, class_b_attribute=2.0)

print(class_a.to_json())
print(base_class_encoder.encode(class_a))

print(class_b.to_json())
print(base_class_encoder.encode(class_b))

Please let me know if this works for you.

@JackUrb
Copy link
Author

JackUrb commented Sep 11, 2024

Hi @Fatal1ty, thanks for digging into this and explaining upfront your intent on building these encoders. Your first example captures the issue fairly well, as it'd be somewhat unreasonable to crawl for things that haven't necessarily been defined. Short of a registry pattern inside of the discriminator that explicitly points out the target subclasses (which I think would be somewhat unwieldy), there's no clear way to do this at creation-time of the encoder.

For the use case within flytekit, it's not clear that this ends up being cleaner than just using issubclass(DataClassJSONMixin) and calling to_json rather than using a JSONEncoder in those cases, but I definitely appreciate the context and possible solution.

@JackUrb JackUrb closed this as completed Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants