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.

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

[serve] configure logging for build app task #46347

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

zcin
Copy link
Contributor

@zcin zcin commented Jun 30, 2024

[serve] configure logging for build app task

Configure logging for the build_serve_application task. Although it is a short-lived task, it is part of every application deployment process, and this can improve observability for the deployment process.
Added some light logging to the task as well.
Example:

INFO 2024-07-01 16:54:13,660 build_default_task 15893 application_state.py:1047 - Importing application 'default'.
WARNING 2024-07-01 16:54:13,660 build_default_task 15893 api.py:432 - The default value for `max_ongoing_requests` has changed from 100 to 5 in Ray 2.32.0.
ERROR 2024-07-01 16:54:13,661 build_default_task 15893 application_state.py:1077 - Exception importing application 'default'.
Traceback (most recent call last):
  File "/Users/cindyz/ray/python/ray/serve/_private/application_state.py", line 1049, in build_serve_application
    app = call_app_builder_with_args_if_necessary(import_attr(import_path), args)
  File "/Users/cindyz/ray/python/ray/_private/utils.py", line 1194, in import_attr
    return getattr(module, attr_name)
AttributeError: module 'hello' has no attribute 'haa'

Signed-off-by: Cindy Zhang cindyzyx9@gmail.com

@zcin zcin force-pushed the pr46347 branch 3 times, most recently from 2f561cc to f6c89eb Compare July 2, 2024 16:19
@zcin zcin marked this pull request as ready for review July 2, 2024 16:25
@zcin zcin requested review from GeneDer and edoakes July 2, 2024 16:26
Returns:
Deploy arguments: a list of deployment arguments if application
was built successfully, otherwise None.
Error message: a string if an error was raised, otherwise None.
"""
configure_component_logger(
component_name=f"build_{name}_task",
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern is this component name. Currently in Serve we have controller, proxy, and replica. Can definitely add a new type for this, but given this is free formed, it might be hard for users to find them on the platform. Also, depending on whether we want to show these by default, we might want to follow up with a product change to add a new component name filter in the log viewer UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

synced offline, setting component name to "controller" and setting component id to build_default_pid. Since the component name is used for filtering logs, creating a new component name for the build task is confusing and hard to find. It makes the most sense to group it under controller logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like the right approach to me too. we can always iterate

@zcin zcin force-pushed the pr46347 branch 2 times, most recently from 1c3f6a9 to c09bd44 Compare July 2, 2024 21:35
Copy link
Contributor

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

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

LGTM!

@zcin zcin added the go add ONLY when ready to merge, run all tests label Jul 3, 2024
Configure logging for the `build_serve_application` task. Although it is a short-lived task, it is part of every application deployment process, and this can improve observability for the deployment process.
Added some light logging to the task as well.
Example:
```
INFO 2024-07-01 16:54:13,660 build_default_task 15893 application_state.py:1047 - Importing application 'default'.
WARNING 2024-07-01 16:54:13,660 build_default_task 15893 api.py:432 - The default value for `max_ongoing_requests` has changed from 100 to 5 in Ray 2.32.0.
ERROR 2024-07-01 16:54:13,661 build_default_task 15893 application_state.py:1077 - Exception importing application 'default'.
Traceback (most recent call last):
  File "/Users/cindyz/ray/python/ray/serve/_private/application_state.py", line 1049, in build_serve_application
    app = call_app_builder_with_args_if_necessary(import_attr(import_path), args)
  File "/Users/cindyz/ray/python/ray/_private/utils.py", line 1194, in import_attr
    return getattr(module, attr_name)
AttributeError: module 'hello' has no attribute 'haa'
```


Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
@zcin
Copy link
Contributor Author

zcin commented Jul 16, 2024

@edoakes tests are passing, is this ready to merge?

@edoakes edoakes merged commit aafc7a6 into ray-project:master Jul 16, 2024
5 checks passed
@zcin zcin deleted the pr46347 branch August 21, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants