Refactoring inline imports and resolve circular dependencies #113

bmerkle · 2025-12-03T21:32:43Z

Refactoring for issue #112

all inline imports are removed

formatted imports and from imports
(sorted, arranged etc, via ruff tool) ruff check --select I --fix .

circular dependecies are removed, a new types.py file was introduced

searchlib.py improved handling of pydantic classes

note that the patch set is quite large (99 files) but i looked through to assert that nothing broke.

unit tests ran fine. (except mcp server)

…ecks

- Rearranged import statements for better readability and consistency. - Consolidated imports from the same module into single lines where applicable. - Removed unused imports and organized them to follow a logical order. - Introduced a new `types.py` file to hold shared type definitions, reducing circular dependencies. - Updated version number to 0.3.3 in `uv.lock`. - searchlib.py improved handling of pydantic classes

bmerkle · 2025-12-03T21:36:51Z

(typeagent) PS C:\work\microsoft\typeagent-py> .\make.bat test
Running unit tests...
================================================================= test session starts =================================================================
platform win32 -- Python 3.13.5, pytest-8.4.2, pluggy-1.6.0
rootdir: C:\work\microsoft\typeagent-py
configfile: pyproject.toml
testpaths: test
plugins: anyio-4.11.0, logfire-4.14.2, asyncio-1.2.0, mock-3.15.1
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=function, asyncio_default_test_loop_scope=function
collected 358 items

test\test_add_messages_with_indexing.py ... [ 0%]
test\test_auth.py ....... [ 2%]
test\test_collections.py ........................... [ 10%]
test\test_conversation_metadata.py .................... [ 15%]
test\test_demo.py . [ 16%]
test\test_embedding_consistency.py ... [ 17%]
test\test_embeddings.py ........... [ 20%]
test\test_factory.py ... [ 20%]
test\test_incremental_index.py .. [ 21%]
test\test_interfaces.py ...................... [ 27%]
test\test_knowledge.py .... [ 28%]
test\test_kplib.py ...... [ 30%]
test\test_mcp_server.py ss [ 31%]
test\test_message_text_index_population.py . [ 31%]
test\test_message_text_index_serialization.py .. [ 31%]
test\test_messageindex.py ....s... [ 34%]
test\test_online.py . [ 34%]
test\test_podcast_incremental.py .. [ 34%]
test\test_podcasts.py . [ 35%]
test\test_property_index_population.py . [ 35%]
test\test_propindex.py ............ [ 38%]
test\test_query.py .................................. [ 48%]
test\test_query_method.py .. [ 48%]
test\test_related_terms_fast.py . [ 49%]
test\test_related_terms_index_population.py . [ 49%]
test\test_reltermsindex.py ............. [ 53%]
test\test_searchlib.py ....................................................... [ 68%]
test\test_secindex.py ... [ 69%]
test\test_secindex_storage_integration.py . [ 69%]
test\test_semrefindex.py ...................... [ 75%]
test\test_serialization.py ........ [ 77%]
test\test_sqlite_indexes.py ..................... [ 83%]
test\test_sqlitestore.py ..... [ 85%]
test\test_storage_providers_unified.py .............................. [ 93%]
test\test_timestampindex.py . [ 93%]
test\test_transcripts.py ....... [ 95%]
test\test_utils.py .... [ 96%]
test\test_vectorbase.py ........... [100%]

====================================================== 355 passed, 3 skipped in 64.52s (0:01:04) ======================================================

gvanrossum

Maybe we should install and run isort as part of 'make format'...

gvanrossum · 2025-12-04T00:26:58Z

test/test_message_text_index_serialization.py

-import pytest
 import sqlite3

 import numpy as np
+import pytest


The 3rd party modules should be in one block. But fixtures is local and needs to be separated by a blank line.

gvanrossum · 2025-12-04T00:28:02Z

test/test_online.py

 # Licensed under the MIT License.

 import pytest
-


Put that blank line back please.

gvanrossum · 2025-12-04T00:46:36Z

test/test_searchlib.py

-        assert group.terms[0] == term1
-        assert group.terms[1] == term2
+        assert pydantic_dataclass_to_dict(group.terms[0]) == pydantic_dataclass_to_dict(term1)
+        assert pydantic_dataclass_to_dict(group.terms[1]) == pydantic_dataclass_to_dict(term2)


Curious why this was needed. Doesn't pydantic's dataclass create an __eq__ method etc.?

If you use Pydantic dataclasses, you get the normal Python dataclass dunder methods, including eq.
If you use Pydantic models, equality is handled differently, there is no generation of eq etc.

you are right that we are currently using dataclasses and not models, but if we would switch to models, the new code above would also work (so in this case it is more generic and prepared for changes in the future) .

gvanrossum

Flight's leaving.

gvanrossum · 2025-12-04T00:54:50Z

tmp_debug_tag.py

@@ -0,0 +1,14 @@
+"""Legacy debug script placeholder.


This file probably shouldn't be imported. If it should, it needs a copyright header.

gvanrossum · 2025-12-04T00:55:19Z

tools/add_copyright.py

 from pathlib import Path
 from typing import List, Tuple

-


Please don't mess with this spacing.

sorry, I missed make format

gvanrossum · 2025-12-04T00:56:48Z

tools/get_keys.py


+import colorama
+
 # Azure SDK imports


I'd rather change this comment than move the imports.

gvanrossum · 2025-12-04T00:57:55Z

tools/vizcmp.py

-from colorama import init as colorama_init, Back, Fore, Style
+from colorama import Back, Fore, Style
+from colorama import init as colorama_init


Why this change?

gvanrossum · 2025-12-04T00:59:02Z

typeagent/aitools/auth.py

-from dataclasses import dataclass
 import time
+from dataclasses import dataclass


Original was better...

gvanrossum · 2025-12-04T01:01:22Z

typeagent/knowpro/convsettings.py

        if self._storage_provider is None:
+            # Import here to avoid circular import
            from ..storage.memory import MemoryStorageProvider
-


Doesn't 'make format' put this blank line back?

gvanrossum · 2025-12-04T01:04:26Z

typeagent/knowpro/searchlib.py

+from typing import Any, cast
+
+
+def pydantic_dataclass_to_dict(obj: Any) -> Any:


Why do we need this? It seems significant but I fail to see why.

gvanrossum · 2025-12-04T01:07:33Z

typeagent/storage/__init__.py

-    MemoryStorageProvider,
-    MemoryMessageCollection,
-    MemorySemanticRefCollection,
+                     MemoryMessageCollection,
+                     MemorySemanticRefCollection,
+                     MemoryStorageProvider,


Bad formatting. Use 'make format'

gvanrossum · 2025-12-04T01:08:33Z

typeagent/storage/memory/provider.py

        exc_tb: object,
    ) -> None:
        """Exit transaction context. No-op for in-memory storage."""
-        pass


I like to preserve such 'pass' lines.

gvanrossum · 2025-12-04T01:10:18Z

typeagent/storage/sqlite/collections.py

 import typing

+from ...knowpro import interfaces, serialization
 from .schema import ShreddedMessage, ShreddedSemanticRef


Correct! My guiding principle here is that the "closest" import comes last.

gvanrossum · 2025-12-04T01:12:18Z

typeagent/storage/sqlite/provider.py

            ValueError: If embeddings in the database don't match the expected size.
        """
-        from .schema import deserialize_embedding



If you delete the last online import, also delete the blank line (which was imposed by the formatter).

bmerkle · 2025-12-04T05:42:38Z

@gvanrossum question: can we have a simple, low-threshold communcation channel (like chat, discord, ms-teams, or linked-in chat) to brainstrom or discuss in advance things ? Then i might explain beforehand and we can easily chat if it make sense. :-)

in the PR here I used ruff instead of isort and format. It might sort thing differently than isort so this change caused some noise or disagreement about sorting and formatting i think. Appended is a short comparison of ruff vs. isort and format.

bmerkle · 2025-12-04T05:44:52Z

✅ What Ruff brings — and where it may “beat” isort

Ruff is a fast, all-in-one linter + formatter + import-sorter — it tries to replace not only isort, but also tools like linters, formatters, and more in one go.

If you enable the import-sorting rules (rule group “I”), Ruff can sort imports — so you don’t need a separate tool just for imports.
Medium

For large code bases or in continuous-integration / pre-commit settings, Ruff tends to be much faster than combining multiple tools (formatter + linter + isort) because it’s implemented in Rust and optimized for performance.

For convenience: with Ruff you can have linting + formatting + import handling in a single command or setup, reducing the number of dependencies or tools you need to maintain.

So if you want a unified toolchain, care about speed/performance, and are okay with potentially slightly different import-sorting behavior — Ruff is a compelling modern choice.

⚠️ Where isort still shines (or where Ruff may fall short)

Import sorting in Ruff is “intended to be near-equivalent” to isort (especially if you configure isort with certain profiles), but there are known differences — e.g. in how aliased imports are grouped, or how inline comments are handled.

Ruff’s import-sorting capabilities are “good enough” for many, but if you rely on very precise import ordering/grouping rules, or need maximum control over formatting decisions, isort might be more predictable and configurable.

Because Ruff is broader in scope (linter + formatter + sorter), if something goes wrong — configuration issues, conflicts with formatting — it may be harder to isolate and debug than a dedicated import sorter.

Some users report edge cases or inconsistencies when mixing Ruff’s sorting + formatting + other tools (especially if also using other formatters or import tools).

bmerkle · 2025-12-04T07:42:05Z

i did not run make format check , and the refactoring broke some typechecking tests.
sorry i will rework the PR.

bmerkle · 2025-12-04T11:25:47Z

@gvanrossum okay, much of the noise was caused by the reformatting of imports at the top of the file.
I did not only fix inline imports, but at the same time rearranged the imports because i had to touch them anyway.
Via ruff i did a automatic reformatting, but it should conform to PEP 8 and be consistent because it is automatic.
So hopefully this explains the modifications regarding imports. I looked through all files and they LGTM.

ruff formats imports according to the PEP 8 import ordering rules, which generally means:

first import ...
then from ... import ...

Ruff’s import sorter (ruff --fix, rule I001, internally inspired by isort) enforces:

Standard library imports
Third-party imports
Local application imports

…cies Added dataclasses.py to wrap pydantic’s decorator with dataclass_transform + overloads so pyright understands generated initializers and fields; updated all knowpro modules, tests, and related utilities to import the wrapper instead of pydantic.dataclasses.dataclass. Adjusted dependent modules (interfaces.py, kplib.py, search.py, search_query_schema.py, date_time_schema.py, answer_response_schema.py, universal_message.py, email_message.py, and several tests) to use the new wrapper while preserving import ordering and existing behavior. all other changes a import and formatting related modifications (which should be ok) make format, check, test is running clean

…agent-py into refactoring_imports

bmerkle · 2025-12-04T13:18:23Z

relevant changes:

Added dataclasses.py to wrap pydantic’s decorator with dataclass_transform + overloads so pyright understands generated initializers and fields;
updated all knowpro modules, tests, and related utilities to import the wrapper instead of pydantic.dataclasses.dataclass.

Adjusted dependent modules (interfaces.py, kplib.py, search.py, search_query_schema.py, date_time_schema.py, answer_response_schema.py, universal_message.py, email_message.py, and several tests) to use the new wrapper while preserving import ordering and existing behavior.

note:

all other changes a import and formatting related modifications (which should be ok)
make format, check, test is running clean

gvanrossum · 2025-12-04T18:41:09Z

chat

I'm on Discord but I don't want to reveal my handle here and I have heavy privacy barriers around it. If you email me privately at [email protected] we can arrange something.

KRRT7 · 2025-12-07T11:57:02Z

Generally in vibe-coded projects there tend to be a lot of inline imports because LLMs are bad at writing code properly and tend to hack around things by adding inline imports vs structuring and importing the right way the first time around.

My suggestion would be to reset against main, implement the necessary changes, and then run make format only, strictly resolving the inline imports / implementing the refactor. This way you'll have an easy-to-review PR for this specific concern.

Later on you could make a separate PR switching over to ruff and avoiding the 101 files diff in this PR and have it happen in that subsequent PR

Another thing: instead of asking LLMs about the differences between isort and ruff import sorting, I'd highly suggest reading over https://docs.astral.sh/ruff/faq/#how-does-ruffs-import-sorting-compare-to-isort as it's up to date.

bmerkle · 2025-12-07T16:55:31Z

My suggestion would be to reset against main, implement the necessary changes, and then run make format only, strictly resolving the inline imports / implementing the refactor. This way you'll have an easy-to-review PR for this specific concern.

Later on you could make a separate PR switching over to ruff and avoiding the 101 files diff in this PR and have it happen in that subsequent PR

Hi @KRRT7 thanks for your comments.
regarding the PR i am open to reset against main, and do it again, but i think we will end up with the same number of modification.

Refactoring the inline import is a kind of mass operations when it affects several files.
I also automatically rearranged the import and from statements of the beginning of the files.
While they follow the accepted PEP style and have been done consistent and automatically with ruff, they have raised some discussion (alphabetical sort in the list, etc).

The interesting part of this PR is the removal of circular dependencies. I can outline and explain this to better understand the PR.

The rest of the PR is pretty trivial as it concerns only the above mentioned import and inline rearrangement and it by nature a mass change. I reviewed this in VSCode and you have only to really check the beginning sections of the files. And the inline imports which have been pulled up to the top of the file.

You could argue that i have done all 3 things (remove circular dependencies, remove inline import, reformat imports) in one step, but i think it made sense.

Please lets me know what you think. I can resubmit 3 PR for each step, but if we reformat the imports, number of modifications we be the same.

gvanrossum · 2025-12-07T18:53:22Z

I strongly prefer to undo all the import reformats for this PR, if possible. The other two are intertwined and should probably be kept together.

bmerkle · 2025-12-07T21:20:16Z

OK i will create a new PR.

bmerkle · 2025-12-09T22:50:55Z

please cancel/close this PR as it is too large and covers too much different aspects.
I will create 3 separate, smaller PR for each aspect:

reduce curicular dependencies Refactor imports to reduce circular dependencies and enhance code organization #118
fix inline imports (still TODO)
fix import layout (still TODO)

bmerkle added 7 commits November 12, 2025 22:24

Use sys.executable for python interpreter instead of fixed path

62e6aeb

Refactor search_index to use more Pythonic conditions for argument ch…

1b85245

…ecks

Merge branch 'microsoft:main' into main

63f4ea9

Merge branch 'microsoft:main' into main

815aeed

Merge branch 'main' of https://github.com/bmerkle/typeagent-py

d3aef1a

Merge branch 'main' of https://github.com/bmerkle/typeagent-py

c36657a

bmerkle had a problem deploying to build-pipeline December 3, 2025 21:32 — with GitHub Actions Failure

bmerkle changed the title ~~Refactoring for issue https://github.com/microsoft/typeagent-py/issues/112~~ Refactoring for issue 112 Dec 3, 2025

Merge branch 'main' into refactoring_imports

1c81c1d

gvanrossum temporarily deployed to build-pipeline December 3, 2025 22:19 — with GitHub Actions Inactive

gvanrossum reviewed Dec 4, 2025

View reviewed changes

bmerkle added 2 commits December 4, 2025 14:17

Merge branch 'refactoring_imports' of https://github.com/bmerkle/type…

d2864eb

…agent-py into refactoring_imports

bmerkle had a problem deploying to build-pipeline December 4, 2025 13:18 — with GitHub Actions Failure

bmerkle changed the title ~~Refactoring for issue 112~~ Refactoring inline imports and resolve circular dependencies Dec 4, 2025

bmerkle closed this Dec 9, 2025

		from typing import Any, cast


		def pydantic_dataclass_to_dict(obj: Any) -> Any:

Refactoring inline imports and resolve circular dependencies #113

Refactoring inline imports and resolve circular dependencies #113

Uh oh!

Conversation

bmerkle commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmerkle commented Dec 3, 2025

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmerkle commented Dec 4, 2025

Uh oh!

bmerkle commented Dec 4, 2025

Uh oh!

bmerkle commented Dec 4, 2025

Uh oh!

bmerkle commented Dec 4, 2025

Uh oh!

bmerkle commented Dec 4, 2025

relevant changes:

note:

Uh oh!

gvanrossum commented Dec 4, 2025

Uh oh!

KRRT7 commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmerkle commented Dec 7, 2025

Uh oh!

gvanrossum commented Dec 7, 2025

Uh oh!

bmerkle commented Dec 7, 2025

Uh oh!

bmerkle commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmerkle commented Dec 3, 2025 •

edited

Loading

KRRT7 commented Dec 7, 2025 •

edited

Loading