Skip to content

fix(logger): to support logger for multiple runs through port assignment#71

Merged
BinWang28 merged 3 commits intomiroflow-v0.3from
fix-logger
Oct 11, 2025
Merged

fix(logger): to support logger for multiple runs through port assignment#71
BinWang28 merged 3 commits intomiroflow-v0.3from
fix-logger

Conversation

@BinWang28
Copy link
Member

as titled

Checklist for PR

  • Write a descriptive PR title following the Angular commit message format: <type>(<scope>): <subject>

    • Examples: feat(agent): add pdf tool via mcp, perf: make llm client async, fix(utils): load custom config via importlib
    • Valid types: feat, fix, docs, style, refactor, perf, test, build, ci, revert
    • The check-pr-title CI job will validate your title format
    • Bad title examples and why they fail:
      • Update README ❌ Missing type and colon
      • feat add new feature ❌ Missing colon after type
      • Feature: add new tool ❌ Invalid type (should be feat)
      • feat(Agent): add tool ❌ Scope should be lowercase
      • feat(): add tool ❌ Empty scope not allowed
      • feat(my_scope): add tool ❌ Underscores not allowed in scope
      • feat(my space): add tool ❌ Space not allowed in scope
      • feat(scope):add tool ❌ Missing space after colon
      • feat(scope): ❌ Empty subject
  • Run lint and format locally:

    • uv tool run ruff@0.8.0 check --fix .
    • uv tool run ruff@0.8.0 format .
    • CI job lint enforces ruff default format/lint rules on all new codes.

@BinWang28 BinWang28 requested a review from Copilot October 11, 2025 03:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the logger to support multiple concurrent runs by implementing dynamic port assignment for ZMQ connections. Instead of using a fixed port that could cause conflicts, the system now automatically finds available ports and manages the ZMQ address globally.

  • Adds dynamic port discovery to prevent port conflicts between multiple logger instances
  • Implements global ZMQ address management with getter/setter functions
  • Adds graceful fallback handling when ZMQ connections fail

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

return actual_addr
except RuntimeError:
# Fallback to random port
return sock.bind_to_random_port("tcp://127.0.0.1")
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

The bind_to_random_port method returns a port number, not an address string. This should be port = sock.bind_to_random_port('tcp://127.0.0.1') followed by return f'tcp://127.0.0.1:{port}' to maintain consistent return type.

Suggested change
return sock.bind_to_random_port("tcp://127.0.0.1")
port = sock.bind_to_random_port("tcp://127.0.0.1")
return f"tcp://127.0.0.1:{port}"

Copilot uses AI. Check for mistakes.
Comment on lines +87 to +91
print(f"ZMQ handler connected to: {addr}")
except zmq.error.ZMQError as e:
# If connection fails, disable the handler
print(f"Warning: Could not connect to ZMQ listener at {addr}: {e}")
print("Disabling ZMQ logging for this handler")
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

Direct print() statements should be replaced with proper logging calls. Use logger.info() for connection messages and logger.warning() for connection failures to maintain consistent logging practices.

Suggested change
print(f"ZMQ handler connected to: {addr}")
except zmq.error.ZMQError as e:
# If connection fails, disable the handler
print(f"Warning: Could not connect to ZMQ listener at {addr}: {e}")
print("Disabling ZMQ logging for this handler")
logging.info(f"ZMQ handler connected to: {addr}")
except zmq.error.ZMQError as e:
# If connection fails, disable the handler
logging.warning(f"Could not connect to ZMQ listener at {addr}: {e}")
logging.warning("Disabling ZMQ logging for this handler")

Copilot uses AI. Check for mistakes.
# Bind to available port
actual_addr = _bind_zmq_socket(sock, bind_addr)
set_zmq_address(actual_addr)
print(f"ZMQ listener bound to: {actual_addr}")
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

Direct print() statements should be replaced with proper logging calls. Use logger.info() for connection messages and logger.warning() for connection failures to maintain consistent logging practices.

Suggested change
print(f"ZMQ listener bound to: {actual_addr}")
root_logger.info(f"ZMQ listener bound to: {actual_addr}")

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +101
if self.sock is not None:
msg = f"{record.getMessage()}"
self.sock.send_string(f"{self.task_id}||{self.tool_name}||{msg}")
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

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

The emit method should return early when self.sock is None instead of nesting all the logging logic inside an if statement. This would improve readability and reduce indentation depth.

Suggested change
if self.sock is not None:
msg = f"{record.getMessage()}"
self.sock.send_string(f"{self.task_id}||{self.tool_name}||{msg}")
if self.sock is None:
return
msg = f"{record.getMessage()}"
self.sock.send_string(f"{self.task_id}||{self.tool_name}||{msg}")

Copilot uses AI. Check for mistakes.
@BinWang28 BinWang28 merged commit 08fd95a into miroflow-v0.3 Oct 11, 2025
3 checks passed
@BinWang28 BinWang28 deleted the fix-logger branch October 11, 2025 03:20
Zhudongsheng75 pushed a commit to open-compass/MiroFlow that referenced this pull request Dec 27, 2025
…ent (MiroMindAI#71)

* to pass lint

* accomodate copilot suggests

* pass lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants