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] fix logging error on passing traceback object into exc_info #46105

Merged
merged 2 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions python/ray/serve/_private/logging_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,23 +100,22 @@ def format(self, record: logging.LogRecord) -> str:
The formatted log record in json format.
"""
record_format = copy.deepcopy(self.component_log_fmt)
record_attributes = copy.deepcopy(record.__dict__)
Copy link
Contributor

Choose a reason for hiding this comment

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

no idea why this was here in the first place...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel it's probably a copy pasta from the line above 😅

record_format[SERVE_LOG_LEVEL_NAME] = record.levelname
record_format[SERVE_LOG_TIME] = self.asctime_formatter.format(record)

for field in ServeJSONFormatter.ADD_IF_EXIST_FIELDS:
if field in record_attributes:
record_format[field] = record_attributes[field]
if field in record.__dict__:
record_format[field] = record.__dict__[field]

record_format[SERVE_LOG_MESSAGE] = self.message_formatter.format(record)

if SERVE_LOG_EXTRA_FIELDS in record_attributes:
if not isinstance(record_attributes[SERVE_LOG_EXTRA_FIELDS], dict):
if SERVE_LOG_EXTRA_FIELDS in record.__dict__:
if not isinstance(record.__dict__[SERVE_LOG_EXTRA_FIELDS], dict):
raise ValueError(
f"Expected a dictionary passing into {SERVE_LOG_EXTRA_FIELDS}, "
f"but got {type(record_attributes[SERVE_LOG_EXTRA_FIELDS])}"
f"but got {type(record.__dict__[SERVE_LOG_EXTRA_FIELDS])}"
)
for k, v in record_attributes[SERVE_LOG_EXTRA_FIELDS].items():
for k, v in record.__dict__[SERVE_LOG_EXTRA_FIELDS].items():
if k in record_format:
raise KeyError(f"Found duplicated key in the log record: {k}")
record_format[k] = v
Expand Down
4 changes: 2 additions & 2 deletions python/ray/serve/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ class LoggingConfig(BaseModel):
from ray import serve
from ray.serve.schema import LoggingConfig
# Set log level for the deployment.
@serve.deployment(LoggingConfig(log_level="DEBUG")
@serve.deployment(LoggingConfig(log_level="DEBUG"))
class MyDeployment:
def __call__(self) -> str:
return "Hello world!"
# Set log directory for the deployment.
@serve.deployment(LoggingConfig(logs_dir="/my_dir")
@serve.deployment(LoggingConfig(logs_dir="/my_dir"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a doc change, since I saw it while reading it. Unrelated to the actual bug.

class MyDeployment:
def __call__(self) -> str:
return "Hello world!"
Expand Down
40 changes: 40 additions & 0 deletions python/ray/serve/tests/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -832,5 +832,45 @@ def test_stream_to_logger():
stream_to_logger.i_dont_exist


@pytest.mark.skipif(sys.platform == "win32", reason="Fail to create temp dir.")
@pytest.mark.parametrize(
"ray_instance",
[
{"RAY_SERVE_LOG_TO_STDERR": "0"},
],
indirect=True,
)
def test_json_logging_with_unpickleable_exc_info(
serve_and_ray_shutdown, ray_instance, tmp_dir
):
"""Test the json logging with unpickleable exc_info.

exc_info field is often used to log the exception stack trace. However, we had issue
where deepcopy is applied to traceback object from exc_info which is not pickleable
and caused logging error.

See: https://github.com/ray-project/ray/issues/45912
"""
logs_dir = Path(tmp_dir)
logging_config = LoggingConfig(encoding="JSON", logs_dir=str(logs_dir))
logger = logging.getLogger("ray.serve")

@serve.deployment(logging_config=logging_config)
class App:
def __call__(self):
try:
raise Exception("fake_exception")
except Exception as e:
logger.info("log message", exc_info=e)
return "foo"

serve.run(App.bind())
requests.get("http://127.0.0.1:8000/")
for log_file in os.listdir(logs_dir):
with open(logs_dir / log_file) as f:
assert "Logging error" not in f.read()
assert "cannot pickle" not in f.read()


if __name__ == "__main__":
sys.exit(pytest.main(["-v", "-s", __file__]))
Loading