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

feat(resources): support serialization of Resources dataclasses #2250

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

cameronraysmith
Copy link
Contributor

@cameronraysmith cameronraysmith commented Mar 8, 2024

Why are the changes needed?

If one would like to pass Resources instances as configuration to workflows, they need to be serializable.

What changes were proposed in this pull request?

Following #1735, declare Resources and ResourceSpec to inherit from DataClassJSONMixin

How was this patch tested?

  • Unit tests that (de)serialize Resources instances are added.
  • Preliminary successful CI execution on fork.

Check all the applicable boxes

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

Related PRs

- support serialization of Resources and ResourceSpec
- allow passing these as configuration to workflows or between tasks
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>
Copy link

welcome bot commented Mar 8, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@cameronraysmith cameronraysmith marked this pull request as ready for review March 8, 2024 20:02
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Mar 8, 2024
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.52%. Comparing base (64b8468) to head (07f8788).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
- Coverage   86.01%   83.52%   -2.49%     
==========================================
  Files         322      296      -26     
  Lines       24376    23205    -1171     
  Branches     3689     3477     -212     
==========================================
- Hits        20966    19383    -1583     
- Misses       2754     3201     +447     
+ Partials      656      621      -35     

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

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 10, 2024
@pingsutw pingsutw merged commit 998589f into flyteorg:master Mar 10, 2024
43 of 44 checks passed
Copy link

welcome bot commented Mar 10, 2024

Congrats on merging your first pull request! 🎉

@cameronraysmith cameronraysmith deleted the serialize-resources branch March 10, 2024 15:19
austin362667 pushed a commit to austin362667/flytekit that referenced this pull request Mar 16, 2024
…eorg#2250)

* feat(resources): inherit from from DataClassJSONMixin

- support serialization of Resources and ResourceSpec
- allow passing these as configuration to workflows or between tasks
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>

* test(resources): add tests to (de)serialize Resources instances
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
* feat(resources): inherit from from DataClassJSONMixin

- support serialization of Resources and ResourceSpec
- allow passing these as configuration to workflows or between tasks
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>

* test(resources): add tests to (de)serialize Resources instances
Signed-off-by: Cameron Smith <cameron.ray.smith@gmail.com>
Signed-off-by: Jan Fiedler <jan@union.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants