Skip to content

fix(hooks): register dynamically loaded hook modules - #606

Open
林SO (Linxiushen) wants to merge 1 commit into
microsoft:mainfrom
Linxiushen:fix/hooks-module-registration
Open

林SO (Linxiushen) wants to merge 1 commit into
microsoft:mainfrom
Linxiushen:fix/hooks-module-registration

Conversation

@Linxiushen

Copy link
Copy Markdown

Fixes #605

Custom rollout hooks containing a dataclass and from __future__ import annotations fail during loading because load_hooks does not register the executing module in sys.modules. Register the module before execution, use a name derived from its resolved path so different hook files do not collide, and restore the previous registration if execution, validation, or construction fails.

The regression tests load real Python files and execute on_enqueue with RolloutCreate. They cover postponed annotations, independent module identities through pickle round trips, and rollback after module execution, class validation, or constructor errors. On the original source these tests produce 6 failures and 2 passes; all 8 pass with the fix.

Validation:

  • uv sync --frozen --no-default-groups --extra dev --group dev --group verl-cpu followed by uv run --locked --no-sync pytest -v --durations=20 tests: 128 passed, on Linux/Python 3.12.14 with CPU PyTorch.
  • uv run --locked --no-sync pyright: 0 errors, 0 warnings.
  • Full repository Ruff lint/format, copyright-header check, and pre-commit hooks pass.
  • uv build: source distribution and wheel built successfully.

Developed with Codex assistance and independent agent review. No GPU training was run; this change is confined to the Python hook loader.

Copilot AI balanced review requested due to automatic review settings September 27, 2026 21:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused implementation correctly addresses the loader defect and includes comprehensive regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes dynamic hook loading by registering path-unique modules, enabling postponed annotations and pickle support.

Changes:

  • Registers hook modules in sys.modules with deterministic path-based names.
  • Restores prior module state when loading fails.
  • Adds regression coverage for dataclasses, pickling, module isolation, and failures.
File Description
agentlightning/​hooks.py Adds collision-resistant module registration and rollback.
tests/​test_hooks.py Tests successful loading, isolation, pickling, and cleanup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

This branch has not been deployed

No deployments
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.

Custom hook dataclasses with postponed annotations fail during module loading

2 participants