Skip to content

Turn async accessor methods to sync properties for provider attributes#256

Open
gvanrossum-ms wants to merge 3 commits intomicrosoft:mainfrom
gvanrossum:sync-storage-props
Open

Turn async accessor methods to sync properties for provider attributes#256
gvanrossum-ms wants to merge 3 commits intomicrosoft:mainfrom
gvanrossum:sync-storage-props

Conversation

@gvanrossum-ms
Copy link
Copy Markdown
Contributor

Plus some changes to AGENTS.md -- I want to start using worktrees (so we can do multiple projects in parallel), but the agent got confused and was looking in all the wrong places (so we added more hints to AGENTS.md).

@gvanrossum-ms
Copy link
Copy Markdown
Contributor Author

@KRRT7 @bmerkle

@KRRT7
Copy link
Copy Markdown
Contributor

KRRT7 commented Apr 29, 2026

Hey Guido, would you mind splitting the AGENTS.md changes into a separate PR? Keeps the sync properties refactor easy to review on its own.

Copy link
Copy Markdown
Contributor

@KRRT7 KRRT7 left a comment

Choose a reason for hiding this comment

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

Sync properties refactor looks good — none of the old async getters were doing actual async work, so @property is more honest and Pythonic.

Two things worth noting in the PR description:

  1. The sqlite provider's old get_conversation_threads() was creating a new ConversationThreads instance on every call (with a lazy import). Now it creates one in __init__ and returns the same instance. That's a behavioral change / bug fix — you'd lose thread state between calls before.

  2. term_to_semantic_ref_index was renamed to semantic_ref_index on the sqlite provider to match the interface. Minor but worth calling out.

@gvanrossum
Copy link
Copy Markdown
Collaborator

Hey Guido, would you mind splitting the AGENTS.md changes into a separate PR? Keeps the sync properties refactor easy to review on its own.

Just ignore AGENTS.md.

  1. The sqlite provider's old get_conversation_threads() was creating a new ConversationThreads instance on every call (with a lazy import). Now it creates one in __init__ and returns the same instance. That's a behavioral change / bug fix — you'd lose thread state between calls before.

It's a bugfix, but it doesn't actually change behavior -- there's only one non-test call site, in ConversationSecondaryIndexes.create(), which sets self.threads and then that attribute is used elsewhere. (It's a bug, because the memory provider doesn't create a new one -- and that provider is the original one.)

On top of that, I don't think the actual ingest and query algorithms even uses the threads attribute. At least the new version is consistent. :-)

2. `term_to_semantic_ref_index` was renamed to `semantic_ref_index` on the sqlite provider to match the interface. Minor but worth calling out.

It's a bit awkward: the class is TermToSemanticRefIndex and it is stored on the class as private _term_to_semantic_ref_index, but the callers (almost?) all store it into a variable named semantic_ref_index (or index, in at least one case). The class name covers what it does well (it maps terms to semantic ref IDs) but it's too awkward, so everyone shortens it -- even the sqlite schema uses SemanticRefIndex for the table name.

I don't believe any action is necessary, but I'll wait for approval from @bmerkle so I don't have to cheat to merge it myself.

@bmerkle
Copy link
Copy Markdown
Collaborator

bmerkle commented Apr 29, 2026

hey, great work. LGTM. no additional feedback from my side.
had to merge in main, so @gvanrossum has to merge it

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.

4 participants