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

[WIP] Remove dataclasses json dependency from flytekit V2 #2709

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Aug 26, 2024

I am going to collaborate with @guyarad, an engineer from Nimble Way to finish this.

Tracking issue

flyteorg/flyte#4418

Why are the changes needed?

Reduce container's size.

What changes were proposed in this pull request?

The philosophy of removing dataclasses-json from flytekit but keep ensuring backward compatibility is to try import dataclasses_json except ImportError for all cases.
When we can't use api from dataclasses-json, we can use mashumaro.

  1. inheritance

example: https://github.com/flyteorg/flytekit/pull/2709/files#diff-542ea91d83fb95892fd56588f09967f76665eed2f6cb40afd1fddde776800875R164-R171

(1) get a new class called BaseClass
(2) change every dataclasses with "DataClassJsonMixin" parent class to BaseClass

Before

class ImageConfig(DataClassJsonMixin):

After

try:
    from dataclasses_json import DataClassJsonMixin
    DataClassBaseClass = DataClassJsonMixin
except ImportError:
    DataClassBaseClass = object

class ImageConfig(BaseClass):
  1. command line input (click_types)
    example: fcb49c8#diff-cc3172a3cdf60bb2ffb23b81cb5b92d6c4a89063d691e86932b312e1df09f292R317-R328

  2. from_json and to_json functions

example: fcb49c8#diff-c56afc489e5060224889036253b85ae7c7f1df9f0f7b1130916d9c614293cbb9R963-R968

the from_json and to_json functions are from DataClassJsonMixin
we can should use decoder.decode(json_str) when

if has_attr(from_json):
    return cls.from_json(json_str)
else:
    decoder = JSONDecoder(SerializationSettings)
    return decoder.decode(json_str)
if has_attr(to_json):
    json_str = self.to_json()
else:
    encoder = JSONEncoder(SerializationSettings)
    json_str = encoder.encode(self)
  1. get_literal_type

example: https://github.com/flyteorg/flytekit/pull/2709/files#diff-68624e201f644896b015de1a5716eabef9b8b5a106caba1d9f21c51c0708684bR442-R443

from marshmallow_jsonschema import JSONSchema
from mashumaro.jsonschema import build_json_schema

  1. dataclass_json()
    example: https://github.com/flyteorg/flytekit/pull/2709/files#diff-68624e201f644896b015de1a5716eabef9b8b5a106caba1d9f21c51c0708684bR1849-R1853

  2. for unit tests, we should use pytest skip if dataclasses_json is not in the dependency

  3. add CI to test cases when we remove dataclass-json

  4. add warning logs when dataclasses-json is needed

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

#2557

Docs link

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 19.04762% with 17 lines in your changes missing coverage. Please review.

Project coverage is 41.22%. Comparing base (54f0a46) to head (fcb49c8).

Files Patch % Lines
flytekit/interaction/click_types.py 0.00% 9 Missing ⚠️
flytekit/remote/remote.py 0.00% 6 Missing ⚠️
flytekit/configuration/__init__.py 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2709      +/-   ##
==========================================
+ Coverage   39.16%   41.22%   +2.05%     
==========================================
  Files         187      187              
  Lines       19236    19250      +14     
  Branches     4019     4020       +1     
==========================================
+ Hits         7533     7935     +402     
+ Misses      11470    11186     -284     
+ Partials      233      129     -104     

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

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
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.

1 participant