Skip to content

Commit 8a8e65c

Browse files
committed
fix(config): address xhigh code-review findings on the concurrency knobs
- resolve_compile_concurrency moves to config.py (matching resolve_timeout's convention) and now rejects bool (bool is an int subclass, so `compile_concurrency: true` previously silently became concurrency=1) and logs a warning on any malformed value, same as the other resolvers. - _build_index_config now warns when pageindex_max_concurrency is configured but the installed PageIndex doesn't support the field yet, instead of silently dropping it with no signal to the user. - Replaced the tautological test_forwards_max_concurrency_when_supported (which branched on the same runtime condition as the code under test, so it never exercised the forwarding assertion under CI's pinned PageIndex) with fake IndexConfig doubles that make both branches deterministic regardless of the installed pageindex version. - Added a cross-check test pinning DEFAULT_CONFIG["compile_concurrency"] against the compiler's own DEFAULT_COMPILE_CONCURRENCY so the two literals can't silently drift apart. - Added an end-to-end test proving index_long_document's own loaded config (not just a hand-built dict) reaches PageIndexClient's IndexConfig. - Hoisted the compile-concurrency resolution out of `recompile --all`'s per-document loop (was recomputed every iteration despite being loop-invariant). - Documented both new config.yaml keys in config.yaml.example and examples/configuration/README.md (kept in sync).
1 parent e12e74e commit 8a8e65c

8 files changed

Lines changed: 190 additions & 39 deletions

File tree

config.yaml.example

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider)
22
language: en # Wiki output language
33
pageindex_threshold: 20 # PDF pages threshold for PageIndex
44

5+
# Optional: cap concurrent LLM calls to avoid provider rate limits or "too many
6+
# open files" on large PDFs. Omit either to use the default.
7+
# pageindex_max_concurrency: 10 # PageIndex indexing concurrency (null = PageIndex's own default)
8+
# compile_concurrency: 5 # concept/entity page generation concurrency
9+
510
# Optional: override the entity-type vocabulary used for entity pages.
611
# Omit this key to use the default 7 types
712
# (person, organization, place, product, work, event, other).

examples/configuration/README.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ model: gpt-5.4 # LLM model (any LiteLLM-supported provider)
7070
language: en # Wiki output language
7171
pageindex_threshold: 20 # PDF pages threshold for PageIndex
7272

73+
# Optional: cap concurrent LLM calls to avoid provider rate limits or "too many
74+
# open files" on large PDFs. Omit either to use the default.
75+
# pageindex_max_concurrency: 10 # PageIndex indexing concurrency (null = PageIndex's own default)
76+
# compile_concurrency: 5 # concept/entity page generation concurrency
77+
7378
# Optional: override the entity-type vocabulary used for entity pages.
7479
# Omit this key to use the default 7 types
7580
# (person, organization, place, product, work, event, other).
@@ -95,6 +100,8 @@ pageindex_threshold: 20 # PDF pages threshold for PageIndex
95100
| `model` | `gpt-5.4` | LLM used for all compile/query/chat work. |
96101
| `language` | `en` | Language the wiki is written in. |
97102
| `pageindex_threshold` | `20` | PDFs with this many pages **or more** take the long-doc (PageIndex) path; shorter ones go through the short-doc path. See [`pageindex-cloud/`](../pageindex-cloud/). |
103+
| `pageindex_max_concurrency` | `null` | Caps concurrent indexing LLM calls PageIndex makes for a single long document. Lower it if you hit provider rate limits or "too many open files" on large PDFs. `null` lets PageIndex apply its own default. |
104+
| `compile_concurrency` | `5` | Caps concurrent LLM calls OpenKB makes while generating concept/entity pages. Lower it if your LLM provider rate-limits. |
98105
| `entity_types` | 7 defaults | Custom vocabulary for entity pages. `other` is always kept. |
99106
| `litellm:` || A pass-through block for LiteLLM. See below. |
100107

openkb/cli.py

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,14 @@ def filter(self, record: logging.LogRecord) -> bool:
4747
litellm.suppress_debug_info = True
4848
from dotenv import load_dotenv
4949

50-
from openkb.agent.compiler import DEFAULT_COMPILE_CONCURRENCY, compile_long_doc
50+
from openkb.agent.compiler import compile_long_doc
5151
from openkb.config import (
5252
DEFAULT_CONFIG,
5353
load_config,
5454
save_config,
5555
load_global_config,
5656
register_kb,
57+
resolve_compile_concurrency,
5758
resolve_extra_headers,
5859
set_extra_headers,
5960
resolve_timeout,
@@ -439,19 +440,6 @@ def _run_compile_with_retry(coro_factory, label: str) -> None:
439440
raise
440441

441442

442-
def _compile_concurrency(config: dict) -> int:
443-
"""Concurrency cap for the compile step (concept/entity generation).
444-
445-
Configurable per KB via ``compile_concurrency`` in config.yaml; a missing,
446-
null, or non-positive value falls back to the compiler's built-in default.
447-
Lower it when the LLM provider rate-limits.
448-
"""
449-
value = config.get("compile_concurrency")
450-
if isinstance(value, int) and value > 0:
451-
return value
452-
return DEFAULT_COMPILE_CONCURRENCY
453-
454-
455443
def add_single_file(
456444
file_path: Path, kb_dir: Path, *, stage: bool = True
457445
) -> Literal["added", "skipped", "failed"]:
@@ -577,7 +565,7 @@ def _add_single_file_locked(
577565
kb_dir,
578566
model,
579567
doc_description=index_result.description,
580-
max_concurrency=_compile_concurrency(config),
568+
max_concurrency=resolve_compile_concurrency(config),
581569
),
582570
label=f"Compiling long doc (doc_id={index_result.doc_id})",
583571
)
@@ -591,7 +579,7 @@ def _add_single_file_locked(
591579
source_path,
592580
kb_dir,
593581
model,
594-
max_concurrency=_compile_concurrency(config),
582+
max_concurrency=resolve_compile_concurrency(config),
595583
),
596584
label="Compiling short doc",
597585
)
@@ -719,7 +707,7 @@ def import_from_pageindex_cloud(doc_id: str, kb_dir: Path) -> Literal["added", "
719707
kb_dir,
720708
model,
721709
doc_description=cloud.description,
722-
max_concurrency=_compile_concurrency(config),
710+
max_concurrency=resolve_compile_concurrency(config),
723711
),
724712
label=f"Compiling imported doc (doc_id={doc_id})",
725713
)
@@ -1665,6 +1653,7 @@ def _classify(meta: dict) -> str:
16651653
_setup_llm_key(kb_dir)
16661654
config = load_config(openkb_dir / "config.yaml")
16671655
model: str = config.get("model", DEFAULT_CONFIG["model"])
1656+
max_concurrency = resolve_compile_concurrency(config)
16681657

