-
Notifications
You must be signed in to change notification settings - Fork 32
Add cycle detection and control loop validation to concore validate #213
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: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,9 @@ | |
| import re | ||
| import zmq | ||
| import numpy as np | ||
| import atexit | ||
| import signal | ||
|
|
||
| logging.basicConfig( | ||
| level=logging.INFO, | ||
| format='%(levelname)s - %(message)s' | ||
|
|
@@ -72,6 +75,7 @@ def recv_json_with_retry(self): | |
|
|
||
| # Global ZeroMQ ports registry | ||
| zmq_ports = {} | ||
| _cleanup_in_progress = False | ||
|
|
||
| def init_zmq_port(port_name, port_type, address, socket_type_str): | ||
| """ | ||
|
|
@@ -98,12 +102,45 @@ def init_zmq_port(port_name, port_type, address, socket_type_str): | |
| logging.error(f"An unexpected error occurred during ZMQ port initialization for {port_name}: {e}") | ||
|
|
||
| def terminate_zmq(): | ||
| for port in zmq_ports.values(): | ||
| """Clean up all ZMQ sockets and contexts before exit.""" | ||
| global _cleanup_in_progress | ||
|
|
||
| if _cleanup_in_progress: | ||
| return # Already cleaning up, prevent reentrant calls | ||
|
|
||
| if not zmq_ports: | ||
| return # No ports to clean up | ||
|
|
||
| _cleanup_in_progress = True | ||
| print("\nCleaning up ZMQ resources...") | ||
| for port_name, port in zmq_ports.items(): | ||
| try: | ||
| port.socket.close() | ||
| port.context.term() | ||
| print(f"Closed ZMQ port: {port_name}") | ||
| except Exception as e: | ||
| logging.error(f"Error while terminating ZMQ port {port.address}: {e}") | ||
| zmq_ports.clear() | ||
| _cleanup_in_progress = False | ||
|
|
||
| def signal_handler(sig, frame): | ||
| """Handle interrupt signals gracefully.""" | ||
| print(f"\nReceived signal {sig}, shutting down gracefully...") | ||
| # Prevent terminate_zmq from being called twice: once here and once via atexit | ||
| try: | ||
| atexit.unregister(terminate_zmq) | ||
| except Exception: | ||
| # If unregister fails for any reason, proceed with explicit cleanup anyway | ||
| pass | ||
| terminate_zmq() | ||
| sys.exit(0) | ||
|
|
||
| # Register cleanup handlers | ||
| atexit.register(terminate_zmq) | ||
| signal.signal(signal.SIGINT, signal_handler) # Handle Ctrl+C | ||
| if not hasattr(sys, 'getwindowsversion'): | ||
| signal.signal(signal.SIGTERM, signal_handler) # Handle termination (Unix only) | ||
|
Comment on lines
+138
to
+142
|
||
|
|
||
| # --- ZeroMQ Integration End --- | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,134 @@ | |||||||||||||||||||||||||||||||||||||||||||||
| from bs4 import BeautifulSoup | ||||||||||||||||||||||||||||||||||||||||||||||
| from rich.panel import Panel | ||||||||||||||||||||||||||||||||||||||||||||||
| from rich.table import Table | ||||||||||||||||||||||||||||||||||||||||||||||
| from rich.tree import Tree | ||||||||||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||||||||||
| from collections import defaultdict, deque | ||||||||||||||||||||||||||||||||||||||||||||||
| from typing import List, Set, Tuple, Dict | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
4
to
+8
|
||||||||||||||||||||||||||||||||||||||||||||||
| from rich.table import Table | |
| from rich.tree import Tree | |
| import re | |
| from collections import defaultdict, deque | |
| from typing import List, Set, Tuple, Dict | |
| import re | |
| from collections import defaultdict, deque | |
| from typing import List, Tuple, Dict |
Copilot
AI
Feb 6, 2026
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.
The docstring says this function "Detect[s] all cycles", but this DFS implementation only records cycles when it encounters a back-edge and does not reliably enumerate all distinct simple cycles (and can emit duplicates in some graphs). Either adjust the wording/return contract or switch to a proper cycle-enumeration approach (e.g., Johnson’s algorithm) / SCC-based reporting.
| Detect all cycles in the workflow graph using DFS. | |
| Args: | |
| nodes_dict: Mapping of node IDs to labels | |
| edges: List of (source_id, target_id) tuples | |
| Returns: | |
| List of cycles, where each cycle is a list of node IDs | |
| Detect cycles in the workflow graph using a DFS-based approach. | |
| Note: | |
| This function records cycles when a back-edge is encountered during DFS. | |
| It is intended for detecting the presence and structure of cycles and may | |
| not enumerate all distinct simple cycles in the graph, and some cycles | |
| may be reported more than once in certain graph topologies. | |
| Args: | |
| nodes_dict: Mapping of node IDs to labels | |
| edges: List of (source_id, target_id) tuples | |
| Returns: | |
| List of detected cycles, where each cycle is a list of node IDs |
Copilot
AI
Feb 6, 2026
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.
Although this is described as a reachability check, the implementation explicitly treats edges as undirected (adds both source->target and target->source). That makes "unreachable"/"reachable" wording misleading for directed workflows; consider renaming/rewording to "disconnected" (weak connectivity) or implement directed reachability from a defined entrypoint.
Copilot
AI
Feb 6, 2026
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.
These warnings say "Unreachable node", but the connectivity check is weak/undirected from an arbitrary node, so nodes in other components are really "disconnected" rather than unreachable in the directed sense. Rewording the warning (or changing the connectivity algorithm) would avoid confusing users.
| warnings.append(f"Unreachable node: {node_label}") | |
| warnings.append(f"Disconnected node (not connected to main component): {node_label}") |
Copilot
AI
Feb 6, 2026
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.
This switches validation output symbols to ASCII (X/OK/!/i), but other CLI commands consistently use Rich/unicode symbols (e.g., run.py/stop.py use ✓/✗/⚠). Unless there’s a specific compatibility requirement, this makes CLI output inconsistent; consider keeping the same symbols across commands or centralizing the style choice.
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.
Registering signal handlers at import time can raise ValueError when concore is imported from a non-main thread, and it also overrides the host application’s signal handling just by importing the module. Consider making this opt-in (e.g., an init/enable_signal_handlers function) or guarding registration so it only happens in the main thread / when running as the top-level process.