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

Generic dataclass detection fails for unions #7

Closed
pculbertson opened this issue Sep 1, 2022 · 5 comments
Closed

Generic dataclass detection fails for unions #7

pculbertson opened this issue Sep 1, 2022 · 5 comments

Comments

@pculbertson
Copy link

Hi Brent,

I have a very low-level bug to flag for you -- when saving/loading nested dataclasses to yaml (using extras.to_yaml(), extras.from_yaml(), if a dataclass has a Union of two custom types, they don't get detected as custom types for the yaml.Loader to construct.

I wrote a MWE to replicate the issue:

import dataclasses
import dcargs

from typing import Union

@dataclasses.dataclass
class TypeA:
    data: int

@dataclasses.dataclass
class TypeB:
    data: int
    
@dataclasses.dataclass
class Wrapper:
    subclass: Union[TypeA, TypeB] = TypeA(1)
    
if __name__ == "__main__":
    wrapper1 = Wrapper() # Create Wrapper object.
    wrapper2 = dcargs.extras.from_yaml(Wrapper, dcargs.extras.to_yaml(wrapper1)) # Errors, no constructor for TypeA

No worries if this is too low-level to deal with right now -- I think we can work around it by just pickling the configs, but wanted to flag something is going awry in the custom type detection.

@brentyi brentyi closed this as completed in 40e01c7 Sep 1, 2022
@pculbertson
Copy link
Author

Wow thanks so much for the lightning-quick patch :)

@brentyi
Copy link
Owner

brentyi commented Sep 1, 2022

oops, seems it was too fast and the CI is broken :)

looking into it...

@brentyi brentyi reopened this Sep 1, 2022
@brentyi
Copy link
Owner

brentyi commented Sep 1, 2022

Actually, everything seems to be in order; just had a typo in a test + an outdated version of typing_extensions locally.

Appreciate the issue report + reproduction instructions! As an FYI I am thinking of deprecating the API though, I can think of a couple other issues and the benefit over just using PyYAML seems fairly incremental. Will file a separate issue.

@brentyi brentyi closed this as completed Sep 1, 2022
@pculbertson
Copy link
Author

Thanks, appreciate the heads-up! I wanted to file an issue in case the Union typing issue went further than just the YAML utils. Before your patch, we were just pickling the dataclass objects with no problem, we might just stick with that.

@brentyi brentyi mentioned this issue Sep 1, 2022
2 tasks
@brentyi
Copy link
Owner

brentyi commented Sep 1, 2022

Sounds good!

If you haven't considered it, I'd recommend also looking into serializing with PyYAML + the standard yaml.load(your_yaml, Loader=yaml.Loader) and yaml.dump(your_dataclass_instance) helpers. It should be basically as flexible as pickle, bigger file sizes but still human-readable + editable.

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

2 participants