Skip to content

Stop bypassing the knowpro.interfaces aggregator - #306

Open
Bernhard Merkle (bmerkle) wants to merge 1 commit into
microsoft:mainfrom
bmerkle:fix-import-style-aggregator-bypass
Open

Stop bypassing the knowpro.interfaces aggregator#306
Bernhard Merkle (bmerkle) wants to merge 1 commit into
microsoft:mainfrom
bmerkle:fix-import-style-aggregator-bypass

Conversation

@bmerkle

Copy link
Copy Markdown
Collaborator

Summary

interfaces.py explicitly re-exports interfaces_core/interfaces_indexes/interfaces_search/interfaces_serialization/interfaces_storage via __all__ (exempted from AGENTS.md's "no re-export" rule by its own exception clause), but 4 files partially imported some symbols through it and others directly from the defining submodule:

  • conversation_base.py: TextLocation (from interfaces_core)
  • add_messages.py: IKnowledgeExtractor, IMessage, MessageOrdinal, TextLocation (from interfaces_core)
  • storage/sqlite/provider.py: ChunkFailure (from interfaces_storage)
  • podcast_ingest.py: AddMessagesResult (from interfaces_core)

Investigated and ruled out both plausible justifications (details in #298):

  • Not a circular-import workaround — none of the interfaces_*.py submodules import anything from these 4 files.
  • Not a missing-export issue — every bypassed symbol is confirmed present in the relevant submodule's __all__.

Git history shows the bypass lines were added months after the aggregator existed (2025-12-31), across unrelated feature commits (streaming API, batching/pipeline overlap) with no import-style rationale — incidental, not deliberate.

Merges each bypassed import into the existing (or a new) from .interfaces import (...) block.

Found via a whole-project import-style scan, tracked in #298.

Test plan

  • make (format, check on 3.12/3.14, test, build) — 0 pyright errors, 737 passed / 12 skipped, wheel builds

interfaces.py explicitly re-exports interfaces_core/interfaces_indexes/
interfaces_search/interfaces_serialization/interfaces_storage via
__all__ (exempted from AGENTS.md's "no re-export" rule by its own
exception clause), but 4 files partially imported some symbols
through it and others directly from the defining submodule:

- conversation_base.py: TextLocation (from interfaces_core)
- add_messages.py: IKnowledgeExtractor, IMessage, MessageOrdinal,
  TextLocation (from interfaces_core)
- storage/sqlite/provider.py: ChunkFailure (from interfaces_storage)
- podcast_ingest.py: AddMessagesResult (from interfaces_core)

Investigated and ruled out both plausible justifications: none of the
interfaces_*.py submodules import anything from these 4 files (no
circular-import risk), and every bypassed symbol is confirmed present
in the relevant submodule's __all__ (nothing missing from the
aggregator). Git history shows the bypass lines were added months
after the aggregator existed, across unrelated feature commits with
no import-style rationale -- incidental, not deliberate.

Merges each bypassed import into the existing (or a new)
`from .interfaces import (...)` block.

Found via a whole-project import-style scan, tracked in microsoft#298.

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.

Pull request overview

This PR standardizes import style by routing several knowpro interface symbol imports through the knowpro.interfaces aggregator module, eliminating “bypass” imports that reached directly into interfaces_core / interfaces_storage.

Changes:

  • Consolidate ChunkFailure, ConversationMetadata, and STATUS_INGESTED imports in the SQLite provider to come from knowpro.interfaces.
  • Consolidate AddMessagesResult and Datetime imports in podcast ingest to come from knowpro.interfaces.
  • Consolidate TextLocation and other interface symbols in knowpro core modules (conversation_base.py, add_messages.py) to come from knowpro.interfaces.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/typeagent/storage/sqlite/provider.py Merges ChunkFailure into the existing knowpro.interfaces import.
src/typeagent/podcasts/podcast_ingest.py Imports AddMessagesResult via the knowpro.interfaces aggregator instead of interfaces_core.
src/typeagent/knowpro/conversation_base.py Removes direct interfaces_core import of TextLocation, using the aggregator import block instead.
src/typeagent/knowpro/add_messages.py Consolidates several interface-symbol imports into a single from .interfaces import (...) block.

@bmerkle

Copy link
Copy Markdown
Collaborator Author

robgruen would you mind taking a look at this one when you get a chance? Thanks!

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