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

updating dask plugin to use container resources with overrides #351

Merged
merged 4 commits into from
May 24, 2023

Conversation

hamersaw
Copy link
Contributor

TL;DR

This PR updates the Dask plugin to use container resources by default and then merges the task execution overrides and platform overrides if necessary.

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

fixes flyteorg/flyte#3694

Follow-up issue

NA

Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw requested a review from bstadlbauer May 17, 2023 16:03
Signed-off-by: Daniel Rammer <[email protected]>
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #351 (72d1fe8) into master (9a2bbba) will increase coverage by 1.37%.
The diff coverage is 82.60%.

❗ Current head 72d1fe8 differs from pull request most recent head 265599c. Consider uploading reports for the commit 265599c to get more accurate results

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   62.77%   64.15%   +1.37%     
==========================================
  Files         148      148              
  Lines       12664    10286    -2378     
==========================================
- Hits         7950     6599    -1351     
+ Misses       4105     3075    -1030     
- Partials      609      612       +3     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
go/tasks/plugins/k8s/dask/dask.go 85.77% <57.14%> (+1.85%) ⬆️
go/tasks/plugins/k8s/ray/ray.go 77.48% <84.09%> (-0.09%) ⬇️
...tasks/pluginmachinery/flytek8s/container_helper.go 86.36% <88.88%> (+0.36%) ⬆️

... and 128 files with indirect coverage changes

bstadlbauer
bstadlbauer previously approved these changes May 22, 2023
Copy link
Member

@bstadlbauer bstadlbauer left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you so much for fixing this!

go/tasks/plugins/k8s/dask/dask.go Outdated Show resolved Hide resolved
go/tasks/plugins/k8s/dask/dask.go Outdated Show resolved Hide resolved
@bstadlbauer
Copy link
Member

@hamersaw I've also tried this with my local setup and could confirm that it worked 👍

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

@bstadlbauer bstadlbauer left a comment

Choose a reason for hiding this comment

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

LGTM

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.

[BUG] Dask Plugin Resource Assignment Regression
3 participants