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

Walk inheritance hierarchy before checking is_dataclass in type_engine #1239

Merged

Conversation

rahul-theorem
Copy link
Contributor

TL;DR

Swap steps 3 & 4 in type_engine.py to walk the inheritance hierarchy to check for a registered TypeTransformer before checking is_dataclass

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

We have internal tooling which generates dataclasses from OpenAPI specifications, which all include a Generated mixin and are guaranteed to have a to_dict/from_dict method. We'd like to register a singletype transformer for all classes which have the Generated mixin instead of taking an extra dependency on dataclass-json; however, this isn't possible with the present implementation since the type engine checks is_dataclass before walking the inheritance hierarchy to resolve the type transformer.

This PR swaps this behavior so we can successfully detect a type transformer for Generated rather than falling back to the default dataclass serialization.

See https://flyte-org.slack.com/archives/CREL4QVAQ/p1666049599823129 for more details/discussion

Tracking Issue

N/A

Follow-up issue

N/A

@rahul-theorem rahul-theorem force-pushed the rahul/type-transformer-ordering branch from 8b70787 to c760b39 Compare October 17, 2022 23:54
@rahul-theorem rahul-theorem marked this pull request as ready for review October 17, 2022 23:54
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #1239 (c760b39) into master (ab9aa65) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1239   +/-   ##
=======================================
  Coverage   68.68%   68.68%           
=======================================
  Files         288      288           
  Lines       26323    26323           
  Branches     2939     2939           
=======================================
  Hits        18079    18079           
  Misses       7766     7766           
  Partials      478      478           
Impacted Files Coverage Δ
flytekit/core/type_engine.py 59.38% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@eapolinario eapolinario merged commit b7ecdf6 into flyteorg:master Oct 18, 2022
VPraharsha03 pushed a commit to VPraharsha03/flytekit that referenced this pull request Oct 29, 2022
flyteorg#1239)

Signed-off-by: Rahul Mehta <[email protected]>

Signed-off-by: Rahul Mehta <[email protected]>
Signed-off-by: Vivek Praharsha <[email protected]>
kiliangojek pushed a commit to kiliangojek/flytekit that referenced this pull request Nov 15, 2022
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

Successfully merging this pull request may close these issues.

2 participants