16691658
# Import lazily and reference via the module so tests can patch
16701659
# ``openkb.agent.compiler.compile_*`` and see the call.
@@ -1707,7 +1696,7 @@ def _classify(meta: dict) -> str:
17071696
doc_id,
17081697
kb_dir,
17091698
model,
1710-
max_concurrency=_compile_concurrency(config),
1699+
max_concurrency=max_concurrency,
17111700
)
17121701
)
17131702
except Exception as exc:
@@ -1735,7 +1724,7 @@ def _classify(meta: dict) -> str:
17351724
source_path,
17361725
kb_dir,
17371726
model,
1738-
max_concurrency=_compile_concurrency(config),
1727+
max_concurrency=max_concurrency,
17391728
)
17401729
)
17411730
except Exception as exc:

openkb/config.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,27 @@ def resolve_timeout(config: dict) -> float | None:
183183
return value
184184

185185

186+
def resolve_compile_concurrency(config: dict) -> int:
187+
"""Resolve the optional ``compile_concurrency:`` key for the compile step
188+
(concept/entity generation).
189+
190+
Returns ``DEFAULT_CONFIG["compile_concurrency"]`` when absent, ``None``, or
191+
invalid; rejects bools and non-positive values, warning when present but
192+
unusable (an explicit ``null`` is the normal "use the default" case and
193+
warns silently, matching ``resolve_timeout``).
194+
"""
195+
value = config.get("compile_concurrency")
196+
if value is None:
197+
return DEFAULT_CONFIG["compile_concurrency"]
198+
if isinstance(value, bool) or not isinstance(value, int) or value <= 0:
199+
logger.warning(
200+
"config: 'compile_concurrency' must be a positive integer, got %r — using default.",
201+
value,
202+
)
203+
return DEFAULT_CONFIG["compile_concurrency"]
204+
return value
205+
206+
186207
def resolve_litellm_settings(config: dict) -> dict[str, Any]:
187208
"""Resolve the optional ``litellm:`` mapping of LiteLLM module settings.
188209

openkb/indexer.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,14 @@ def _build_index_config(config: dict[str, Any]) -> IndexConfig:
169169
"if_add_doc_description": True,
170170
}
171171
max_concurrency = config.get("pageindex_max_concurrency")
172-
if max_concurrency is not None and "max_concurrency" in IndexConfig.model_fields:
173-
kwargs["max_concurrency"] = max_concurrency
172+
if max_concurrency is not None:
173+
if "max_concurrency" in IndexConfig.model_fields:
174+
kwargs["max_concurrency"] = max_concurrency
175+
else:
176+
logger.warning(
177+
"config: 'pageindex_max_concurrency' is set but the installed "
178+
"PageIndex version does not support it yet — ignoring it."
179+
)
174180
return IndexConfig(**kwargs)
175181

176182

tests/test_add_command.py

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -97,16 +97,6 @@ def test_add_single_file_compile_failure_rolls_back_converted_artifacts(self, tm
9797
assert not (kb_dir / "wiki" / "sources" / "notes.md").exists()
9898
assert HashRegistry(kb_dir / ".openkb" / "hashes.json").all_entries() == {}
9999

100-
def test_compile_concurrency_resolution(self):
101-
from openkb.agent.compiler import DEFAULT_COMPILE_CONCURRENCY
102-
from openkb.cli import _compile_concurrency
103-
104-
assert _compile_concurrency({}) == DEFAULT_COMPILE_CONCURRENCY
105-
assert _compile_concurrency({"compile_concurrency": 3}) == 3
106-
# None / non-positive / non-int fall back to the compiler default.
107-
assert _compile_concurrency({"compile_concurrency": None}) == DEFAULT_COMPILE_CONCURRENCY
108-
assert _compile_concurrency({"compile_concurrency": 0}) == DEFAULT_COMPILE_CONCURRENCY
109-
110100
def test_add_forwards_compile_concurrency_from_config(self, tmp_path):
111101
from unittest.mock import AsyncMock
112102

tests/test_config.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
get_extra_headers,
66
get_timeout,
77
load_config,
8+
resolve_compile_concurrency,
89
resolve_extra_headers,
910
resolve_litellm_settings,
1011
resolve_timeout,
@@ -46,6 +47,59 @@ def test_load_compile_concurrency_override(tmp_path):
4647
assert load_config(config_path)["compile_concurrency"] == 3
4748

4849

50+
def test_resolve_compile_concurrency_absent_uses_default():
51+
assert resolve_compile_concurrency({}) == DEFAULT_CONFIG["compile_concurrency"]
52+
53+
54+
def test_resolve_compile_concurrency_valid_value():
55+
assert resolve_compile_concurrency({"compile_concurrency": 3}) == 3
56+
57+
58+
def test_resolve_compile_concurrency_rejects_bool(caplog):
59+
with caplog.at_level(logging.WARNING, logger="openkb.config"):
60+
result = resolve_compile_concurrency({"compile_concurrency": True})
61+
assert result == DEFAULT_CONFIG["compile_concurrency"]
62+
assert "compile_concurrency" in caplog.text
63+
64+
65+
def test_resolve_compile_concurrency_rejects_non_positive(caplog):
66+
with caplog.at_level(logging.WARNING, logger="openkb.config"):
67+
assert (
68+
resolve_compile_concurrency({"compile_concurrency": 0})
69+
== DEFAULT_CONFIG["compile_concurrency"]
70+
)
71+
assert "compile_concurrency" in caplog.text
72+
caplog.clear()
73+
with caplog.at_level(logging.WARNING, logger="openkb.config"):
74+
assert (
75+
resolve_compile_concurrency({"compile_concurrency": -1})
76+
== DEFAULT_CONFIG["compile_concurrency"]
77+
)
78+
assert "compile_concurrency" in caplog.text
79+
80+
81+
def test_resolve_compile_concurrency_rejects_non_int():
82+
assert (
83+
resolve_compile_concurrency({"compile_concurrency": "3"})
84+
== DEFAULT_CONFIG["compile_concurrency"]
85+
)
86+
87+
88+
def test_resolve_compile_concurrency_none_is_silent(caplog):
89+
# Explicit null / absent is the normal "use the default" case — no warning.
90+
with caplog.at_level(logging.WARNING, logger="openkb.config"):
91+
resolve_compile_concurrency({"compile_concurrency": None})
92+
assert caplog.text == ""
93+
94+
95+
def test_compile_concurrency_defaults_match_compiler_default():
96+
"""DEFAULT_CONFIG and the compiler's own DEFAULT_COMPILE_CONCURRENCY are two
97+
independent literals; catch drift immediately if only one is ever updated."""
98+
from openkb.agent.compiler import DEFAULT_COMPILE_CONCURRENCY
99+
100+
assert DEFAULT_CONFIG["compile_concurrency"] == DEFAULT_COMPILE_CONCURRENCY
101+
102+
49103
def test_load_missing_file_returns_defaults(tmp_path):
50104
missing = tmp_path / "nonexistent" / "config.yaml"
51105
config = load_config(missing)

tests/test_indexer.py

Lines changed: 87 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,10 @@
22

33
from __future__ import annotations
44

5+
import logging
56
from unittest.mock import MagicMock, patch
67

78
import pytest
8-
from pageindex import IndexConfig
99

1010
from openkb.indexer import (
1111
IndexResult,
@@ -15,25 +15,78 @@
1515
)
1616

1717

18+
class _FakeIndexConfigWithConcurrency:
19+
"""Stand-in for a PageIndex ``IndexConfig`` that declares ``max_concurrency``.
20+
21+
Used instead of relying on whatever ``pageindex`` happens to be installed in
22+
this environment, so the forwarding tests are deterministic regardless of
23+
the currently-pinned PageIndex version (see ``test_forwards_...`` below).
24+
"""
25+
26+
model_fields = {
27+
"if_add_node_text": None,
28+
"if_add_node_summary": None,
29+
"if_add_doc_description": None,
30+
"max_concurrency": None,
31+
}
32+
33+
def __init__(self, **kwargs):
34+
self.__dict__.update(kwargs)
35+
36+
37+
class _FakeIndexConfigWithoutConcurrency:
38+
"""Stand-in for a PageIndex ``IndexConfig`` predating ``max_concurrency``."""
39+
40+
model_fields = {
41+
"if_add_node_text": None,
42+
"if_add_node_summary": None,
43+
"if_add_doc_description": None,
44+
}
45+
46+
def __init__(self, **kwargs):
47+
self.__dict__.update(kwargs)
48+
49+
1850
class TestBuildIndexConfig:
1951
def test_sets_base_flags(self):
2052
cfg = _build_index_config({})
2153
assert cfg.if_add_node_text is True
2254
assert cfg.if_add_node_summary is True
2355
assert cfg.if_add_doc_description is True
2456

25-
def test_forwards_max_concurrency_when_supported(self):
57+
def test_forwards_max_concurrency_when_supported(self, monkeypatch):
58+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithConcurrency)
2659
cfg = _build_index_config({"pageindex_max_concurrency": 8})
27-
if "max_concurrency" in IndexConfig.model_fields:
28-
assert cfg.max_concurrency == 8
29-
else:
30-
# A PageIndex predating the field: forwarded as a no-op, never raised.
31-
assert not hasattr(cfg, "max_concurrency")
60+
assert cfg.max_concurrency == 8
3261

33-
def test_none_value_is_left_to_pageindex_default(self):
62+
def test_does_not_forward_when_unsupported(self, monkeypatch):
63+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithoutConcurrency)
64+
cfg = _build_index_config({"pageindex_max_concurrency": 8})
65+
assert not hasattr(cfg, "max_concurrency")
66+
67+
def test_none_value_is_left_to_pageindex_default(self, monkeypatch):
68+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithConcurrency)
3469
cfg = _build_index_config({"pageindex_max_concurrency": None})
3570
assert getattr(cfg, "max_concurrency", None) is None
3671

72+
def test_warns_when_configured_but_unsupported(self, monkeypatch, caplog):
73+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithoutConcurrency)
74+
with caplog.at_level(logging.WARNING, logger="openkb.indexer"):
75+
_build_index_config({"pageindex_max_concurrency": 8})
76+
assert "pageindex_max_concurrency" in caplog.text
77+
78+
def test_no_warning_when_unset(self, monkeypatch, caplog):
79+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithoutConcurrency)
80+
with caplog.at_level(logging.WARNING, logger="openkb.indexer"):
81+
_build_index_config({})
82+
assert caplog.text == ""
83+
84+
def test_no_warning_when_supported(self, monkeypatch, caplog):
85+
monkeypatch.setattr("openkb.indexer.IndexConfig", _FakeIndexConfigWithConcurrency)
86+
with caplog.at_level(logging.WARNING, logger="openkb.indexer"):
87+
_build_index_config({"pageindex_max_concurrency": 8})
88+
assert caplog.text == ""
89+
3790

3891
class TestNormalizePageContent:
3992
def test_normalizes_pageindex_dicts(self):
@@ -204,6 +257,32 @@ def test_localclient_called_with_index_config(self, kb_dir, sample_tree, tmp_pat
204257
assert ic.if_add_node_summary is True
205258
assert ic.if_add_doc_description is True
206259

260+
def test_pageindex_max_concurrency_flows_from_kb_config(self, kb_dir, sample_tree, tmp_path):
261+
"""The KB's real config.yaml, loaded by index_long_document itself, must
262+
reach the IndexConfig passed to PageIndexClient — not just the isolated
263+
_build_index_config unit tested directly with a hand-built dict."""
264+
(kb_dir / ".openkb" / "config.yaml").write_text(
265+
"model: gpt-4o-mini\npageindex_max_concurrency: 7\n", encoding="utf-8"
266+
)
267+
268+
doc_id = "conc-789"
269+
fake_col = self._make_fake_collection(doc_id, sample_tree)
270+
fake_client = MagicMock()
271+
fake_client.collection.return_value = fake_col
272+
273+
pdf_path = tmp_path / "report.pdf"
274+
pdf_path.write_bytes(b"%PDF-1.4 fake")
275+
276+
with (
277+
patch("openkb.indexer.IndexConfig", _FakeIndexConfigWithConcurrency),
278+
patch("openkb.indexer.PageIndexClient", return_value=fake_client) as mock_cls,
279+
patch("openkb.images.convert_pdf_to_pages", return_value=self._fake_pages()),
280+
):
281+
index_long_document(pdf_path, kb_dir)
282+
283+
_, kwargs = mock_cls.call_args
284+
assert kwargs["index_config"].max_concurrency == 7
285+
207286
def test_cloud_page_content_is_normalized(self, kb_dir, sample_tree, tmp_path, monkeypatch):
208287
doc_id = "cloud-123"
209288
fake_col = self._make_fake_collection(doc_id, sample_tree)

0 commit comments

Comments
 (0)