Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change flytekit Pytorch, TFJob and MPI plugins to use new kubeflow config #1627

Merged
merged 12 commits into from
May 31, 2023

Conversation

yubofredwang
Copy link
Contributor

@yubofredwang yubofredwang commented May 9, 2023

TL;DR

We have updated the plugin config in flyteidl and flytepropeller. This PR updates flytekit to uses the new configs for kubeflow plugins including: pytorch, tfjob and mpijob.

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

This change is backward compatible. If users continue using the old config like

@task(
task_config=TFJob(num_workers=1)
)
def my_task:
      ...

This still works.

However, the new config should look like:

@task(
task_config=TFJob(worker=Worker(replicas=1))
)
def my_task:
      ...

Tracking Issue

flyteorg/flyte#3308

@welcome
Copy link

welcome bot commented May 9, 2023

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).

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #1627 (d14b602) into master (01830e4) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head d14b602 differs from pull request most recent head 6701bc3. Consider uploading reports for the commit 6701bc3 to get more accurate results

@@            Coverage Diff             @@
##           master    #1627      +/-   ##
==========================================
- Coverage   71.00%   71.00%   -0.01%     
==========================================
  Files         336      336              
  Lines       30713    30713              
  Branches     5565     5565              
==========================================
- Hits        21808    21807       -1     
  Misses       8362     8362              
- Partials      543      544       +1     

see 1 file with indirect coverage changes

@yubofredwang yubofredwang force-pushed the master branch 3 times, most recently from ac8f627 to fbb0860 Compare May 13, 2023 01:34
Yubo Wang added 4 commits May 12, 2023 18:36
Signed-off-by: Yubo Wang <[email protected]>
Signed-off-by: Yubo Wang <[email protected]>
Signed-off-by: Yubo Wang <[email protected]>

Signed-off-by: Yubo Wang <[email protected]>
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Looks great, thank you. some minor comments below.

plugins/flytekit-kf-tensorflow/README.md Show resolved Hide resolved
plugins/flytekit-kf-tensorflow/README.md Outdated Show resolved Hide resolved
plugins/flytekit-kf-tensorflow/README.md Outdated Show resolved Hide resolved
Signed-off-by: Yubo Wang <[email protected]>
Yubo Wang and others added 3 commits May 15, 2023 11:26
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks

plugins/flytekit-kf-tensorflow/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

sorry for the extra bit of process, but can you update the requirements.txt file in each of the plugins? This is going to get rid of the plugin test errors in CI. Make sure to use python 3.9.

plugins/flytekit-kf-tensorflow/setup.py Outdated Show resolved Hide resolved
@yubofredwang yubofredwang changed the title [WIP]Change flytekit to use new kubeflow plugins config Change flytekit Pytorch, TFJob and MPI plugins to use new kubeflow config May 16, 2023
@yubofredwang
Copy link
Contributor Author

yubofredwang commented May 17, 2023

sorry for the extra bit of process, but can you update the requirements.txt file in each of the plugins? This is going to get rid of the plugin test errors in CI. Make sure to use python 3.9.

Updated to use python 3.9. I noticed all other plugins are using python 3.8. build-plugins 3.8 tests will fail in this case

eapolinario
eapolinario previously approved these changes May 17, 2023
Copy link
Collaborator

@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.

one minor question, otherwise LGTM.

plugins/flytekit-kf-mpi/README.md Show resolved Hide resolved
@eapolinario
Copy link
Collaborator

sorry for the extra bit of process, but can you update the requirements.txt file in each of the plugins? This is going to get rid of the plugin test errors in CI. Make sure to use python 3.9.

Updated to use python 3.9. I noticed all other plugins are using python 3.8. build-plugins 3.8 tests will fail in this case

I apologize for flip-flopping on this. I checked the requirements files for the 3 plugins and couldn't see why it wouldn't work for python 3.8, so I took the liberty of regenerating the requirements files. Will merge after tests pass.

@eapolinario eapolinario merged commit 894c260 into flyteorg:master May 31, 2023
@welcome
Copy link

welcome bot commented May 31, 2023

Congrats on merging your first pull request! 🎉

