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 Update #1577

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Dask Update #1577

merged 1 commit into from
Dec 11, 2024

Conversation

py-rh
Copy link
Contributor

@py-rh py-rh commented Dec 11, 2024

No description provided.

@py-rh py-rh requested a review from dongreenberg December 11, 2024 20:49
@@ -2076,16 +2076,16 @@ def connect_dask(
)

# Note: We need to do this on the head node too, because this creates all the worker processes
for node in self.ips:
for idx, node in enumerate(self.ips):
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably cleaner to just iterate over self.internal_ips - otherwise there could potentially be an index out of range error thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer have a config field which contains both, so we have to iterate over one of the two since we use both. self.ips gives us node which is used in self.run()

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, are the array lengths always the same? Instead, it might be good to add a check before calling self.internal_ips[idx] like:

if len(self.internal_ips) > idx:
    self.run...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they are guaranteed to be the same length, no? Since each represents each node of a cluster.

Something much more wrong has happened with cluster launch if the cluster config dropped one of internal or external IPs. I'm not religiously opposed to adding a check, but this is the wrong place to do it if so.

@py-rh py-rh merged commit 31e173b into main Dec 11, 2024
8 of 10 checks passed
@py-rh py-rh deleted the py/dask-change branch December 11, 2024 21:24
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.

2 participants