-
Notifications
You must be signed in to change notification settings - Fork 471
add option for log_file to setup_logging
#723
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
base: main
Are you sure you want to change the base?
Conversation
| if log_file is not None: | ||
| handler = logging.FileHandler(log_file) | ||
| else: | ||
| handler = logging.StreamHandler(sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileHandler not closed before clearing handlers
Low Severity
When setup_logging is called multiple times with log_file specified, logger.handlers.clear() removes the old handler from the list without calling handler.close() first. With the previous StreamHandler(sys.stderr) this was benign since stderr is always open, but with the new FileHandler this leaks file descriptors. Each subsequent call leaves the previous log file handle open.
Additional Locations (1)
mikasenghaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you envision each env worker calling setup_logging with a diff logfile? if so, im wondering if this should not rather live in prime-rl because it seems quite prime-rl specific
also i think setup_logging is called upon import by vf iirc, so we would have multiple calls to this func (once upon import, and then once from the env worker to set it up). wondering if its not just easier to pipe process stdout/stderr to file but maybe missing smth
|
|
||
| # Create a StreamHandler that writes to stderr | ||
| handler = logging.StreamHandler(sys.stderr) | ||
| # Create handler - file or stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we not rather support console + file? ie. file logging is an add-on, not a replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then it defies the purpose of keeping orchestrator logs clean via files.
i would then just handle streams in prime-rl natively.
https://github.com/PrimeIntellect-ai/prime-rl/pull/1561/changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can also do
vf.setup_logging(level=vf_log_level.upper())
# Redirect verifiers to file instead of inherited stderr
vf_logger = logging.getLogger("verifiers")
vf_logger.handlers.clear()
vf_logger.addHandler(logging.FileHandler(log_file))
in prime-rl. thought it was cleaner to have it live in vf.
Description
adds option to write logs to file by passing
log_filetosetup_loggingType of Change
Testing
uv run pytestlocally.Checklist
Additional Notes
Note
Logging enhancement
vf.setup_logging(level="INFO", log_file: str | None = None)to acceptlog_file; when provided, useFileHandlerelse default toStreamHandler(stderr)docs/reference.mdto reflect new signature and behaviorWritten by Cursor Bugbot for commit 8805864. This will update automatically on new commits. Configure here.