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

Replace cluster internal_external_ips attribute with internal_ips #1417

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

carolineechen
Copy link
Collaborator

@carolineechen carolineechen commented Nov 14, 2024

replace self.internal_external_ips with just self.internal_ips (self.ips already stores external ips). leave ability to load internal_external_ips from kwargs for backwards compatibility for now.

Copy link

sentry-io bot commented Nov 14, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: runhouse/resources/hardware/cluster.py

Function Unhandled Issue
_sync_default_env_to_cluster AttributeError: 'OnDemandCluster' object has no attribute 'secrets' runhouse.resources.hardware.cluster in _sync_def...
Event Count: 1
__init__ ValueError: Could not locate secret ssh-sky-key _...
Event Count: 1
📄 File: runhouse/resources/hardware/on_demand_cluster.py (Click to Expand)
Function Unhandled Issue
__init__ AttributeError: module 'sky' has no attribute 'global_user_state' __m...
Event Count: 2
---

Did you find this useful? React with a 👍 or 👎

@jlewitt1
Copy link
Collaborator

maybe we should rebase this onto the PRs moving IPs into launched properties and making into a list?

@carolineechen
Copy link
Collaborator Author

@jlewitt1 those PRs are rebased onto this one so it should be the same in the end either way - this + the list one should be good to go, and still need to fix a few spots I missed for the last 2 PRs in the stack

@carolineechen carolineechen merged commit 9baae99 into main Nov 18, 2024
12 checks passed
@carolineechen carolineechen deleted the cc/internal-ips branch November 18, 2024 17:38
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