tensorflow plugin implementation #103

pingsutw · 2020-07-11T17:23:04Z

TL;DR

TensorFlow plugin for Flyte that leverages https://github.com/kubeflow/tf-operator to run distributed tensorflow jobs on k8s.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

related to flyteorg/flyteidl#71

Tracking Issue

flyteorg/flyte#115

Follow-up issue

NA

pingsutw · 2020-07-11T17:50:02Z

The test failure seems not related to PR.

kumare3 · 2020-07-11T22:25:24Z

I will take a look at the test

pingsutw · 2020-07-12T17:44:52Z

Thanks :)

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow_test.go

swiftdiaries · 2020-08-03T11:38:07Z

Otherwise, LGTM :)

codecov-commenter · 2020-08-13T09:12:41Z

Codecov Report

Merging #103 into master will increase coverage by 0.46%.
The diff coverage is 79.13%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   58.70%   59.17%   +0.46%     
==========================================
  Files          99      100       +1     
  Lines        5936     6075     +139     
==========================================
+ Hits         3485     3595     +110     
- Misses       2103     2122      +19     
- Partials      348      358      +10

Impacted Files	Coverage Δ
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go	`79.13% <79.13%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc4e8b5...c6f7763. Read the comment docs.

pingsutw · 2020-08-13T12:39:24Z

@swiftdiaries sorry for the late reply.
I've updated and rebased the PR

swiftdiaries · 2020-08-13T14:51:35Z

LGTM
I don't have write access to this repo, I think @kumare3 might want to do a round of reviews before merging it

swiftdiaries · 2020-08-13T14:52:08Z

This is really cool 😬 Great to see this come along

kumare3 · 2020-08-13T15:42:00Z

Hey I thought this was merged. One check seems
To be failing?

pingsutw · 2020-08-14T05:51:17Z

Test failure seems unrelated.
btw, I'm going to write an example in flytekit.

kumare3 · 2020-08-14T20:20:24Z

Cc @wild-endeavor @EngHabu @katrogan

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow.go

kumare3 · 2020-07-11T22:18:28Z

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow.go

+		return nil, flyteerr.Errorf(flyteerr.BadTaskSpecification, "Unable to create pod spec: [%v]", err.Error())
+	}
+
+	overrideDefaultContainerName(taskCtx, podSpec)


Do you need to do this?

Tensorflow operator will also force pod to have a container named tensorflow, I've updated the comment.

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow.go

kumare3 · 2020-08-18T23:47:35Z

hmm ya the test is not related, it is interesting to me that your branch has this error, but master does not?

pingsutw · 2020-08-20T09:43:22Z

I've moved some common code between PyTorch and TensorFlow to common_opeartor

pingsutw · 2020-08-20T09:44:49Z

hmm ya the test is not related, it is interesting to me that your branch has this error, but master does not?

I got the same error in master
https://github.com/pingsutw/flyteplugins/actions/runs/215382594

kumare3 · 2020-08-20T13:51:26Z

Let me review today and we can merge soon

kumare3

LGTM

kumare3 · 2020-09-14T16:50:42Z

Some files are not go-imported. Once they are done, the linter should be green and let us merge it afterwards. I do not have permissions to your fork.

codecov-io · 2021-02-04T08:51:58Z

Codecov Report

Merging #103 (91cc3c8) into master (79bcea8) will decrease coverage by 1.97%.
The diff coverage is 46.55%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   61.98%   60.01%   -1.98%     
==========================================
  Files         109      129      +20     
  Lines        6256     6955     +699     
==========================================
+ Hits         3878     4174     +296     
- Misses       1984     2361     +377     
- Partials      394      420      +26

Flag	Coverage Δ
unittests	`60.01% <46.55%> (?)`

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

Impacted Files	Coverage Δ
go/tasks/aws/config.go	`9.09% <0.00%> (-40.91%)`	⬇️
go/tasks/aws/config_flags.go	`29.41% <ø> (-7.44%)`	⬇️
...ks/pluginmachinery/core/allocationstatus_enumer.go	`0.00% <0.00%> (ø)`
go/tasks/pluginmachinery/core/resource_manager.go	`0.00% <ø> (ø)`
...o/tasks/pluginmachinery/internal/webapi/monitor.go	`0.00% <0.00%> (ø)`
go/tasks/plugins/hive/executor.go	`10.29% <0.00%> (ø)`
go/tasks/plugins/k8s/sagemaker/custom_training.go	`69.60% <0.00%> (ø)`
...sks/plugins/k8s/sagemaker/hyperparameter_tuning.go	`61.49% <0.00%> (ø)`
go/tasks/plugins/webapi/athena/config.go	`0.00% <0.00%> (ø)`
go/tasks/plugins/webapi/athena/plugin.go	`3.00% <3.00%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a49ee2...3518953. Read the comment docs.

