Skip to content

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>
@welcome
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
@codecov
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
@welcome
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