[core] Use pipe to redirect stdout #49548

dentiny · 2025-01-02T22:23:33Z

A followup for oversized logging file. It supports rotated logging file via overriding stdout and stderr with logging module.

High-level abstract:

Application streams to pipefd write
Background threads poll pipefd read side and continues streaming the target file via logging

Update: This PR only does rotation for stdout files; as previous, stderr are less important compared with stdout, and much trickier to get right because more unit tests are listening to the stderr.

Closes #49563

Signed-off-by: dentiny <[email protected]>

python/ray/_private/log_rotation_with_pipe.py

python/ray/_private/ray_logging/__init__.py

Signed-off-by: dentiny <[email protected]>

jjyao · 2025-01-03T06:22:36Z

python/ray/_private/log_rotation_with_pipe.py

+class PipeStreamWriteHandle(IO[AnyStr]):
+    def __init__(self, write_fd: int, log_name: str):
+        self.name = log_name
+        self.write_fd = write_fd
+        self.stream = os.fdopen(write_fd, "w")
+
+    def name(self):
+        return self.name
+
+    def fileno(self):
+        return self.write_fd
+
+    def write(self, s: str) -> int:
+        return self.stream.write(s)
+
+    def flush(self):
+        self.stream.flush()
+
+    def close(self):
+        self.stream.close()
+
+    def isatty(self) -> bool:
+        return False
+
+    def readable(self) -> bool:
+        return False
+
+    def writable(self) -> bool:
+        return True
+
+    def seekable(self) -> bool:
+        return False
+


configure_log_file only requires this object has fileno() method not others?

write, flush, name, fileno is definitely needed --- I have met issues without any of them;
Others are suggested by gpt, didn't verify one by one.

My personal opinion is it doesn't hurt to implement these overloads;

All these overload functions are straightforward and short enough

Some attribute function help explain the class, i.e. readable and writable.

dentiny · 2025-01-03T08:50:06Z

There're CI failure, I will take a look at daytime.

MengjinYan

Thanks!!

Also, a minor general comment: probably we should also update the wiki page here

ray/doc/source/ray-observability/user-guides/configure-logging.md

Line 460 in 526fad6

    
           Ray supports log rotation of log files. Note that not all components support log rotation. (Raylet, Python, and Java worker logs do not rotate).

to indicate the change.

python/ray/_private/log_rotation_with_pipe.py

Signed-off-by: dentiny <[email protected]>

doc/source/ray-observability/user-guides/configure-logging.md

jjyao · 2025-01-07T06:14:50Z

python/ray/_private/log_rotation_with_pipe.py

+
+    atexit.register(cleanup)
+
+    return pipe_write_stream


Do we need the PipeStreamWriteHandle wrapper, can we directly return open(write_fd)?

open takes a path-like object, not a fd
Ref: https://docs.python.org/3/library/functions.html#open

fdopen is the interface we want to wrap fd into a file object
Ref: https://docs.python.org/3/library/os.html#os.fdopen

Can we use fdopen then?

How to you plan to provide name and write then?

I see. write() I think is supported by the object returned by fdopen()? The issue is name then. I guess you can do

file_like_io_object = os.fdopen(write_fd) file_like_io_object.name = "xxxx"

Just tried

Traceback (most recent call last): File "logging_redirection_test.py", line 13, in <module> _LOG_STDOUT_FNAME, _ROTATION_MAX_BYTE, _ROTATION_FILE_NUM) File "/home/ubuntu/py-logging-redirection/pipe_logging.py", line 90, in open_pipe_with_rotation pipe_write_stream.name = fname AttributeError: attribute 'name' of '_io.TextIOWrapper' objects is not writable

Code snippet:

pipe_write_stream = os.fdopen(write_fd, "w") pipe_write_stream.name = fname

Seems we can do https://stackoverflow.com/questions/60622854/how-to-instantiate-an-io-textiowrapper-object-with-a-name-attribute so we don't need to implement our own IO subclass with all these methods.

Signed-off-by: dentiny <[email protected]>

jjyao · 2025-01-08T17:35:31Z

doc/source/ray-observability/user-guides/configure-logging.md


-By default, logs rotate when they reach 512MB (maxBytes), and have a maximum of five backup files (backupCount). Indexes are appended to all backup files (e.g., `raylet.out.1`)
-To change the log rotation configuration, specify environment variables. For example,
+By default, ray doesn't rotate for internal components. To change the log rotation configuration, specify environment variables. For example,


There are some files that we have rotation by default? What are they?

jjyao · 2025-01-08T17:38:52Z

python/ray/_private/log_rotation_with_pipe.py

+                with cond:
+                    # An empty line is read over, which indicates write side has
+                    # closed.
+                    if line == _EOF_TOKEN:


why do we need _EOF_TOKEN? If you close the pipe, reader will know? readline() should return empty string to indicate EOF.

jjyao · 2025-01-08T17:40:44Z

python/ray/_private/log_rotation_with_pipe.py

+    read_thread = threading.Thread(target=read_log_content_from_pipe, daemon=True)
+    dump_thread = threading.Thread(target=dump_log_content_to_buffer, daemon=True)


Can you give these two threads name?

jjyao · 2025-01-08T17:51:34Z

python/ray/_private/workers/default_worker.py

-    "log file, default is "
-    f"{ray_constants.LOGGING_ROTATE_BYTES} bytes.",
+    default=sys.maxsize,
+    help="Specify the max bytes for rotating " "log file, default no rotation.",


Suggested change