pingsutw · 2021-02-04T08:58:14Z

cc @EngHabu @swiftdiaries @kumare3 Can you help me review this, Thanks.
I've fixed the conflict and the PR tests and lint already passed.

However, I don't why Build Co-Pilot Docker image failed.
Do you know how to fix it?

Removing intermediate container b9a18bc4bc63
 ---> 943a353f299a
Successfully built 943a353f299a
Successfully tagged my_awesome_image:latest
+ set +x

[Action Step] Tagging image...
Tag: ghcr.io/flyteorg/flytecopilot:1a52a8f80c80711ab2175ce3b58db715ad13af59

[Action Step] Pushing image...
ERROR: Can't push when not logged in to registry. Set "push_image_and_stages: false" if you don't want to push

EngHabu

Awesome work, @pingsutw. Just have a few comments and we can merge!

go/tasks/plugins/k8s/kfoperators/common/common_operator.go

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow.go

pingsutw · 2021-02-04T21:33:22Z

@EngHabu Thanks for your review. I will update ASAP.

Co-authored-by: Haytham Abuelfutuh <[email protected]>

pingsutw · 2021-02-12T07:01:07Z

@EngHabu and @kumare3 I've updated it. I think it's ready to merge.

kumare3 · 2021-02-12T07:47:24Z

I think I am +1 on the issue, will wait for @EngHabu to approve.
On the other hand @pingsutw we also need to add an example here - https://github.com/flyteorg/flytesnacks/tree/push-images/cookbook/plugins

go/tasks/plugins/k8s/kfoperators/common/common_operator.go

EngHabu · 2021-02-12T15:39:55Z

Let's address this: #103 (comment) then let's get it merged!

pingsutw · 2021-02-12T15:44:28Z

Let's address this: #103 (comment) then let's get it merged!

okay, I'm working on it.

* Tensorflow plugin implementaion * Fix checkstyle * Update go/tasks/plugins/k8s/kfoperators/common/common_operator.go Co-authored-by: Haytham Abuelfutuh <[email protected]> * Address comments * Fix lint error * Address comment * Fix lint * Update to running Co-authored-by: Haytham Abuelfutuh <[email protected]> Signed-off-by: Katrina Rogan <[email protected]>

* Tensorflow plugin implementaion * Fix checkstyle * Update go/tasks/plugins/k8s/kfoperators/common/common_operator.go Co-authored-by: Haytham Abuelfutuh <[email protected]> * Address comments * Fix lint error * Address comment * Fix lint * Update to running Co-authored-by: Haytham Abuelfutuh <[email protected]>

pingsutw mentioned this pull request Jul 22, 2020

tensorflow plugin implementation flyteorg/flytekit#141

Merged

8 tasks

swiftdiaries reviewed Aug 3, 2020

View reviewed changes

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow_test.go Show resolved Hide resolved

swiftdiaries reviewed Aug 3, 2020

View reviewed changes

go/tasks/plugins/k8s/kfoperators/tensorflow/tensorflow_test.go Show resolved Hide resolved

pingsutw force-pushed the tf-plugin branch from 06827e3 to c4cc876 Compare August 12, 2020 10:11

swiftdiaries previously approved these changes Aug 13, 2020

View reviewed changes

kumare3 reviewed Aug 18, 2020

View reviewed changes

pingsutw dismissed swiftdiaries’s stale review via 8372136 August 20, 2020 09:40

kumare3 self-requested a review September 14, 2020 16:48

kumare3 previously approved these changes Sep 14, 2020

View reviewed changes

pingsutw dismissed kumare3’s stale review via 2e21611 September 17, 2020 05:09

