Add kubeflow cluster environment #7300

neggert · 2021-04-30T18:53:40Z

What does this PR do?

Adds a ClusterEnvironment that works with the PyTorchJob operator in Kubeflow.

One open question from our discussion in Slack: is there a way to automatically tell if we're running inside a PyTorchJob? I've looked into this a bit, and I don't think there is. It's pretty easy to tell if we're in Kubernetes (various environment variables), but I haven't found a good way to check whether we're specifically running in a PyTorchJob. We could maybe query the Kubernetes API to find out, but that seems like overkill to me. Looking for feedback here.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
- Discussed in Slack: https://pytorch-lightning.slack.com/archives/CU5PF2E2J/p1619447878081900
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2021-04-30T18:53:43Z

Hello @neggert! Thanks for updating this PR.

In the file pytorch_lightning/trainer/connectors/accelerator_connector.py:

Line 424:17: W503 line break before binary operator

Comment last updated at 2021-05-12 12:41:29 UTC

codecov · 2021-04-30T18:55:24Z

Codecov Report

Merging #7300 (04c571c) into master (db54b30) will decrease coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #7300    +/-   ##
=======================================
- Coverage      92%     88%    -4%     
=======================================
  Files         199     200     +1     
  Lines       13067   13100    +33     
=======================================
- Hits        11997   11470   -527     
- Misses       1070    1630   +560

awaelchli · 2021-04-30T19:27:56Z

Hey! Awesome

Just fyi here is where we select the environment:

https://github.com/PyTorchLightning/pytorch-lightning/blob/490cc57809ebeba19003b4101393a8a058217c31/pytorch_lightning/trainer/connectors/accelerator_connector.py#L482

And here the example of TorchElastic spying on Env variables to identify itself:
As you can see in TorchElasticEnvironment.is_using_torch_elastic() we spy on ENV variables:
https://github.com/PyTorchLightning/pytorch-lightning/blob/490cc57809ebeba19003b4101393a8a058217c31/pytorch_lightning/plugins/environments/torchelastic_environment.py#L29

Now, as you say we need to detect PyTorchJob. I don't know PyTorchJob, but I searched GitHub for os.environ query and found here https://github.com/kubeflow/pytorch-operator/search?q=os.environ there is a PYTORCHJOB_VERSION env variable. But I'm not sure if this one is guaranteed to be set. Or at minimum we can document that we autodetec IF this variable is present.

(btw the coverage is sometimes red because not yet all jobs have reported results (e.g. GPU still running)

neggert · 2021-04-30T19:51:40Z

I'm not seeing PYTORCHJOB_VERSION get set in our Kubeflow install. It looks to me like that variable is used by the Python SDK, but it's not actually injected into the container itself.

Here's the code that injects variables into the pod containers: https://github.com/kubeflow/pytorch-operator/blob/4aeb6503162465766476519339d3285f75ffe03e/pkg/controller.v1/pytorch/pod.go#L259

The only thing I can think would be to look for KUBERNETES_PORT, MASTER_ADDR, MASTER_PORT, WORLD_SIZE and RANK to be set, but not GROUP_RANK, LOCAL_RANK, or LOCAL_WORLD_SIZE, since the latter would be set if running under TorchElastic. This seems like it would be brittle, but maybe I'm overthinking it.

awaelchli · 2021-04-30T21:30:28Z

You think it may be brittle because you suspect there are other cluster managers that could be running with Kubernetes and be sharing the same env variables? I wouldn't know how to answer this but I could ask around. If we are unsure, it may be better to go with the manual selection / through argument parsing.

neggert · 2021-04-30T23:29:16Z

Yeah, that's pretty much the lines I'm thinking along. If you're comfortable with it, though, I'm happy to make the code change.

awaelchli · 2021-05-02T18:54:05Z

yes sounds good !

Borda · 2021-05-04T06:52:11Z

pytorch_lightning/plugins/environments/kubeflow_environment.py

+    def creates_children(self) -> bool:
+        return True
+
+    def master_address(self) -> str:
+        return os.environ['MASTER_ADDR']
+
+    def master_port(self) -> int:
+        return int(os.environ['MASTER_PORT'])
+
+    def world_size(self) -> int:
+        return int(os.environ['WORLD_SIZE'])
+
+    def set_world_size(self, size: int) -> None:
+        log.debug("KubeflowEnvironment.set_world_size was called, but setting world size is not allowed. Ignored.")
+
+    def global_rank(self) -> int:
+        return int(os.environ["RANK"])
+
+    def set_global_rank(self, rank: int) -> None:
+        log.debug(
+            "KubeflowEnvironment.set_global_rank was called, but setting global rank is not allowed. Ignored."
+        )
+
+    def local_rank(self) -> int:
+        return 0
+
+    def node_rank(self) -> int:
+        return self.global_rank()


@awaelchli just thinking why are they not as property?

not my decision. they were never properties from the very beginning and when I started working on these environments it was easier to keep the existing pattern.

I will refactor it, issue tracking here #6303

for more information, see https://pre-commit.ci

neggert · 2021-05-07T21:07:10Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+                    use_torchelastic_ddp or
+                    use_kubeflow_ddp or
+                    use_ddp_cpu_kubeflow
+            ):


This seems like it might be due for a refactor, but I just extended the existing code for now, since I'm not 100% sure of all the edge cases this might be handling.

it's getting a bit ridiculous yes.
I have ideas how to simplify this in the future. But your addition looks solid!
So to summarize: The Kubeflow cluster environment is available with: DDPPlugin, DDPSpawnPlugin, user custom plugin, correct?

I think this will always auto-select DDPPlugin when in Kubeflow. Per my other comment, I'm not sure DDPSpawnPlugin makes much sense in this environment.

…htning into kubeflow_environment

awaelchli

great addition!!

pytorch_lightning/plugins/environments/kubeflow_environment.py

tests/accelerators/test_accelerator_connector.py

awaelchli · 2021-05-07T23:11:11Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+                    use_torchelastic_ddp or
+                    use_kubeflow_ddp or
+                    use_ddp_cpu_kubeflow
+            ):


it's getting a bit ridiculous yes.
I have ideas how to simplify this in the future. But your addition looks solid!
So to summarize: The Kubeflow cluster environment is available with: DDPPlugin, DDPSpawnPlugin, user custom plugin, correct?

Co-authored-by: Adrian Wälchli <[email protected]>

neggert · 2021-05-11T19:14:55Z

Any idea why that pre-commit check is failing? Pre-commit seems to run find on my end.

ananthsub

Nice!

carmocca · 2021-05-12T12:17:22Z

Any idea why that pre-commit check is failing? Pre-commit seems to run find on my end.

Unrelated, opened #7500 to fix it

neggert requested review from awaelchli, Borda, carmocca, edenlightning, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners April 30, 2021 18:53

awaelchli added the feature Is an improvement or enhancement label Apr 30, 2021

awaelchli added this to the v1.4 milestone Apr 30, 2021

awaelchli self-assigned this Apr 30, 2021

Borda reviewed May 4, 2021

View reviewed changes

mergify bot added has conflicts and removed has conflicts labels May 6, 2021

neggert added 6 commits May 7, 2021 14:31

Add kubeflow cluster environment

d18e4cf

Add KubeflowEnvironment to docs

86e1660

Add KubeflowEnvironment to the changelog

d92dff7

break up a long line

c9c151e

Add method to detect kubeflow environment

ba9017e

Select Kubeflow environment when available

cbdde3c

neggert force-pushed the kubeflow_environment branch from 84bd318 to cbdde3c Compare May 7, 2021 21:05

[pre-commit.ci] auto fixes from pre-commit.com hooks

f5852c9

for more information, see https://pre-commit.ci

neggert commented May 7, 2021

View reviewed changes

neggert added 2 commits May 7, 2021 16:09

Run pre-commit

e3ea5c3

Merge branch 'kubeflow_environment' of github.com:neggert/pytorch-lig…

5333f42

…htning into kubeflow_environment

awaelchli approved these changes May 7, 2021

View reviewed changes

tchaton mentioned this pull request May 10, 2021

PyTorch Lightning Example. kubeflow/pytorch-operator#334

Closed

task_idx == 0

bc1cf0c

Co-authored-by: Adrian Wälchli <[email protected]>

ananthsub approved these changes May 11, 2021

View reviewed changes

awaelchli added the ready PRs ready to be merged label May 12, 2021

carmocca approved these changes May 12, 2021

View reviewed changes

Merge branch 'master' into kubeflow_environment

04c571c

tchaton merged commit f4f51e0 into Lightning-AI:master May 17, 2021

awaelchli mentioned this pull request Jun 27, 2022

Multiple GPU per node could fail silently with KubeflowEnvironment #13374

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kubeflow cluster environment #7300

Add kubeflow cluster environment #7300

neggert commented Apr 30, 2021

pep8speaks commented Apr 30, 2021 •

edited

Loading

codecov bot commented Apr 30, 2021 •

edited

Loading

awaelchli commented Apr 30, 2021 •

edited

Loading

neggert commented Apr 30, 2021

awaelchli commented Apr 30, 2021

neggert commented Apr 30, 2021

awaelchli commented May 2, 2021

Borda May 4, 2021

awaelchli May 4, 2021

neggert May 7, 2021

awaelchli May 7, 2021

neggert May 10, 2021

awaelchli left a comment

awaelchli May 7, 2021

neggert commented May 11, 2021

ananthsub left a comment

carmocca commented May 12, 2021

Add kubeflow cluster environment #7300

Add kubeflow cluster environment #7300

Conversation

neggert commented Apr 30, 2021

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Apr 30, 2021 • edited Loading

Comment last updated at 2021-05-12 12:41:29 UTC

codecov bot commented Apr 30, 2021 • edited Loading

Codecov Report

awaelchli commented Apr 30, 2021 • edited Loading

neggert commented Apr 30, 2021

awaelchli commented Apr 30, 2021

neggert commented Apr 30, 2021

awaelchli commented May 2, 2021

Borda May 4, 2021

Choose a reason for hiding this comment

awaelchli May 4, 2021

Choose a reason for hiding this comment

neggert May 7, 2021

Choose a reason for hiding this comment

awaelchli May 7, 2021

Choose a reason for hiding this comment

neggert May 10, 2021

Choose a reason for hiding this comment

awaelchli left a comment

Choose a reason for hiding this comment

awaelchli May 7, 2021

Choose a reason for hiding this comment

neggert commented May 11, 2021

ananthsub left a comment

Choose a reason for hiding this comment

carmocca commented May 12, 2021

pep8speaks commented Apr 30, 2021 •

edited

Loading

codecov bot commented Apr 30, 2021 •

edited

Loading

awaelchli commented Apr 30, 2021 •

edited

Loading