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

send cluster status to den when running runhouse status cli command #1097

Merged
merged 1 commit into from
Aug 28, 2024
Merged

send cluster status to den when running runhouse status cli command #1097

merged 1 commit into from
Aug 28, 2024

Conversation

BelSasha
Copy link
Contributor

@BelSasha BelSasha commented Aug 1, 2024

No description provided.

@BelSasha Graphite App
Copy link
Contributor Author

BelSasha commented Aug 1, 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 marked this pull request as ready for review August 1, 2024 19:17
@dongreenberg
Copy link
Contributor

I'd rather not have this logic duplicated between the ClusterServlet and main.py, and generally we try to have interfaces with ClusterServlet and EnvServlet go through the obj_store, not directly from main.py or HTTP_server. Can we just modify the ClusterServlet.aperiodic_cluster_checks method to call a ClusterServlet.asend_cluster_status(status) method, and then have obj_store.astatus take an optional argument send_to_den=True, which calls ClusterServlet.asend_cluster_status(status) before returning it?

@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch 3 times, most recently from 0990747 to 76921a1 Compare August 2, 2024 08:25
jlewitt1
jlewitt1 reviewed Aug 2, 2024
View reviewed changes
runhouse/main.py Outdated Show resolved Hide resolved
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from 76921a1 to 73a3bb6 Compare August 2, 2024 12:30
@BelSasha BelSasha requested a review from jlewitt1 August 2, 2024 12:30
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from 73a3bb6 to 1688d7b Compare August 2, 2024 13:17
jlewitt1
jlewitt1 reviewed Aug 11, 2024
View reviewed changes
runhouse/main.py Outdated Show resolved Hide resolved
@BelSasha BelSasha mentioned this pull request Aug 11, 2024
Merged
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from 1688d7b to e0ff18c Compare August 11, 2024 13:45
jlewitt1
jlewitt1 reviewed Aug 11, 2024
View reviewed changes
runhouse/main.py Outdated Show resolved Hide resolved
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from e0ff18c to 0509b93 Compare August 11, 2024 15:53
@BelSasha BelSasha requested a review from jlewitt1 August 11, 2024 15:54
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch 7 times, most recently from b56c01a to 4569713 Compare August 15, 2024 09:54
jlewitt1
jlewitt1 reviewed Aug 22, 2024
View reviewed changes
runhouse/main.py Outdated Show resolved Hide resolved
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch 2 times, most recently from 4354a26 to de74386 Compare August 22, 2024 11:18
@BelSasha BelSasha requested a review from jlewitt1 August 22, 2024 11:19
jlewitt1
jlewitt1 reviewed Aug 22, 2024
View reviewed changes
tests/test_resources/test_clusters/test_on_demand_cluster.py Outdated
@@ -201,6 +202,37 @@ def test_fn_to_docker_container(self, ondemand_aws_cluster):
####################################################################################################
# Status tests
####################################################################################################

@pytest.mark.level("minimal")
Copy link
Collaborator

Choose a reason for hiding this comment

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

these tests should prob be in test_cluster no? doesn't really need to be limited to ondemand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but we are using cluster.teardown() which is an on-demand cluster method. Runhouse Cluster does not have such method. Moreover, seems like runhouse stop is not a part of runhouse cli API (not in the docs - is it on purpose?), so not sure if we should use it in the test to this use-case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to do teardown? i see those tests are being skipped anyways maybe we can remove them

Copy link
Contributor Author

@BelSasha BelSasha Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

to test that once we call cluster.teardown() the updated status is sent to den. We could delete it for now since they are skipped, but I think it better to add them to a separate test suite that runs tests related to cluster termination (I think we've previously talked about it). wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you mean confirming that when you call teardown the status gets updated in Den? maybe i'm missing something but i don't see that in this test?

@BelSasha BelSasha requested a review from jlewitt1 August 22, 2024 13:23
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch 4 times, most recently from b4e8562 to 6e1af83 Compare August 25, 2024 07:09
jlewitt1
jlewitt1 reviewed Aug 25, 2024
View reviewed changes
runhouse/main.py Outdated Show resolved Hide resolved
jlewitt1
jlewitt1 reviewed Aug 25, 2024
View reviewed changes
tests/test_resources/test_clusters/test_on_demand_cluster.py Outdated
)
logs_end_line = cluster_status.get("cluster_config").get("end_log_line", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about comparing the start and end lines before and after the status call? wouldn't that be a way to confirm the status update worked without actually having to test the call to Den?

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 could do that, but it this case we'll have to wait 1 min to ensure that the cluster check loop were executed during the test. This will cause quite latency in executing the tests....

In this case i think that calling den is less "harmful". wdyt? @jlewitt1

@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from 6e1af83 to f906fb9 Compare August 25, 2024 09:11
@BelSasha BelSasha requested a review from jlewitt1 August 25, 2024 11:13
@BelSasha BelSasha force-pushed the sb/send_cluster_status_to_den_from_cli branch from f906fb9 to a1e13a8 Compare August 25, 2024 12:26
@BelSasha BelSasha mentioned this pull request Aug 25, 2024
Merged
@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/send_cluster_status_to_den_from_cli branch from da456c2 to 4fb129a Compare August 28, 2024 10:02
@BelSasha BelSasha merged commit 25f1c87 into main Aug 28, 2024
12 checks passed
@BelSasha BelSasha deleted the sb/send_cluster_status_to_den_from_cli branch August 28, 2024 10:35
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

@dongreenberg dongreenberg Awaiting requested review from dongreenberg

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

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