[Serve] A patch for sync down logs #4036

root-hbx · 2024-10-05T00:00:16Z

A patch of #3063

fix all previous merge conflicts in #3063
migrate log components to sky/serve/core.py, as required by #4046

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)
- validate_log_file unittest is finished to check whether log file exists and replica ID matches.
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: conda deactivate; bash -i tests/backward_compatibility_tests.sh

…ircular imports

…s method (avoid circular import)

…le replica

…g err

cblmemo · 2024-10-23T22:00:14Z

sky/serve/core.py

+    if (sync_down_all_components or
+            service_component == serve_utils.ServiceComponent.CONTROLLER):
+        logger.info(
+            'Starting the process to prepare and download controller logs...')
+        runner = controller_handle.get_command_runners()
+        controller_log_file_name = (
+            serve_utils.generate_remote_controller_log_file_name(service_name))
+        logger.info('Downloading the controller logs...')
+        runner.rsync(source=controller_log_file_name,
+                     target=os.path.join(
+                         target_directory,
+                         serve_constants.CONTROLLER_LOG_FILE_NAME),
+                     up=False,
+                     stream_logs=False)
+        if not sync_down_all_components:
+            single_file_synced = True
+            target_directory = os.path.join(
+                target_directory, serve_constants.CONTROLLER_LOG_FILE_NAME)


is it possible that we keep a list of source & target and use loop to rsync them down?

Got it, just to confirm:

In fact, sync_down_logs() for the controller/load balancer only deals with one instance. Unlike replicas, which have multiple instances where using a loop is more convenient.

So, the reason for "keeping a list of source & target and using a loop to rsync them down" for the controller/load balancer is mainly for the sake of reducing code duplication and simplifying the code logic?

Yes. Currently it has a lot of duplication. Please also double check other places in the code.

cblmemo · 2024-10-23T22:02:18Z

sky/serve/serve_utils.py

+    # Get record from serve state.
+    service_record = serve_state.get_service_from_name(service_name)
+    if service_record is None:
+        raise ValueError(f'Service {service_name!r} does not exist.')


Lets add ux_utils.print_exception_no_traceback like #4111.

cblmemo · 2024-10-23T22:03:22Z

sky/serve/serve_utils.py

+
+    # These copies of logs may still be continuously updating.
+    # We need to synchronize the latest log data from the remote server.
+    for replica in service_record['replica_info']:


this included the terminal replicas? should we continue for them?

cblmemo · 2024-10-23T22:04:41Z

sky/serve/serve_utils.py

+        except exceptions.CommandError as e:
+            logger.info('Failed to download the logs: '
+                        f'{common_utils.format_exception(e)}')


Lets directly raise those errors. we should not transparently let the error happen

root-hbx · 2024-10-24T01:41:46Z

Thanks for the comments @cblmemo, fixed and ready for next round :)

cblmemo · 2024-10-28T22:18:48Z

sky/serve/core.py

+def _sync_log_file(runner,
+                   source,
+                   target_directory,
+                   target_name=None,
+                   run_timestamp=None) -> None:


Lets add type annotation here

cblmemo · 2024-10-28T22:23:13Z

sky/serve/core.py

+        single_file_synced = True
+        target_directory = os.path.join(target_directory, log_file_constant)
+
+    return runner, target_directory, single_file_synced


Why do we need to construct runner inside this function?

cblmemo · 2024-10-28T22:25:32Z

sky/serve/core.py

+    _sync_log_file(runner,
+                   remote_service_dir_name,
+                   sky_logs_directory,
+                   run_timestamp=run_timestamp)


Is it possible that we prepare all the logs (the replica log, controller log & lb log), construct all targets to download, and loop them once, and then cleanup?

cblmemo · 2024-10-28T22:26:15Z

sky/serve/core.py

+def _sync_down_replica_logs(service_name, run_timestamp, replica_id,
+                            controller_handle, sky_logs_directory) -> None:
+    """Helper function of sync_down_logs.
+        - prepare and download replica log file.
+    """


Coupled with the comment before, lets inline this function.

cblmemo · 2024-10-28T22:37:00Z

sky/serve/core.py

+    """
+    try:
+        subprocess_utils.handle_returncode(returncode, command, error_message)
+        logger.info(f'{error_message.split(".")[0]} successfully.')


Why do we need to print out error message if runs successfully?>

cblmemo · 2024-10-28T22:37:12Z

sky/serve/core.py

+        'download on the controller.')
+
+
+def _run_command_with_error_handling(returncode, command,


This function feels too shallow for me. Lets inline this.

cblmemo · 2024-10-28T22:39:24Z

sky/serve/serve_utils.py

+def sync_normal_replica_logs(service_name: str, replica: dict,
+                             dir_for_download: str,
+                             target_replica_id: Optional[int]) -> None:


Not sure what is a "normal" replica. Do you mean nonterminal replica?
Also, this function is only called once. Lets inline it to reduce fragmentation.

Yes, it means nonterminal replica here

In fact, I'm a little bit confused about the relationship between inlining and redundancy here. If all these helper functions above are put back in their original place, it will make the main function sync_down_logs() / prepare_replica_logs_for_download() quite long and the logic rather complex. I'm not sure if I should inline them?

root-hbx · 2024-10-30T06:18:42Z

Thanks for the detailed suggestions! @cblmemo

Before I proceed with further changes, I'd like to confirm a few details:

Inlining functions that are only used once, even though it may make the main function somewhat longer.
For _sync_log_file(), adding type annotations, preparing all logs (the replica log, controller log, and lb log), constructing all download targets, looping through them once, and then performing cleanup.

Last week, you mentioned there was redundancy elsewhere, but I didn't fully understand, I encapsulated many processes into functions to simplify the logic of the main function. In fact, after handling it this way, I didn't find any other redundancies.

If I proceed with the changes as described, actually, I’d only need to reset the current PR back to here and then modify the _sync_log_file() function based on that.

Would any redundancy remain if I proceed this way? I would truly appreciate any insights and guidance provided.

cblmemo · 2024-10-31T06:04:05Z

Thanks for asking @root-hbx ! Those are good questions.

General ideas to keep in mind:

Define a function for repeatedly used code blocks.
Avoid very shallow functions (e.g. this), except that you are intentionally keeping everything aligned (e.g. accessing the same file path in multiple places in the code).

Several suggestions:

Merging if branches with the same conditions (especially when the condition is long). e.g. both for (caller of the _prepare_and_sync_log function)[https://github.com/root-hbx/skypilot/blob/6f83693b6ffda98589035405f44972c831312214/sky/serve/core.py#L837-L842] and in the function itself, we are checking the component (which is a very lengthy condition). Is it possible to construct the log file name outside the function and pass it in?
Could we double-check in what case it will happen? e.g. here, in what case would those replica log not exist?

Also, could you please add type annotation when writing the code? It will help the type checker to check your code :))

root-hbx · 2024-10-31T22:04:51Z

Thanks for your detailed advice and guidance @cblmemo . That's super helpful! I am currently refactoring the code according to the aforementioned standards :D

root-hbx · 2024-11-02T04:04:37Z

Hi, @cblmemo ! I’ve refactored the corresponding code based on your feedback and enhanced the unit test for serve_utils. PTAL when you have a moment 🤩

David Tran and others added 30 commits January 31, 2024 18:35

[skyserve] sync down controller logs

40540b2

[skyserve] sync down load balancer logs

d9653af

[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

[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

use sync down logs directly

04a0b37

move sky serve controller constant back to serve_utils

e02b290

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

add assert and fix nit

bf443e2

cblmemo reviewed Oct 23, 2024

View reviewed changes

root-hbx added 3 commits October 23, 2024 17:41

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

fc11824

fix logging issues

ab7ec44

rsync all in loop and avoid interfere from terminal replicas

66b6638

root-hbx added 2 commits October 26, 2024 16:24

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

dfb2bef

refactor core::sync_down_logs load_balancer/controller

b197fc0

root-hbx requested a review from cblmemo October 27, 2024 18:50

cblmemo reviewed Oct 28, 2024

View reviewed changes

root-hbx added 2 commits October 30, 2024 00:08

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

9c2ee4f

recover and fix slightly for serve_utils

70a7bd6

root-hbx force-pushed the serve-sync-log branch from 784c764 to 70a7bd6 Compare October 31, 2024 00:46

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

6f83693

refactor skyserver::core

3af2cf1

root-hbx added 10 commits October 31, 2024 16:23

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

00d7caf

add comments of skyserver::core

51f7b93

nits fixed in skyserve::serve_state

5b51fc4

fix serve_utils logic issues

32ceb00

refactor serve_utils

db504e5

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

5f3e438

add comments

d0a8ab7

refactor serve_utils and simplify its logic

785da4c

refactor test_serve_utils in unit_tests

c0ffd0e

nits fixed in unittests

3cfa688

Merge remote-tracking branch 'skypilot/master' into serve-sync-log

50bb6f3

andylizf mentioned this pull request Feb 9, 2025

[Serve] Support SkyServe Logs Sync-Down #4682

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serve] A patch for sync down logs #4036

[Serve] A patch for sync down logs #4036

root-hbx commented Oct 5, 2024 •

edited

Loading

cblmemo Oct 23, 2024

root-hbx Oct 24, 2024

cblmemo Oct 26, 2024

cblmemo Oct 23, 2024

cblmemo Oct 23, 2024

cblmemo Oct 23, 2024

root-hbx commented Oct 24, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

cblmemo Oct 28, 2024

root-hbx Oct 28, 2024

root-hbx commented Oct 30, 2024

cblmemo commented Oct 31, 2024

root-hbx commented Oct 31, 2024

root-hbx commented Nov 2, 2024

		'download on the controller.')


		def _run_command_with_error_handling(returncode, command,

[Serve] A patch for sync down logs #4036

Are you sure you want to change the base?

[Serve] A patch for sync down logs #4036

Conversation

root-hbx commented Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

root-hbx commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

root-hbx commented Oct 30, 2024

cblmemo commented Oct 31, 2024

root-hbx commented Oct 31, 2024

root-hbx commented Nov 2, 2024

root-hbx commented Oct 5, 2024 •

edited

Loading