Skip to content

fix: declare log file fields in RunState dataclass - #15557

Open
vominh1919 wants to merge 2 commits into
NousResearch:mainfrom
vominh1919:fix/rl-training-dataclass-fields
Open

vominh1919 wants to merge 2 commits into
NousResearch:mainfrom
vominh1919:fix/rl-training-dataclass-fields

Conversation

@vominh1919

Copy link
Copy Markdown
Contributor

Problem

api_log_file, trainer_log_file, and env_log_file are set dynamically on RunState instances (lines 337, 360, 401) but are never declared as dataclass fields. They work via __dict__ but:

  1. Are invisible to type checkers and IDE autocomplete
  2. Are not listed by dataclasses.fields()
  3. Would break if slots=True is added to the dataclass (a common optimization)

Fix

Declare the three fields explicitly in the RunState dataclass:

api_log_file: Optional[Any] = None
trainer_log_file: Optional[Any] = None
env_log_file: Optional[Any] = None

…m providers

Fixes:
- NousResearch#13766: CLI agents no longer emit MEDIA:/path tags (CLI has no attachment channel)
- NousResearch#13765: Add 'from __future__ import annotations' to 64 files for Python 3.9 PEP-604 compatibility
- NousResearch#13764: Model switch now searches custom_providers catalog before API probe

Changes:
- agent/prompt_builder.py: Extend CLI platform hint to prevent MEDIA: tag emission
- cli.py: Load custom_providers unconditionally (not just for picker)
- hermes_cli/model_switch.py: Add _find_model_in_custom_providers() helper, insert step c2 in PATH B
- hermes_cli/models.py: normalize_provider() handles custom:* slugs
- 64 files: Add 'from __future__ import annotations' for Python 3.9 compatibility
api_log_file, trainer_log_file, and env_log_file are set dynamically
on RunState instances but were never declared as dataclass fields.
They work via __dict__ but would break if slots=True is added (a
common optimization).  Declare them explicitly so type checkers,
IDE autocomplete, and dataclasses.fields() see them.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Apr 25, 2026
@alt-glitch alt-glitch added comp/tools Tool registry, model_tools, toolsets and removed comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jun 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the cleanup. The three declarations are internally consistent for the historical module, but they no longer have a target on current main.

Problems

  • tools/rl_training_tool.py:140 is in a module explicitly removed by current-main commit 5af672c75 (tools/rl_training_tool.py, all RL tools/tests, and the Atropos integration). A tracked-HEAD search finds no remaining RunState or log-file assignment to amend.
  • This PR also includes unrelated parent commit 7174fbf725e0, which changes 69 files including cli.py and agent/prompt_builder.py; the stated fix itself is only the three-line commit c71d84dbc764.

Suggested changes

  • Any future RL restoration should be proposed against the current architecture with its required integration and tests, rather than salvaging this field-only change.
  • The 7174fbf725e0 changes should be evaluated independently of this dataclass cleanup.

Automated hermes-sweeper review.

Comment thread tools/rl_training_tool.py
api_process: Optional[subprocess.Popen] = None
trainer_process: Optional[subprocess.Popen] = None
env_process: Optional[subprocess.Popen] = None
api_log_file: Optional[Any] = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Current main deliberately deleted this entire Atropos/RL module in 5af672c75; there is no remaining RunState to amend. A field-only salvage cannot apply without a separate decision to restore the RL integration.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants