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

Dask Distributor #1379

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Dask Distributor #1379

merged 1 commit into from
Nov 13, 2024

Conversation

dongreenberg
Copy link
Contributor

No description provided.

@dongreenberg dongreenberg marked this pull request as ready for review November 5, 2024 14:59
Copy link
Contributor Author

dongreenberg commented Nov 5, 2024

This was referenced Nov 5, 2024
Copy link
Contributor

@py-rh py-rh left a comment

Choose a reason for hiding this comment

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

LGTM

@dongreenberg dongreenberg force-pushed the pytorch_distributor branch 2 times, most recently from 2536203 to 72eb158 Compare November 6, 2024 20:30
@dongreenberg dongreenberg force-pushed the pytorch_distributor branch 2 times, most recently from d196d6a to 79438ee Compare November 8, 2024 21:42
@dongreenberg dongreenberg force-pushed the dask_distributor branch 2 times, most recently from 198c085 to 1ad9750 Compare November 12, 2024 23:06
@@ -141,6 +142,10 @@ def address(self, addr):
self.ips = self.ips or [None]
self.ips[0] = addr

@property
def internal_ips(self):
return self.ips
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.ips refers to external ips right?

Copy link
Contributor

@py-rh py-rh Nov 13, 2024

Choose a reason for hiding this comment

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

image
Whatever Donny has done here works great with Dask, but yes -- I think it works because it's never used anywhere lol

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's because for a static cluster internal_ips and external_ips are the same, and we need the cluster to have an internal_ips property just like on_demand_cluster, or the dask (and ray) start sequence for each needs to be different (we need to use an internal ip to connect from the worker nodes to the head node).

Copy link
Contributor

Choose a reason for hiding this comment

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

🤯🤯🤯

Copy link
Contributor Author

dongreenberg commented Nov 13, 2024

Merge activity

  • Nov 12, 11:52 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 13, 12:10 AM EST: Graphite rebased this pull request as part of a merge.
  • Nov 13, 12:11 AM EST: A user merged this pull request with Graphite.

@dongreenberg dongreenberg changed the base branch from pytorch_distributor to graphite-base/1379 November 13, 2024 05:05
@dongreenberg dongreenberg changed the base branch from graphite-base/1379 to main November 13, 2024 05:07
@dongreenberg dongreenberg merged commit d190794 into main Nov 13, 2024
9 of 13 checks passed
@dongreenberg dongreenberg deleted the dask_distributor branch November 13, 2024 05:11
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.

3 participants