feat: add configurable metrics toggle and guard node hooks - #15
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _default_metrics_enabled() -> bool: | ||
| try: | ||
| from .config import get_config | ||
|
|
||
| return bool(get_config().metrics_enabled) | ||
| except Exception: | ||
| return False | ||
|
|
||
|
|
||
| metrics_collector = GameMetrics(enabled=_default_metrics_enabled()) |
There was a problem hiding this comment.
The new _default_metrics_enabled() call at module import time eagerly invokes get_config() and caches the default configuration before application code has a chance to provide a custom path. Because get_config memoizes the first loaded instance, any subsequent call like get_config("/custom/path.yaml") will now silently return the already loaded default config, so runtime configuration overrides no longer work unless callers explicitly call reload_config first. This regression is triggered whenever src.game.metrics is imported (which happens from the nodes modules) and prevents consumers from loading non-default configs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def host_setup(state: GameState) -> Dict[str, Any]: | ||
| """Initializes the game, assigning roles and words.""" | ||
| config = get_config() | ||
| metrics_collector.set_enabled(config.metrics_enabled) |
There was a problem hiding this comment.
The new metrics toggle is forcibly reset at the start of every game. host_setup immediately calls metrics_collector.set_enabled(config.metrics_enabled), so any code that turns metrics on programmatically (e.g. run_multilingual_metrics_batch now calls metrics_collector.set_enabled(True) before executing games) is undone as soon as the host node runs when the config file has metrics.enabled: false (the default). As a result the batch command never records metrics and prints empty aggregates unless the user edits the config file first. Consider respecting the current collector state or only setting it once at process start so temporary toggles can work.
Useful? React with 👍 / 👎.
|
@codex reivew |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def host_setup(state: GameState) -> Dict[str, Any]: | ||
| """Initializes the game, assigning roles and words.""" | ||
| config = get_config() | ||
| metrics_collector.set_enabled(config.metrics_enabled) | ||
|
|
There was a problem hiding this comment.
The new metrics toggle forces every game start to call metrics_collector.set_enabled(config.metrics_enabled). During run_multilingual_metrics_batch the collector is explicitly enabled (and reset) to gather benchmark data, but config.metrics_enabled defaults to False, so the first host node immediately switches the collector off and clears its state. As a result the multilingual metrics CLI now yields empty metrics/quality scores unless the user edits config.yaml to enable metrics beforehand, which is a regression from the previous always-on behaviour and defeats the explicit enabling in the batch runner. Consider skipping this override when a batch run has already enabled metrics or when config.metrics_enabled is false by default.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
No description provided.