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

modify cluster status checks tests #1197

Merged
merged 1 commit into from
Sep 5, 2024
Merged

modify cluster status checks tests #1197

merged 1 commit into from
Sep 5, 2024

Conversation

BelSasha
Copy link
Contributor

No description provided.

@BelSasha BelSasha marked this pull request as ready for review August 25, 2024 16:40
@BelSasha BelSasha mentioned this pull request Aug 25, 2024
Merged
@BelSasha Graphite App
Copy link
Contributor Author

BelSasha commented Aug 25, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @BelSasha and the rest of your teammates on Graphite Graphite

@BelSasha BelSasha mentioned this pull request Aug 25, 2024
Closed
@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 6 times, most recently from 5928f9e to 9e540f6 Compare August 26, 2024 12:49
@BelSasha BelSasha requested a review from jlewitt1 August 26, 2024 13:05
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from a1e13a8 to da456c2 Compare August 27, 2024 07:21
@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 2 times, most recently from 717583b to 2a92617 Compare August 27, 2024 09:16
BelSasha
BelSasha commented Aug 28, 2024
View reviewed changes
tests/utils.py Outdated Show resolved Hide resolved
runhouse/servers/cluster_servlet.py Outdated Show resolved Hide resolved
runhouse/servers/cluster_servlet.py Outdated Show resolved Hide resolved
tests/test_servers/test_servlet.py Outdated Show resolved Hide resolved
tests/test_servers/test_servlet.py Outdated Show resolved Hide resolved
tests/test_servers/test_servlet.py Outdated Show resolved Hide resolved
tests/test_servers/test_servlet.py Outdated Show resolved Hide resolved
tests/test_resources/test_clusters/test_on_demand_cluster.py Show resolved Hide resolved
runhouse/resources/hardware/on_demand_cluster.py Outdated Show resolved Hide resolved
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from da456c2 to 4fb129a Compare August 28, 2024 10:02
@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 3 times, most recently from 45e2b88 to dfc559c Compare August 28, 2024 10:27
Base automatically changed from sb/send_cluster_status_to_den_from_cli to main August 28, 2024 10:35
@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 5 times, most recently from 07f29ea to 2e55a66 Compare August 29, 2024 09:53
@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 2 times, most recently from 38ac9fe to 74be517 Compare September 2, 2024 08:41
jlewitt1
jlewitt1 reviewed Sep 2, 2024
View reviewed changes
runhouse/servers/cluster_servlet.py Outdated
@@ -84,6 +85,16 @@ async def aset_cluster_config(self, cluster_config: Dict[str, Any]):

self.cluster_config = cluster_config

new_cluster_name = self.cluster_config.get("name", None)

if self._cluster_name != new_cluster_name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

confused by what's going on here - why do we need to reset the cluster name if we area already pulling it from the cliuster config above?

Copy link
Contributor Author

@BelSasha BelSasha Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are setting a new cluster_config, so we want to verify that the cluster name saved as ClusterServlet property is matching with the cluster_name set in the new cluster_config we set.

This is a rare use case where the cluster name in the new_cluster_config will differ from the cluster_name that is already set in the ClusterServlet, but i think we should handle it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we even be allowing updating the name here? i feel like that can lead to all sorts of issues with mismatches between what's saved on the cluster vs in den

@BelSasha BelSasha force-pushed the sb/revamp-status-tests branch 5 times, most recently from a3dfa36 to 7f75a0b Compare September 4, 2024 07:41
@BelSasha BelSasha merged commit 0d6293d into main Sep 5, 2024
12 checks passed
@BelSasha BelSasha deleted the sb/revamp-status-tests branch September 5, 2024 14:46
BelSasha added a commit that referenced this pull request Sep 17, 2024
@BelSasha
modify cluster status checks tests (#1197)
4569ea2 
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
BelSasha added a commit that referenced this pull request Sep 17, 2024
@BelSasha
modify cluster status checks tests (#1197)
8fd2d9b 
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
BelSasha added a commit that referenced this pull request Sep 18, 2024
@BelSasha
modify cluster status checks tests (#1197)
2f87281 
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
BelSasha added a commit that referenced this pull request Sep 18, 2024
@BelSasha
modify cluster status checks tests (#1197)
a048a28 
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlewitt1 jlewitt1 jlewitt1 approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@BelSasha @jlewitt1