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

[Docs] Use Pure Dataclass In Example #5829

Merged

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Oct 9, 2024

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

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.35%. Comparing base (d062824) to head (389b097).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5829   +/-   ##
=======================================
  Coverage   36.35%   36.35%           
=======================================
  Files        1304     1304           
  Lines      110148   110148           
=======================================
+ Hits        40042    40043    +1     
+ Misses      65939    65938    -1     
  Partials     4167     4167           
Flag Coverage Δ
unittests-datacatalog 51.37% <ø> (ø)
unittests-flyteadmin 55.60% <ø> (ø)
unittests-flytecopilot 12.17% <ø> (ø)
unittests-flytectl 62.21% <ø> (ø)
unittests-flyteidl 7.17% <ø> (ø)
unittests-flyteplugins 53.35% <ø> (ø)
unittests-flytepropeller 42.02% <ø> (ø)
unittests-flytestdlib 55.37% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Future-Outlier Future-Outlier enabled auto-merge (squash) October 9, 2024 15:13
Copy link
Contributor

@neverett neverett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few questions and suggestions -- let me know what you think!

docs/user_guide/data_types_and_io/dataclass.md Outdated Show resolved Hide resolved
`from dataclasses_json import dataclass_json` instead of inheriting from Mashumaro's `DataClassJSONMixin`.

If you're using Flytekit version >= v1.11.1, you don't need to decorate with `@dataclass_json` or
If you're using Flytekit version below v1.11.1, your dataclass will need to decorate with `@dataclass_json` or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, this means that if you are using Flytekit < 1.11.1, you either need to decorate your dataclass with @dataclass_json or create a class that inherits DataClassJSONMixin, but that you don't need to do both? If you inherit from DataClassJSONMixin, do you need to do from mashumaro.mixins.json import DataClassJSONMixin as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

docs/user_guide/data_types_and_io/dataclass.md Outdated Show resolved Hide resolved
docs/user_guide/data_types_and_io/dataclass.md Outdated Show resolved Hide resolved
Future-Outlier and others added 3 commits October 9, 2024 23:50
Co-authored-by: Nikki Everett <neverett@users.noreply.github.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Co-authored-by: Nikki Everett <neverett@users.noreply.github.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Co-authored-by: Nikki Everett <neverett@users.noreply.github.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
`from dataclasses_json import dataclass_json` instead of inheriting from Mashumaro's `DataClassJSONMixin`.

If you're using Flytekit version >= v1.11.1, you don't need to decorate with `@dataclass_json` or
If you're using Flytekit version below v1.11.1, your dataclass will need to decorate with `@dataclass_json` or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you're using Flytekit version below v1.11.1, your dataclass will need to decorate with `@dataclass_json` or
If you're using Flytekit version greater than v1.10 and less than v1.11.1, you will need add `from mashumaro.mixins.json import DataClassJSONMixin` to your imports and either decorate your dataclass with `@dataclass_json` or create a class that inherits from `DataClassJSONMixin`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this is accurate!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not, but I can update it

Signed-off-by: Future-Outlier <eric901201@gmail.com>
neverett
neverett previously approved these changes Oct 9, 2024
Co-authored-by: Nikki Everett <neverett@users.noreply.github.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
@Future-Outlier Future-Outlier merged commit da2ce74 into master Oct 9, 2024
50 checks passed
@Future-Outlier Future-Outlier deleted the remove-decorator-and-base-class-from-dataclass branch October 9, 2024 17:09
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