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

Fix initial dask job state #357

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

bstadlbauer
Copy link
Member

TL;DR

When the dask plugin creates a DaskJob k8s resource, it will initially not have the .Status.JobStatus property set.

So

status := job.Status.JobStatus

will return an empty string until the DaskOperator sets the status for the first time. While this period is very brief, we've seen it happening a few times.

This PR adds support for the "" state, effectively handling it the same as being queued/initializing.

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

Signed-off-by: Bernhard Stadlbauer <11799671+bstadlbauer@users.noreply.github.com>
@bstadlbauer bstadlbauer requested a review from hamersaw June 8, 2023 07:04
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #357 (6f58551) into master (06866ee) will increase coverage by 1.12%.
The diff coverage is 38.88%.

❗ Current head 6f58551 differs from pull request most recent head 1761c85. Consider uploading reports for the commit 1761c85 to get more accurate results

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   62.82%   63.95%   +1.12%     
==========================================
  Files         148      152       +4     
  Lines       12701    10361    -2340     
==========================================
- Hits         7980     6626    -1354     
+ Misses       4110     3123     -987     
- Partials      611      612       +1     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
go/tasks/pluginmachinery/google/config.go 0.00% <ø> (ø)
...inmachinery/google/default_token_source_factory.go 0.00% <0.00%> (ø)
...sks/pluginmachinery/google/token_source_factory.go 0.00% <0.00%> (ø)
...gke_task_workload_identity_token_source_factory.go 34.00% <34.00%> (ø)
go/tasks/plugins/k8s/dask/dask.go 85.95% <100.00%> (+3.37%) ⬆️
.../plugins/k8s/kfoperators/common/common_operator.go 65.94% <100.00%> (+5.70%) ⬆️
go/tasks/plugins/k8s/kfoperators/mpi/mpi.go 75.00% <100.00%> (+2.42%) ⬆️
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go 78.08% <100.00%> (+2.22%) ⬆️
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go 77.77% <100.00%> (+2.05%) ⬆️

... and 126 files with indirect coverage changes

hamersaw
hamersaw previously approved these changes Jun 8, 2023
@hamersaw
Copy link
Contributor

hamersaw commented Jun 8, 2023

@bstadlbauer looks great, lets merge after the lint fix!

Signed-off-by: Bernhard Stadlbauer <11799671+bstadlbauer@users.noreply.github.com>
@hamersaw hamersaw merged commit 2a7a6f9 into flyteorg:master Jun 8, 2023
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Fix initial dask state

Signed-off-by: Bernhard Stadlbauer <11799671+bstadlbauer@users.noreply.github.com>

* Fix linting

Signed-off-by: Bernhard Stadlbauer <11799671+bstadlbauer@users.noreply.github.com>

---------

Signed-off-by: Bernhard Stadlbauer <11799671+bstadlbauer@users.noreply.github.com>
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.

2 participants