help="Specify the max bytes for rotating " "log file, default no rotation.",

help="Specify the max bytes for rotating log file, default no rotation.",

jjyao · 2025-01-08T17:54:51Z

python/ray/_private/log_rotation_with_pipe.py

+            logger.info(cur_content)
+
+    # Python `atexit` hook only invokes when no other non-daemon threads running.
+    read_thread = threading.Thread(target=read_log_content_from_pipe, daemon=True)


Actually thinking more, I think it's better to implement these in C++ so that the same implementation can be used for all language frontends (e.g. python , java, c++). The current way only works for python workers. I think implementing in C++ should be too much harder?

OK, I will create another PR implemented in C++, I will try to get it out before tomorrow.

dentiny requested review from jjyao and rynewang January 2, 2025 22:23

dentiny mentioned this pull request Jan 2, 2025

[core] Use rotation log file for application logging #49544

Closed

use pipe to redirect stdout/stderr

2dad6b1

Signed-off-by: dentiny <[email protected]>

dentiny force-pushed the hjiang/pipe-application-log branch from e2f2050 to 2dad6b1 Compare January 2, 2025 22:34

dentiny added the go add ONLY when ready to merge, run all tests label Jan 3, 2025

jjyao reviewed Jan 3, 2025

View reviewed changes

python/ray/_private/log_rotation_with_pipe.py Outdated Show resolved Hide resolved

python/ray/_private/log_rotation_with_pipe.py Outdated Show resolved Hide resolved

python/ray/_private/ray_logging/__init__.py Outdated Show resolved Hide resolved

dentiny assigned jjyao and rynewang Jan 3, 2025

dentiny added 2 commits January 3, 2025 06:07

more comments on double thread

9ddccdb

Signed-off-by: dentiny <[email protected]>

remove unused function

2160e0f

Signed-off-by: dentiny <[email protected]>

dentiny requested a review from jjyao January 3, 2025 06:11

reuse old logger and give it a name

3129f18

Signed-off-by: dentiny <[email protected]>

dentiny force-pushed the hjiang/pipe-application-log branch from b3ef8de to 3129f18 Compare January 3, 2025 06:18

jjyao reviewed Jan 3, 2025

View reviewed changes

dentiny requested a review from jjyao January 3, 2025 06:33

MengjinYan mentioned this pull request Jan 3, 2025

[Core] Log rotation on workers #49563

Open

MengjinYan reviewed Jan 3, 2025

View reviewed changes

python/ray/_private/log_rotation_with_pipe.py Outdated Show resolved Hide resolved

allow multiple handlers

5646d6f

Signed-off-by: dentiny <[email protected]>

dentiny requested a review from MengjinYan January 4, 2025 02:25

update readme

687a1cd

Signed-off-by: dentiny <[email protected]>

dentiny requested a review from a team as a code owner January 6, 2025 19:03

fix dtor

5c9d352

Signed-off-by: dentiny <[email protected]>

dentiny changed the title ~~[core] Use pipe to redirect stdout/stderr~~ [core] Use pipe to redirect stdout Jan 7, 2025

only rotate stdout

9f9c4b5

Signed-off-by: dentiny <[email protected]>

jjyao reviewed Jan 7, 2025

View reviewed changes

dentiny added 3 commits January 7, 2025 06:43

optimization on non-rotation case

9b89762

Signed-off-by: dentiny <[email protected]>

by default no rotation to keep current behavior

1f93a4f

Signed-off-by: dentiny <[email protected]>

fix rotation behavior doc

cb9ac3d

Signed-off-by: dentiny <[email protected]>

dentiny requested a review from jjyao January 7, 2025 07:03

dentiny added 2 commits January 7, 2025 07:40

no rotation testing

a390078

Signed-off-by: dentiny <[email protected]>

Merge branch 'master' into hjiang/pipe-application-log

ce368c3

jjyao reviewed Jan 8, 2025

View reviewed changes

dentiny closed this Jan 9, 2025

dentiny mentioned this pull request Jan 9, 2025

[core] pipe logging in C++ #49733

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Use pipe to redirect stdout #49548

[core] Use pipe to redirect stdout #49548

dentiny commented Jan 2, 2025 •

edited by jjyao

Loading

jjyao Jan 3, 2025

dentiny Jan 3, 2025

dentiny commented Jan 3, 2025

MengjinYan left a comment

jjyao Jan 7, 2025

dentiny Jan 7, 2025

dentiny Jan 7, 2025

jjyao Jan 7, 2025

dentiny Jan 7, 2025

jjyao Jan 7, 2025

dentiny Jan 7, 2025

dentiny Jan 7, 2025

jjyao Jan 8, 2025

jjyao Jan 8, 2025

jjyao Jan 8, 2025

jjyao Jan 8, 2025

jjyao Jan 8, 2025

jjyao Jan 8, 2025

dentiny Jan 9, 2025

		read_thread = threading.Thread(target=read_log_content_from_pipe, daemon=True)
		dump_thread = threading.Thread(target=dump_log_content_to_buffer, daemon=True)

	help="Specify the max bytes for rotating " "log file, default no rotation.",
	help="Specify the max bytes for rotating log file, default no rotation.",

[core] Use pipe to redirect stdout #49548

[core] Use pipe to redirect stdout #49548

Conversation

dentiny commented Jan 2, 2025 • edited by jjyao Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dentiny commented Jan 3, 2025

MengjinYan 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

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

dentiny commented Jan 2, 2025 •

edited by jjyao

Loading