[Serve] Sync down logs #3063

dtran24 · 2024-01-31T23:37:38Z

Fixes #2914

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
- sync down when only launch logs exists, and the two replicas are in PROVISIONING state
- sync down across two READY replicas
- sync down when one replica READY, and another replica terminated (e.g. through sky cancel on controller)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

…ircular imports

…s method (avoid circular import)

dtran24 · 2024-02-16T05:25:28Z

Requesting review @cblmemo @MaoZiming

cblmemo

Thanks @dtran24 for this exciting feature!! Left some preliminary comments 🫡

sky/cli.py

sky/serve/core.py

sky/serve/serve_utils.py

tests/unit_tests/test_serve_utils.py

sky/serve/constants.py

…le replica

…g err

sky/backends/cloud_vm_ray_backend.py

sky/cli.py

sky/serve/core.py

sky/serve/constants.py

sky/serve/serve_utils.py

cblmemo

Thanks for the prompt fix!! 🚀 It looks mostly great to me. Left some final comments

sky/backends/cloud_vm_ray_backend.py

sky/serve/core.py

sky/serve/serve_utils.py

cblmemo · 2024-02-24T15:16:32Z

sky/serve/serve_utils.py

+            launch_log_file)
+        replica_info = serve_state.get_replica_info_from_id(
+            service_name, replica_id)
+        if replica_info is None:


For replica that has been normally terminated (scale down), we don't keep the record in database but we will keep its log file under the service directory. So it is likely the replica_info is None IIUC. Should we just continue here?

Inside this for loop, we are only dealing with launch log files, which shouldn't be relevant to replicas that have been normally terminated. For replicas that have been normally terminated, we manage them here

skypilot/sky/serve/serve_utils.py

Lines 657 to 662 in 1eb3a53

terminated_replica_log_files = [

file for file in log_files if not file.endswith('_launch.log') and

has_valid_replica_id(file, target_replica_id)

]

for file_path in terminated_replica_log_files:

shutil.copy(file_path, dir_for_download)

sky/serve/serve_utils.py

…work

cblmemo

also, could you address the merge conflict?

cblmemo · 2024-03-02T09:12:32Z

sky/backends/cloud_vm_ray_backend.py

as mentioned, we might want to move those code under sky/serve.

Michaelvll · 2024-06-17T16:15:40Z

A bump for this PR as a user is requesting this feature.

aj-ya · 2024-06-26T13:18:52Z

Hey! I was trying to explore on the log capturing part. Is there a way to continuously sync down the logs.
Also looks like this PR is a bit outdated. Is this going to be taken up again?

cblmemo · 2024-06-28T04:16:46Z

Hey! I was trying to explore on the log capturing part. Is there a way to continuously sync down the logs. Also looks like this PR is a bit outdated. Is this going to be taken up again?

Hi @aj-ya ! Thanks for your interest in this PR. For now, one workaround would be having a screen session and redirecting the output of sky serve logs to some files:

sky serve logs service-name --controller > ~/serve-logs/controller.log 2>&1

This PR is on our roadmap and we'll try to prioritize it based on your request. Thanks!

David Tran added 2 commits January 31, 2024 18:35

[skyserve] sync down controller logs

40540b2

[skyserve] sync down load balancer logs

d9653af

dtran24 changed the title ~~[SkyServe] Sync down logs~~ [Serve] Sync down logs Feb 4, 2024

David Tran and others added 16 commits February 10, 2024 20:48

[serve] add skeleton for preparing replica logs for download

21239ec

[serve] prepare replica logs for download and sync down

7571d44

[serve] move sky serve controller name to constants file to resolve c…

17adc60

…ircular imports

Merge branch 'master' into skyserve/sync-down-logs

2b3dd94

[serve] optimize imports for __init__ and inline sky logs dir

98e6d27

[serve] use sky logs dir constant from skylet

5168a9d

[serve] add test for extract replica id

15119e0

[serve] remove comments

b7fc194

[serve] remove replica logs after download

2ed1121

[serve] rename vars

26c3f62

[serve] fix codegen and stream logs when running prepare code

07c2825

[serve] fix glob for launch and full replica log files

eed5985

[serve] move replica info classes to prepare for download_replica_log…

02f329f

…s method (avoid circular import)

[serve] add utf8 encoding when using open

36ea3c6

[serve] revert last two commits and add utf-8 encoding

72c238f

[serve] clean up

0095add

dtran24 marked this pull request as ready for review February 16, 2024 05:25

cblmemo reviewed Feb 16, 2024

View reviewed changes

David Tran added 4 commits February 17, 2024 17:34

[serve] support downloading only controller, or only lb, or only sing…

50e17d1

…le replica

[serve] rename method and change some logging

09f0b5b

update exception msg and remove unnecessary else statement for raisin…

acd9967

…g err

refactor into sync_down_serve_logs method in backend file

0766bb7

dtran24 requested a review from cblmemo February 18, 2024 01:18

David Tran added 2 commits February 19, 2024 10:56

use sync down logs directly

04a0b37

move sky serve controller constant back to serve_utils

e02b290

cblmemo reviewed Feb 21, 2024

View reviewed changes

David Tran added 5 commits February 20, 2024 22:41

fix nits

99cf4d6

more nits

684b62b

rename constants for log file names

bfb82dd

remove use of no replica id constant

c830bfa

add try catch

40defe3

dtran24 requested a review from cblmemo February 21, 2024 05:26

cblmemo reviewed Feb 24, 2024

View reviewed changes

David Tran and others added 8 commits February 27, 2024 00:03

add assert and fix nit

bf443e2

add some more punctuation

1350337

import serve constants as serve_constants

84f8515

Merge branch 'master' into skyserve/sync-down-logs

b71d01f

update controller constant used

d2e60cb

Make method private and add constant

1eb3a53

add docstr

ad38f69

add more descriptive error message if sync down for replica does not …

b1f78dc

…work

dtran24 requested a review from cblmemo February 27, 2024 23:06

cblmemo reviewed Mar 2, 2024

View reviewed changes

sky/backends/cloud_vm_ray_backend.py

Copy link

Collaborator

cblmemo Mar 2, 2024

Choose a reason for hiding this comment

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

as mentioned, we might want to move those code under sky/serve.

cblmemo self-assigned this Jun 17, 2024

root-hbx mentioned this pull request Oct 5, 2024

[Serve] A patch for sync down logs #4036

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] Sync down logs #3063

[Serve] Sync down logs #3063

dtran24 commented Jan 31, 2024 •

edited

Loading

dtran24 commented Feb 16, 2024

cblmemo left a comment

cblmemo left a comment

cblmemo Feb 24, 2024

dtran24 Feb 27, 2024

cblmemo left a comment

cblmemo Mar 2, 2024

Michaelvll commented Jun 17, 2024

aj-ya commented Jun 26, 2024

cblmemo commented Jun 28, 2024

	terminated_replica_log_files = [
	file for file in log_files if not file.endswith('_launch.log') and
	has_valid_replica_id(file, target_replica_id)
	]
	for file_path in terminated_replica_log_files:
	shutil.copy(file_path, dir_for_download)

[Serve] Sync down logs #3063

Are you sure you want to change the base?

[Serve] Sync down logs #3063

Conversation

dtran24 commented Jan 31, 2024 • edited Loading

dtran24 commented Feb 16, 2024

cblmemo left a comment

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

cblmemo Feb 24, 2024

Choose a reason for hiding this comment

dtran24 Feb 27, 2024

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

cblmemo Mar 2, 2024

Choose a reason for hiding this comment

Michaelvll commented Jun 17, 2024

aj-ya commented Jun 26, 2024

cblmemo commented Jun 28, 2024

dtran24 commented Jan 31, 2024 •

edited

Loading