eapolinario added a commit that referenced this pull request Jun 29, 2023
…nfig (#1627)

* upgrade tensorflow plugin to v1

Signed-off-by: Yubo Wang <[email protected]>

* minor fix

Signed-off-by: Yubo Wang <[email protected]>

* fix tests and lints

Signed-off-by: Yubo Wang <[email protected]>

* move models file into task make backward compatible

Signed-off-by: Yubo Wang <[email protected]>

Signed-off-by: Yubo Wang <[email protected]>

* add code example in README

Signed-off-by: Yubo Wang <[email protected]>

* bump flyteidl

Signed-off-by: Yubo Wang <[email protected]>

* add pytorch

Signed-off-by: Yubo Wang <[email protected]>

* add mpi and fix requirements.txt

Signed-off-by: Yubo Wang <[email protected]>

* lint and fmt

Signed-off-by: Yubo Wang <[email protected]>

* Regenerate requirements files using python 3.8

Signed-off-by: eduardo apolinario <[email protected]>

---------

Signed-off-by: Yubo Wang <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>
Co-authored-by: Yubo Wang <[email protected]>
Co-authored-by: eduardo apolinario <[email protected]>
eapolinario added a commit that referenced this pull request Jun 30, 2023
* FLYTECTL_CONFIG env var higher precedence, config flag respected in pyflyte package (#1662)

Signed-off-by: Yee Hing Tong <[email protected]>

* Change flytekit Pytorch, TFJob and MPI plugins to use new kubeflow config (#1627)

* upgrade tensorflow plugin to v1

Signed-off-by: Yubo Wang <[email protected]>

* minor fix

Signed-off-by: Yubo Wang <[email protected]>

* fix tests and lints

Signed-off-by: Yubo Wang <[email protected]>

* move models file into task make backward compatible

Signed-off-by: Yubo Wang <[email protected]>

Signed-off-by: Yubo Wang <[email protected]>

* add code example in README

Signed-off-by: Yubo Wang <[email protected]>

* bump flyteidl

Signed-off-by: Yubo Wang <[email protected]>

* add pytorch

Signed-off-by: Yubo Wang <[email protected]>

* add mpi and fix requirements.txt

Signed-off-by: Yubo Wang <[email protected]>

* lint and fmt

Signed-off-by: Yubo Wang <[email protected]>

* Regenerate requirements files using python 3.8

Signed-off-by: eduardo apolinario <[email protected]>

---------

Signed-off-by: Yubo Wang <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>
Co-authored-by: Yubo Wang <[email protected]>
Co-authored-by: eduardo apolinario <[email protected]>

* Root cert should be byte string when loading from caCertFilePath (#1669)

Signed-off-by: Yee Hing Tong <[email protected]>

* Explicitly set the content type for flyte deck (#1658)

* Set content type for flyte deck

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Kevin Su <[email protected]>

* Use protos of new kubeflow.pytorch plugin instead of legacy pytorch plugin (#1678)

Signed-off-by: Fabio Grätz <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>

* More time info for time line deck (#1680)

* more visualization

Signed-off-by: Yicheng-Lu-llll <[email protected]>

* more visualization

Signed-off-by: Yicheng-Lu-llll <[email protected]>

---------

Signed-off-by: Yicheng-Lu-llll <[email protected]>

* Add http_proxy to client & Fix deviceflow (#1611)

* Add http_proxy to client & Fix deviceflow

RB=3890720

Signed-off-by: byhsu <[email protected]>

* nit

Signed-off-by: byhsu <[email protected]>

* lint!

Signed-off-by: byhsu <[email protected]>

---------

Signed-off-by: byhsu <[email protected]>
Co-authored-by: byhsu <[email protected]>

* Pass verify flag to all authenticators (#1641)

Signed-off-by: byhsu <[email protected]>

* feat: Add Auth0/audience support for ClientCredentials flow (#1639)

* feat: Add Auth0/audience support for ClientCredentials flow

Signed-off-by: tnam <[email protected]>

* refactor: Remove unneeded variables & condense code

Signed-off-by: tnam <[email protected]>

* refactor: Reduce verbosity of code

Signed-off-by: tnam <[email protected]>

* refactor(chore): Remove unused commented code

Signed-off-by: tnam <[email protected]>

* fix: Missing comma in input args - authenticator.py 213

Signed-off-by: tnam <[email protected]>

* style: Run pre-commit against all files

Signed-off-by: tnam <[email protected]>

---------

Signed-off-by: tnam <[email protected]>
Co-authored-by: tnam <[email protected]>

* pyflyte run remote file (#1670)

Signed-off-by: ChungYujoyce <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>

* upload deck.html only with deck enable (#1693)

Signed-off-by: Yicheng-Lu-llll <[email protected]>

* Add dask plugin #patch (#1366)

* Add dummy task type to test backend plugin

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add docs page

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add dask models

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add function to convert resources

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add tests to `dask` task

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Remove namespace

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update setup.py

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add dask to `plugin/README.md`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Add README.md for `dask`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Top level export of `JopPodSpec` and `DaskCluster`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update docs for images

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update README.md

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update models after `flyteidl` change

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update task after `flyteidl` change

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Raise error when less than 1 worker

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update flyteidl to >= 1.3.2

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update doc requirements

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update doc-requirements.txt

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Re-lock dependencies on linux

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Update dask API docs

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Fix documentation links

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Default optional model constructor arguments to `None`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Refactor `convert_resources_to_resource_model` to `core.resources`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Use `convert_resources_to_resource_model` in `core.node`

Signed-off-by: Bernhard Stadlbauer <[email protected]>

* Incorporate review feedback

Signed-off-by: Eduardo Apolinario <[email protected]>

* Lint

Signed-off-by: Eduardo Apolinario <[email protected]>

Signed-off-by: Bernhard Stadlbauer <[email protected]>
Signed-off-by: Bernhard Stadlbauer <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Do not bring dask changes

Signed-off-by: eduardo apolinario <[email protected]>

* Remove readthedocs

Signed-off-by: eduardo apolinario <[email protected]>

* Linting

Signed-off-by: eduardo apolinario <[email protected]>

* Force scipy<1.11.0 is on windows

Signed-off-by: eduardo apolinario <[email protected]>

---------

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yubo Wang <[email protected]>
Signed-off-by: eduardo apolinario <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Yicheng-Lu-llll <[email protected]>
Signed-off-by: byhsu <[email protected]>
Signed-off-by: tnam <[email protected]>
Signed-off-by: ChungYujoyce <[email protected]>
Signed-off-by: Bernhard Stadlbauer <[email protected]>
Signed-off-by: Bernhard Stadlbauer <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Yubo Wang <[email protected]>
Co-authored-by: Yubo Wang <[email protected]>
Co-authored-by: eduardo apolinario <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Fabio M. Graetz, Ph.D <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>
Co-authored-by: Yicheng-Lu-llll <[email protected]>
Co-authored-by: ByronHsu <[email protected]>
Co-authored-by: byhsu <[email protected]>
Co-authored-by: TomNam <[email protected]>
Co-authored-by: tnam <[email protected]>
Co-authored-by: ChungYujoyce <[email protected]>
Co-authored-by: bstadlbauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants