feat(semantic-memory): make ingestion poll interval configurable by yiweizh-memverge · Pull Request #1708 · MemMachine/MemMachine

yiweizh-memverge · 2026-09-22T20:10:38Z

Purpose of the change

Fixes #1699 — the background ingestion loop polled for dirty sets on a hardcoded
2-second interval, with no way to tune it per deployment.

Description

Add ingestion_poll_interval_seconds to SemanticMemoryConf (default 2.0,
matching the previous hardcoded value), wire it through SemanticResourceManager
into SemanticService, and expose it on the v2 config update API and the Python
client wrapper. Also wires consolidation_threshold through the manager, though
it is not yet API-exposed.

Floors the error backoff ceiling at the configured interval. The old fixed 60s cap
was harmless at 2s, but with an interval above 60s it made retries during an outage
poll faster than configured.

Behavioral note for reviewers: like the existing ingestion triggers, API updates
take effect on the next server restart — the running SemanticService is cached and
has no reload path.

No dependency changes.

Fixes/Closes

Fixes #1699

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit Test
ruff check and ruff format --check on the changed file: clean.

New coverage:

test_semantic_conf.py — the new field parses and defaults to 2.0
test_semantic_manager_service_params.py — interval and consolidation_threshold
reach SemanticService
test_config_service.py — the v2 config update API accepts the field
test_config.py (client) — the client wrapper passes it through
test_semantic_memory_background.py — asserts the exact backoff sequence, proving
both that doubling happens and that the ceiling never drops below the configured
interval

Checklist

I have signed the commit(s) within this pull request
My code follows the style guidelines of this project (See STYLE_GUIDE.md)
I have performed a self-review of my own code
I have commented my code
My changes generate no new warnings
I have added unit tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have checked my code and corrected any misspellings

Screenshots/Gifs

N/A

Further comments

The config field is documented in api/doc.py.

malatewang

I think we may want to put those parameters into the server configuration.

malatewang · 2026-09-24T21:15:42Z

        default=timedelta(minutes=5),
        description="The amount of time a message is uningested before triggering an ingestion.",
    )
+    ingestion_poll_interval_seconds: float = Field(


Int should be a better type

Thanks Shu! This has been changed.

The background ingestion loop polled for dirty sets on a hardcoded 2-second interval. Add ingestion_poll_interval_seconds to SemanticMemoryConf (default 2.0), wire it through SemanticResourceManager into SemanticService, expose it on the v2 config update API and the Python client wrapper. Also wire consolidation_threshold through the manager (not yet API-exposed). Floor the error backoff ceiling at the configured interval. The old fixed 60s cap was harmless at 2s, but with an interval above 60s it made retries during an outage poll faster than configured. Like the existing ingestion triggers, API updates take effect on the next server restart: the running SemanticService is cached and has no reload path. In the backoff test, bind the ingestion task to a local and assert it is not None before awaiting it: the attribute is typed `Task | None`, which ty rejects as not awaitable. Fixes MemMachine#1699 Co-Authored-By: Claude Opus 5 <[email protected]> Signed-off-by: Yiwei Zhang <[email protected]>

Addresses review feedback on PR MemMachine#1708: an integer number of seconds is a better fit than float for this config field. Co-Authored-By: Claude Sonnet 5 <[email protected]>

edwinyyyu

Reviewed at 22fcc22. Six inline comments below; the one on the backoff ceiling is a question on intent rather than a defect.

edwinyyyu · 2026-09-29T16:59:34Z

        embedding_model: str | None = None,
        ingestion_trigger_messages: int | None = None,
        ingestion_trigger_age_seconds: int | None = None,
+        ingestion_poll_interval_seconds: int | None = None,


Inserting this before timeout shifts timeout for anyone calling positionally: a pre-existing call with timeout=30 as the 11th positional argument now sends ingestion_poll_interval_seconds=30 in the request body and runs with timeout=None. No in-repo caller passes positionally, so this only affects external users, but the method has no keyword-only marker at all.

Suggest a bare * after self so these options are keyword-only. That also makes future additions order-independent.

Good catch. I moved ingestion_poll_interval_seconds after timeout, so nothing shifts for existing positional callers.
I held off on the bare *. As @marvinyu-memverge pointed out, it would also break any caller passing enabled or the other options positionally, which is a wider break than this PR needs. Making the options keyword-only seems worth doing as its own change, happy to open an issue for it.

edwinyyyu · 2026-09-29T16:59:34Z

+            "Minimum number of features sharing a tag before they are "
+            "consolidated during ingestion."
+        ),
+        gt=0,


