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.

Sign up for GitHub

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

Jump to bottom

Added mpi plugin #595

Merged
merged 25 commits into from
Oct 27, 2021
Merged

Added mpi plugin #595

merged 25 commits into from
Oct 27, 2021

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Aug 13, 2021 •
edited
Loading

TL;DR

  • Added flytekit plugin for mpi

Dependant PR

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

@task(
    task_config=MPIJob(
        num_workers=2,
        num_launcher_replicas=1,
        slots=1,
        per_replica_requests=Resources(mem="3000Mi", cpu="1"),
        per_replica_limits=Resources(mem="3000Mi", cpu="1"),
    ),
    retries=3,
    cache=True,
    cache_version="0.5",
)

Tracking Issue

flyteorg/flyte#1543

Follow-up issue

NA
OR
https://github.com/lyft/flyte/issues/

@yindia
Added mpi plugin
29d900a 
Signed-off-by: Yuvraj <[email protected]>
@kumare3
Copy link
Contributor

kumare3 commented Aug 24, 2021

@evalsocket i updated, this try it out. I think we should also add Horovod elastic to this plugin itself.

Lets talk about it. but first lets test this one

kumare3 and others added 3 commits August 24, 2021 17:16
@kumare3
updated
b8f4cdb 
Signed-off-by: Ketan Umare <[email protected]>
@yindia
Added MPIJob in model task
ae5b921 
Signed-off-by: Yuvraj <[email protected]>
@yindia
Added dependancy in plugin setup
ac3c84b 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia mentioned this pull request Aug 30, 2021
Merged
8 tasks
@yindia yindia marked this pull request as ready for review August 30, 2021 18:22
@yindia yindia force-pushed the feature/mpi-plugin branch from 078c7c4 to 61ca584 Compare October 11, 2021 16:24
@yindia yindia force-pushed the feature/mpi-plugin branch 3 times, most recently from da4a8bb to e8d6054 Compare October 19, 2021 14:22
@codecov
Copy link

codecov bot commented Oct 19, 2021 •
edited
Loading

Codecov Report

Merging #595 (796a543) into master (f03ecb6) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   85.74%   85.80%   +0.05%     
==========================================
  Files         357      358       +1     
  Lines       29747    29793      +46     
  Branches     2428     2428              
==========================================
+ Hits        25507    25564      +57     
+ Misses       3601     3590      -11     
  Partials      639      639
Impacted Files Coverage Δ
flytekit/models/common.py 89.09% <0.00%> (-7.31%) ⬇️
flytekit/models/admin/common.py 97.47% <0.00%> (-2.53%) ⬇️
flytekit/models/core/types.py 98.67% <0.00%> (-1.33%) ⬇️
flytekit/remote/component_nodes.py 80.00% <0.00%> (-0.33%) ⬇️
flytekit/remote/nodes.py 72.30% <0.00%> (-0.22%) ⬇️
flytekit/types/file/file.py 90.78% <0.00%> (-0.07%) ⬇️
flytekit/core/promise.py 77.95% <0.00%> (-0.05%) ⬇️
flytekit/clis/flyte_cli/main.py 47.24% <0.00%> (-0.05%) ⬇️
tests/flytekit/unit/core/test_type_engine.py 99.71% <0.00%> (-0.01%) ⬇️
flytekit/core/shim_task.py 77.27% <0.00%> (ø)
... and 81 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 f03ecb6...796a543. Read the comment docs.

@yindia yindia mentioned this pull request Oct 19, 2021
Closed
8 tasks
@yindia yindia requested a review from eapolinario as a code owner October 21, 2021 00:40
@yindia
more changes
377a11a 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia force-pushed the feature/mpi-plugin branch from 9cedd21 to 377a11a Compare October 21, 2021 17:51
@yindia yindia force-pushed the feature/mpi-plugin branch from a6c95fa to 37e34d3 Compare October 22, 2021 00:44
@yindia yindia requested a review from katrogan October 22, 2021 00:46
@yindia yindia force-pushed the feature/mpi-plugin branch from 37e34d3 to 577fff7 Compare October 22, 2021 00:58
@yindia yindia force-pushed the feature/mpi-plugin branch from 18596cb to 2e43df6 Compare October 22, 2021 20:40
@yindia yindia requested a review from katrogan October 22, 2021 21:18
@yindia yindia force-pushed the feature/mpi-plugin branch from 08c8dc4 to a06b88f Compare October 26, 2021 08:38
@yindia yindia force-pushed the feature/mpi-plugin branch from a06b88f to 52f7b23 Compare October 26, 2021 08:44
katrogan
katrogan previously approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

@kumare3 mind reviewing too?

plugins/flytekit-kf-mpi/flytekitplugins/kfmpi/task.py Outdated Show resolved Hide resolved
kumare3
kumare3 previously approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

lgtm, a few minor nits. Please address and then merge

@yindia
minor docs change
3d1902c 
Signed-off-by: Yuvraj <[email protected]>
@yindia yindia dismissed stale reviews from kumare3 and katrogan via 3d1902c October 27, 2021 04:53
@yindia
make lint
a14aaec 
Signed-off-by: Yuvraj <[email protected]>
kumare3
kumare3 previously approved these changes Oct 27, 2021
View reviewed changes
@yindia yindia force-pushed the feature/mpi-plugin branch from eb22fef to 796a543 Compare October 27, 2021 05:23
@yindia yindia merged commit 895cded into master Oct 27, 2021
reverson pushed a commit to reverson/flytekit that referenced this pull request May 27, 2022
@yindia @kumare3
Added mpi plugin (flyteorg#595)
8f6de3b 
* Added mpi plugin

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

Co-authored-by: Ketan Umare <[email protected]>
Signed-off-by: Robert Everson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumare3 kumare3 kumare3 approved these changes

@katrogan katrogan katrogan left review comments

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@eapolinario eapolinario Awaiting requested review from eapolinario eapolinario is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yindia @kumare3 @katrogan