pingsutw force-pushed the tf-plugin branch from 8372136 to 2e21611 Compare September 17, 2020 05:09

pingsutw force-pushed the tf-plugin branch from 2e21611 to ff2a042 Compare February 3, 2021 18:00

Tensorflow plugin implementaion

48035aa

pingsutw force-pushed the tf-plugin branch from ff2a042 to 48035aa Compare February 4, 2021 08:32

Fix checkstyle

f5ddc83

EngHabu reviewed Feb 4, 2021

View reviewed changes

pingsutw and others added 3 commits February 6, 2021 22:45

Update go/tasks/plugins/k8s/kfoperators/common/common_operator.go

287bb4a

Co-authored-by: Haytham Abuelfutuh <[email protected]>

Address comments

24aa57e

Fix lint error

894d52f

EngHabu reviewed Feb 12, 2021

View reviewed changes

go/tasks/plugins/k8s/kfoperators/common/common_operator.go Outdated Show resolved Hide resolved

pingsutw added 2 commits February 12, 2021 23:09

Address comment

21cc0ed

Fix lint

4c8e1bf

Update to running

3518953

EngHabu approved these changes Feb 13, 2021

View reviewed changes

EngHabu merged commit e84585e into flyteorg:master Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tensorflow plugin implementation #103

tensorflow plugin implementation #103

pingsutw commented Jul 11, 2020 •

edited

Loading

pingsutw commented Jul 11, 2020

kumare3 commented Jul 11, 2020

pingsutw commented Jul 12, 2020

swiftdiaries commented Aug 3, 2020

codecov-commenter commented Aug 13, 2020 •

edited

Loading

pingsutw commented Aug 13, 2020

swiftdiaries commented Aug 13, 2020

swiftdiaries commented Aug 13, 2020

kumare3 commented Aug 13, 2020

pingsutw commented Aug 14, 2020

kumare3 commented Aug 14, 2020

kumare3 Jul 11, 2020

pingsutw Aug 20, 2020

kumare3 commented Aug 18, 2020

pingsutw commented Aug 20, 2020

pingsutw commented Aug 20, 2020

kumare3 commented Aug 20, 2020

kumare3 left a comment

kumare3 commented Sep 14, 2020

codecov-io commented Feb 4, 2021 •

edited

Loading

pingsutw commented Feb 4, 2021

EngHabu left a comment

pingsutw commented Feb 4, 2021

pingsutw commented Feb 12, 2021

kumare3 commented Feb 12, 2021

EngHabu commented Feb 12, 2021

pingsutw commented Feb 12, 2021

tensorflow plugin implementation #103

tensorflow plugin implementation #103

Conversation

pingsutw commented Jul 11, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

pingsutw commented Jul 11, 2020

kumare3 commented Jul 11, 2020

pingsutw commented Jul 12, 2020

swiftdiaries commented Aug 3, 2020

codecov-commenter commented Aug 13, 2020 • edited Loading

Codecov Report

pingsutw commented Aug 13, 2020

swiftdiaries commented Aug 13, 2020

swiftdiaries commented Aug 13, 2020

kumare3 commented Aug 13, 2020

pingsutw commented Aug 14, 2020

kumare3 commented Aug 14, 2020

kumare3 Jul 11, 2020

Choose a reason for hiding this comment

pingsutw Aug 20, 2020

Choose a reason for hiding this comment

kumare3 commented Aug 18, 2020

pingsutw commented Aug 20, 2020

pingsutw commented Aug 20, 2020

kumare3 commented Aug 20, 2020

kumare3 left a comment

Choose a reason for hiding this comment

kumare3 commented Sep 14, 2020

codecov-io commented Feb 4, 2021 • edited Loading

Codecov Report

pingsutw commented Feb 4, 2021

EngHabu left a comment

Choose a reason for hiding this comment

pingsutw commented Feb 4, 2021

pingsutw commented Feb 12, 2021

kumare3 commented Feb 12, 2021

EngHabu commented Feb 12, 2021

pingsutw commented Feb 12, 2021

pingsutw commented Jul 11, 2020 •

edited

Loading

codecov-commenter commented Aug 13, 2020 •

edited

Loading

codecov-io commented Feb 4, 2021 •

edited

Loading