gt=0 rules out consolidation_threshold: 0, but the ingestion service treats 0 as "consolidate every tag group regardless of size" (semantic_ingestion.py:360, if self._consolidation_threshold > 0:), and get_feature_set(tag_threshold=...) accepts it. Since this PR is the first config path to that value, either ge=0 to keep the mode reachable, or drop the now-dead branch if 0 was never meant to be supported.

I went with ge=0 so the mode stays reachable. Since 0 means "consolidate every tag group regardless of size" rather than "disabled", which is easy to misread, I documented it in the field description and in the YAML table in configuration.mdx, noting that it costs one LLM call per tag group per ingestion. Added tests that 0 is accepted and -1 is rejected.

Fixed in 05512f1.

edwinyyyu · 2026-09-29T16:59:34Z

-                backoff_sec = min(backoff_sec * 2, 60.0)
+                backoff_sec = min(
+                    backoff_sec * 2,
+                    max(60.0, self._background_ingestion_interval_sec),


Question on intent. With the ceiling at max(60, interval), for any interval >= 60 the backoff starts at interval and is capped at interval, so the doubling never applies and an outage is retried at exactly the healthy cadence (the above-ceiling-floored case pins 120, 120, 120). That satisfies "never faster than configured", which is what the PR body describes.

Is backoff still meant to slow retries at large intervals? If so the ceiling would need to scale, e.g. max(60, 4 * interval). If not, this is fine as is.

Yes, backoff should still slow retries at large intervals. I changed the ceiling to max(60, 4 * interval). The default 2s interval is unchanged (the cap stays at 60s), and a 120s interval now backs off 120 -> 240 -> 480 and holds there. I replaced the above-ceiling-floored case with long-interval-scaled-ceiling pinning that sequence.

Fixed in 05512f1.

edwinyyyu · 2026-09-29T16:59:34Z

+    message = service.update_memory_config(None, spec)
+
+    sm = memory_resource_manager.config.semantic_memory
+    assert sm.ingestion_poll_interval_seconds == 15


This assertion runs against semantic_memory = MagicMock() from the fixture (line 216), so setting then reading any attribute round-trips: the test stays green if the conf field were renamed or the assignment in _apply_semantic_memory_updates misspelled. The same fixture uses a real EpisodicMemoryConfPartial for the episodic side; a real SemanticMemoryConf() here (it constructs with defaults) would make the assertion prove the field exists.

Fixed in 05512f1.

edwinyyyu · 2026-09-29T16:59:34Z

    assert conf.ingestion_trigger_age == timedelta(minutes=2, milliseconds=500)


+def test_semantic_config_ingestion_poll_interval_defaults_and_overrides():


The gt=0 constraints are the only new validation in the PR and nothing exercises them: no test feeds 0 or a negative value to ingestion_poll_interval_seconds or consolidation_threshold at the conf layer, nor to UpdateSemanticMemorySpec.ingestion_poll_interval_seconds at the API layer. Dropping gt=0 leaves the suite green, and 0 would make _interruptible_sleep(0) spin the dirty-set query in a tight loop. Worth a rejection case for 0 and -1 at both layers.

Minor: this test's name says poll interval but it covers consolidation_threshold too.

Added rejection cases for 0 and -1 on ingestion_poll_interval_seconds at both the conf layer (SemanticMemoryConf) and the API layer (UpdateSemanticMemorySpec). For consolidation_threshold, which is now ge=0, there is a test that 0 is accepted and -1 is rejected. I also renamed the test to test_semantic_config_ingestion_settings_defaults_and_overrides since it covers both fields.

Fixed in 05512f1.

edwinyyyu · 2026-09-29T16:59:34Z

    The maximum age (in seconds) of uningested messages before
    triggering an ingestion cycle."""

+    SEMANTIC_INGESTION_POLL_INTERVAL = """


Two hand-maintained field lists still omit the new keys:

RouterDoc.UPDATE_SEMANTIC_CONFIG further down in this file (around lines 1136-1150) enumerates ingestion_trigger_messages and ingestion_trigger_age_seconds but not ingestion_poll_interval_seconds, so the generated endpoint description in docs/openapi.json will not mention it.

The YAML reference table in docs/open_source/configuration.mdx (lines 215-216) lists the sibling trigger keys but neither ingestion_poll_interval_seconds nor consolidation_threshold.

Since #1699 came out of the value not being discoverable (#753), these are the places people will look.

Fixed in 05512f1.

marvinyu-memverge

Reviewed at 22fcc22. The change does what #1699 asks; nothing blocking from me beyond Edwin's open comments, plus one fact for his keyword-only thread.

On the keyword-only suggestion for the client's update_semantic_memory_config: the positional shift has already shipped once - #1406 inserted storage_backend, feature_store, vector_collection and vector_dimensions after database, and that went out in v0.3.9-post1. A bare * after self would also break a caller that passes even enabled positionally, so it's a wider break than the insertion. Appending the new arg after timeout is the zero-break option; * is the cleaner one going forward. Either is defensible, just worth picking knowingly.

Verified:

Defaults are unchanged: 2s poll, consolidation threshold 20. On main, SemanticService.Params.consolidation_threshold was stored but never passed to IngestionService, so ingestion always ran on IngestionService's own default of 20. This PR makes the field live without moving the value.
SemanticResourceManager.get_semantic_service is the only production construction site of SemanticService, so the manager wiring covers every path.
An API update goes through _persist_config -> save_config, and to_yaml_dict serializes the new keys, so "takes effect on the next restart" holds for a server loaded from a config file.

CI hasn't run on this PR yet - it's from a fork, so the workflows from 09-28 are waiting on approval.

…tion-poll-interval-1699

- Move ingestion_poll_interval_seconds after timeout in the client's update_semantic_memory_config so positional callers are unaffected. - Allow consolidation_threshold: 0 (consolidate every tag group) and document what it means. - Scale the ingestion backoff ceiling to max(60, 4 * interval) so long poll intervals still back off on errors. - Use a real SemanticMemoryConf in the config service test fixture and add validation tests for the new fields. - Document the new keys in RouterDoc and configuration.mdx. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Yiwei Zhang <[email protected]>

leomem requested review from edwinyyyu and malatewang September 23, 2026 21:49

malatewang reviewed Sep 24, 2026

View reviewed changes

leomem requested a review from marvinyu-memverge September 25, 2026 23:31

yiweizh-memverge and others added 2 commits September 28, 2026 18:01

fix(semantic-memory): use int for ingestion_poll_interval_seconds

22fcc22

Addresses review feedback on PR MemMachine#1708: an integer number of seconds is a better fit than float for this config field. Co-Authored-By: Claude Sonnet 5 <[email protected]>

yiweizh-memverge force-pushed the feat/semantic-ingestion-poll-interval-1699 branch from 378abdc to 22fcc22 Compare September 28, 2026 22:02

edwinyyyu reviewed Sep 29, 2026

View reviewed changes

marvinyu-memverge reviewed Sep 29, 2026

View reviewed changes

yiweizh-memverge and others added 2 commits October 1, 2026 20:52

Merge remote-tracking branch 'upstream/main' into feat/semantic-inges…

4abdf47

…tion-poll-interval-1699

		assert conf.ingestion_trigger_age == timedelta(minutes=2, milliseconds=500)


		def test_semantic_config_ingestion_poll_interval_defaults_and_overrides():

Conversation

yiweizh-memverge commented Sep 22, 2026

Purpose of the change

Description

Fixes/Closes

Type of change

How Has This Been Tested?

Checklist

Screenshots/Gifs

Further comments

Uh oh!

malatewang 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!

edwinyyyu 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!

marvinyu-memverge left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants