Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix resources requests for AWS Batch plugin #400

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Sep 5, 2023

TL;DR

This PR uses the container resources as the base before merging with resource overrides and platform defaults. The logic involved in this PR updated always setting the resource overrides even if there were none (if missing it would set overrides to the container resources), which introduced this regression.

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

^^^

Tracking Issue

flyteorg/flyte#4003

Follow-up issue

NA

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: +1.33% 🎉

Comparison is base (552f145) 62.83% compared to head (34b080c) 64.17%.

❗ Current head 34b080c differs from pull request most recent head 04e363b. Consider uploading reports for the commit 04e363b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   62.83%   64.17%   +1.33%     
==========================================
  Files         156      156              
  Lines       13116    10647    -2469     
==========================================
- Hits         8242     6833    -1409     
+ Misses       4253     3192    -1061     
- Partials      621      622       +1     
Flag Coverage Δ
unittests ?

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

Files Changed Coverage Δ
go/tasks/plugins/array/awsbatch/transformer.go 81.45% <60.00%> (+2.33%) ⬆️

... and 135 files with indirect coverage changes

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

Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Does flyteorg/flytepropeller#544 also affect other plugins?

@hamersaw
Copy link
Contributor Author

hamersaw commented Sep 7, 2023

Does flyteorg/flytepropeller#544 also affect other plugins?

Good question, as far as I can tell no. Other plugins use the container parsing we define. AWS Batch is unique because it uses the resources to populate a different object (ie. aws batch job).

@hamersaw hamersaw merged commit c978638 into master Sep 7, 2023
6 of 7 checks passed
@hamersaw hamersaw deleted the bug/aws-batch-resources branch September 7, 2023 14:25
eapolinario pushed a commit that referenced this pull request Sep 28, 2023
* retrieving resources from container before merging with overrides and defaults

Signed-off-by: Daniel Rammer <[email protected]>

* fixed linter

Signed-off-by: Daniel Rammer <[email protected]>

---------

Signed-off-by: Daniel Rammer <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants