[2/2][Serve] Support dynamic log #40735

sihanwang41 · 2023-10-27T05:05:38Z

Why are these changes needed?

Follow up with the pr to implement the dynamic logging.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Sihan Wang <[email protected]>

python/ray/serve/_private/application_state.py

python/ray/serve/_private/controller.py

Signed-off-by: Sihan Wang <[email protected]>

edoakes · 2023-11-07T21:41:18Z

Update PR description please

edoakes · 2023-11-07T21:43:14Z

python/ray/serve/_private/application_state.py

+ # When there is no deployment entries and app logging config is set,
+ # apply app logging config to all existing deployment logging config.
+ if not deployment_override_options and app_logging_config:
+ for deployment in list(deployment_infos.values()):
+ deployment.deployment_config.logging_config = app_logging_config


I think this logic is not comprehensive. If there are deployment_override_options, but the list is not complete, then the deployments not in the list will not have their logging config overridden. Am I correct?

Updated logic:

if application logging config is set, apply to all existing deployment config.

if deployment logging config is set, will rely on existing for loop to update the logging config for each deployment.

python/ray/serve/_private/config.py

edoakes · 2023-11-07T21:46:29Z

python/ray/serve/_private/controller.py

+ logger.debug(
+ "Configure the serve controller logger "
+ f"with logging config: {self.logging_config}"
+ )


Would make sure this only logs when the config is changed then make it info-level (it's useful)

Suggested change

logger.debug(

"Configure the serve controller logger "

f"with logging config: {self.logging_config}"

)

logger.debug(

f"Serve controller logging config updated to: {self.logging_config}."

)

python/ray/serve/_private/controller.py

python/ray/serve/tests/test_api.py

edoakes · 2023-11-07T21:57:07Z

python/ray/serve/tests/test_api.py

+ resp = requests.get("http:https://127.0.0.1:8000/")
+ assert resp.status_code == 200
+
+ def test_application_logging_overwrite(self, serve_instance):


please also add a test for overwriting per-deployment

done! Check test_application_logging_overwrite

python/ray/serve/schema.py

python/ray/serve/_private/logging_utils.py

python/ray/serve/schema.py

python/ray/serve/_private/application_state.py

zcin · 2023-11-08T00:37:22Z

python/ray/serve/_private/api.py

@@ -110,6 +111,7 @@ def _check_http_options(
 def _start_controller(
 http_options: Union[None, dict, HTTPOptions] = None,
 grpc_options: Union[None, dict, gRPCOptions] = None,
+ logging_config: Union[None, dict, LoggingConfig] = None,


Can we change this to system_logging_config just like how we have update_system_logging_config in the client? It's a bit confusing to see logging_config in three different places

Should we also change the config option in ServeDeploySchema? There's the same issue of the same option name being used in 3 places. Aware this is an API change, will leave it up to the team to decide.

Signed-off-by: Sihan Wang <[email protected]>

sihanwang41 · 2023-11-08T22:18:51Z

Hi @edoakes and @zcin , ptal, thx!

edoakes

Just one clarification then lgtm

python/ray/serve/_private/api.py

edoakes · 2023-11-08T22:28:32Z

python/ray/serve/_private/application_state.py

+ # If application loggin config is set, apply to all existing deployment config
+ if app_logging_config:
+ for _, info in deployment_infos.items():
+ info.deployment_config.logging_config = LoggingConfig(
+ **app_logging_config
+ ).dict()


Won't this override all deployments' logging config, even if there is one specified for the deployment? The desired behavior is being able to override deployment > application > system (this is standard practice, finest granularity should take precedence).

Hmm from the tests it looks like what I'm saying is already the case. I'm not following how this works because it looks like you're overriding all deployments' logging config here with the application one.

Apply the application logging config here.
And then we have the for loop to override the deployment config if set. https://github.com/ray-project/ray/blob/master/python/ray/serve/_private/application_state.py#L1020

So that deployment logging config will always be set lastly.

What if the user set logging_config in the code? It will always be overridden by the application level logging config.

That was my confusion as well

synced with Cindy offline, we apply the application config out of the function. So that we don't need to track whether setting in the deployment code or yaml file.

Got it. So this works as-is? Can you leave a comment describing this btw it's pretty hard to see from the code

I need to write a unit test to cover this case. Yeah, I will leave comments too!

python/ray/serve/tests/test_logging.py

Signed-off-by: Sihan Wang <[email protected]>

zcin

Some comments + 1 pending comment from above, otherwise LGTM

zcin · 2023-11-08T22:58:33Z

python/ray/serve/_private/application_state.py

+ # If application loggin config is set, apply to all existing deployment config
+ if app_logging_config:
+ for _, info in deployment_infos.items():
+ info.deployment_config.logging_config = LoggingConfig(
+ **app_logging_config
+ ).dict()


What if the user set logging_config in the code? It will always be overridden by the application level logging config.

python/ray/serve/_private/logging_utils.py

Signed-off-by: Sihan Wang <[email protected]>

zcin · 2023-11-09T17:37:31Z

python/ray/serve/_private/application_state.py

@@ -572,7 +573,18 @@ def _reconcile_target_deployments(self) -> None:

 # Set target state for each deployment
 for deployment_name, info in self._target_state.deployment_infos.items():
- self.apply_deployment_info(deployment_name, info)
+ deploy_info = deepcopy(info)


I think the logic itself looks good, just flagging that we will execute a deepcopy once per deployment for every control loop iteration. I think it would be more efficient to store a copy of this info object since it doesn't change between control loop iterations.

Signed-off-by: Sihan Wang <[email protected]>

sihanwang41 · 2023-11-09T22:27:40Z

@architkulkarni ready to merge, windows telemetry is flacky in master.

Follow up with the pr to implement the dynamic logging. --------- Signed-off-by: Sihan Wang <[email protected]>

sihanwang41 added 23 commits October 26, 2023 22:05

[2/2][Serve][WIP] Support dynamic log

25b023b

Signed-off-by: Sihan Wang <[email protected]>

Update

362d477

Signed-off-by: Sihan Wang <[email protected]>

Update

267b46b

Signed-off-by: Sihan Wang <[email protected]>

Update

71dfedb

Signed-off-by: Sihan Wang <[email protected]>

Update

b9eb2f0

Signed-off-by: Sihan Wang <[email protected]>

Fix

ab805a0

Signed-off-by: Sihan Wang <[email protected]>

Add tests

93be0bb

Signed-off-by: Sihan Wang <[email protected]>

Update

9ac17ce

Signed-off-by: Sihan Wang <[email protected]>

Update

9726258

Signed-off-by: Sihan Wang <[email protected]>

Update

488370a

Signed-off-by: Sihan Wang <[email protected]>

Update

1dd3482

Signed-off-by: Sihan Wang <[email protected]>

Update

323b960

Signed-off-by: Sihan Wang <[email protected]>

Fix lint

d1cb0e1

Signed-off-by: Sihan Wang <[email protected]>

Update

808d5bd

Signed-off-by: Sihan Wang <[email protected]>

Fix

0452bd4

Signed-off-by: Sihan Wang <[email protected]>

Fix test

24ebe71

Signed-off-by: Sihan Wang <[email protected]>

Update

d93de3a

Signed-off-by: Sihan Wang <[email protected]>

Fix

dc88ee5

Signed-off-by: Sihan Wang <[email protected]>

Update

3aff0bc

Signed-off-by: Sihan Wang <[email protected]>

Update

17a850e

Signed-off-by: Sihan Wang <[email protected]>

Update

3aeab7d

Signed-off-by: Sihan Wang <[email protected]>

Update

bc6ec9f

Signed-off-by: Sihan Wang <[email protected]>

Update

41eb578

Signed-off-by: Sihan Wang <[email protected]>

sihanwang41 force-pushed the dynamic_logging_2 branch from eeceda0 to 41eb578 Compare October 31, 2023 17:47

Update

890e839

Signed-off-by: Sihan Wang <[email protected]>

sihanwang41 marked this pull request as ready for review October 31, 2023 20:14

sihanwang41 changed the title ~~[2/2][Serve][WIP] Support dynamic log~~ [2/2][Serve] Support dynamic log Oct 31, 2023

edoakes reviewed Oct 31, 2023

View reviewed changes

sihanwang41 added 2 commits October 31, 2023 20:39

Update

5516fdf

Signed-off-by: Sihan Wang <[email protected]>

Update

f77e04b

Signed-off-by: Sihan Wang <[email protected]>

edoakes requested a review from a team November 7, 2023 21:40

edoakes reviewed Nov 7, 2023

View reviewed changes

zcin reviewed Nov 8, 2023

View reviewed changes

sihanwang41 added 9 commits November 7, 2023 21:25

Comments

9edfc77

Signed-off-by: Sihan Wang <[email protected]>

Address comments

a54e591

Signed-off-by: Sihan Wang <[email protected]>

Fix

cc0e544

Signed-off-by: Sihan Wang <[email protected]>

Update

5dc3cf2

Signed-off-by: Sihan Wang <[email protected]>

Update system logging config

7897441

Signed-off-by: Sihan Wang <[email protected]>

Update

fd1e649

Signed-off-by: Sihan Wang <[email protected]>

Update

fce9bff

Signed-off-by: Sihan Wang <[email protected]>

Update

8890145

Signed-off-by: Sihan Wang <[email protected]>

Update

fb1c28d

Signed-off-by: Sihan Wang <[email protected]>

edoakes reviewed Nov 8, 2023

View reviewed changes

sihanwang41 added 2 commits November 8, 2023 14:47

Update

e03a052

Signed-off-by: Sihan Wang <[email protected]>

Update description

564a8d8

Signed-off-by: Sihan Wang <[email protected]>

zcin reviewed Nov 8, 2023

View reviewed changes

Update

4905653

Signed-off-by: Sihan Wang <[email protected]>

edoakes approved these changes Nov 9, 2023

View reviewed changes

sihanwang41 added 4 commits November 8, 2023 19:33

Update

11523e9

Signed-off-by: Sihan Wang <[email protected]>

Update

3b59fa0

Signed-off-by: Sihan Wang <[email protected]>

Update

38ba1ac

Signed-off-by: Sihan Wang <[email protected]>

Merge branch 'master' into dynamic_logging_2

a0d8f95

zcin approved these changes Nov 9, 2023

View reviewed changes

sihanwang41 added 2 commits November 9, 2023 10:41

Fix java

e50ceff

Signed-off-by: Sihan Wang <[email protected]>

Merge branch 'master' into dynamic_logging_2

66aeeda

architkulkarni merged commit 00a1dcb into ray-project:master Nov 9, 2023
25 of 30 checks passed

ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Nov 29, 2023

[2/2][Serve] Support dynamic log (ray-project#40735)

fa64397

Follow up with the pr to implement the dynamic logging. --------- Signed-off-by: Sihan Wang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/2][Serve] Support dynamic log #40735

[2/2][Serve] Support dynamic log #40735

sihanwang41 commented Oct 27, 2023 •

edited

Loading

edoakes commented Nov 7, 2023

edoakes Nov 7, 2023

sihanwang41 Nov 8, 2023 •

edited

Loading

edoakes Nov 7, 2023

edoakes Nov 7, 2023

sihanwang41 Nov 8, 2023 •

edited

Loading

zcin Nov 8, 2023

sihanwang41 Nov 8, 2023

zcin Nov 8, 2023

sihanwang41 commented Nov 8, 2023

edoakes left a comment

edoakes Nov 8, 2023

edoakes Nov 8, 2023

sihanwang41 Nov 8, 2023

zcin Nov 8, 2023

edoakes Nov 8, 2023

sihanwang41 Nov 8, 2023

edoakes Nov 9, 2023

sihanwang41 Nov 9, 2023

zcin left a comment

zcin Nov 8, 2023

zcin Nov 9, 2023

sihanwang41 commented Nov 9, 2023

[2/2][Serve] Support dynamic log #40735

[2/2][Serve] Support dynamic log #40735

Conversation

sihanwang41 commented Oct 27, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

edoakes commented Nov 7, 2023

Choose a reason for hiding this comment

sihanwang41 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 Nov 8, 2023 • 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

sihanwang41 commented Nov 8, 2023

edoakes left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 commented Nov 9, 2023

sihanwang41 commented Oct 27, 2023 •

edited

Loading

sihanwang41 Nov 8, 2023 •

edited

Loading

sihanwang41 Nov 8, 2023 •

edited

Loading