update WorkforceLogger with log_message #3485
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zshanhui
changed the title
zshanhui
changed the title
| print(event.message) | ||
| color = _COLOR_MAP.get(event.level) | ||
| colored_message = f"{color}{event.message}{Fore.RESET}" | ||
| print(colored_message) |
There was a problem hiding this comment.
if we use the logger.info/warning/error. module here, the info and success levels does not log to the console out which changes the original behavior of the print statements. do we want to use logger instead?
camel/societies/workforce/events.py
Outdated
| class WorkforceEventBase(BaseModel): | ||
| model_config = ConfigDict(frozen=True, extra='forbid') | ||
| event_type: Literal[ | ||
| "message_log", |
There was a problem hiding this comment.
Use log instead of message_log for naming consistency
- task_created -> TaskCreatedEvent
- log -> LogEvent
camel/societies/workforce/events.py
Outdated
| class LogEvent(WorkforceEventBase): | ||
| event_type: Literal["message_log"] = "message_log" | ||
| message: str | ||
| level: Literal["info", "warning", "error", "success"] |
There was a problem hiding this comment.
I recommend defining the level parameter with the common logging levels: debug/info/warning/error/critical. each level has a default style
Additionally, the parameter style: Literal["blue", "green", "gray", "xxx" ] | None = None should be added to allow for developer customization.
| if event.level == 'debug': | ||
| logger.debug(colored_message) | ||
| if event.level == 'info': | ||
| logger.info(colored_message) |
There was a problem hiding this comment.
note: the debug and info logs don't show on console out due to minimum=WARNING
…ging-via-callbacks-2nd
|
i've updated the PR to address the comments and changed to use logger module |
| def _get_color_message(self, event: LogEvent) -> str: | ||
| r"""Gets a colored message for a log event.""" | ||
| if event.color is None or event.color not in _COLOR_MAP: | ||
| return event.message | ||
| color = _COLOR_MAP.get(event.color) | ||
| return f"{color}{event.message}{Fore.RESET}" |
There was a problem hiding this comment.
Please move _get_color_message and _COLOR_MAP into WorkforceCallback. Also rename _COLOR_MAP to __COLOR_MAP to mark it as a private attribute
| color = _COLOR_MAP.get(event.color) | ||
| return f"{color}{event.message}{Fore.RESET}" | ||
|
|
||
| def log_message(self, event: LogEvent) -> None: |
There was a problem hiding this comment.
For the default WorkforceLogger, we should keep using print to ensure refactoring doesn’t change the output behavior.
2 participants
Description
Describe your changes in detail (optional if the linked issue already contains a detailed description of the changes).
Fixes #3351
This PR moves the print logs to WorkforceLogger callback, add abstract method log_message to WorkforceCallback. Replaces a couple print statements for demonstration and plan to replace the remaining print statements in a followup PR.
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!