Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions milestones/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ repositories that install `docgen` and maintain their own demo bundle. The
library no longer ships an in-repo dogfood; consumers are the integration test
of record.

**Active:** **[wizard-narration-mode.md](wizard-narration-mode.md)** —
unknown wizard / LLM narration `mode` must not silently generate.
**Active:** **[merge-settings-numerics.md](merge-settings-numerics.md)** —
narration / scene-generation merge must not coerce bool temperature
or model values.

**Shipped:**
- **[wizard-narration-mode.md](wizard-narration-mode.md)** —
unknown wizard / LLM narration `mode` must not silently generate
(#141).
- **[merge-settings-str-lists.md](merge-settings-str-lists.md)** —
narration / scene-generation merge must not `str()` hint and path lists
(#140).
Expand Down
47 changes: 47 additions & 0 deletions milestones/merge-settings-numerics.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Milestone: merge settings must not coerce bool tunables

**Status:** Active
**PR:** [#142](https://github.com/jmjava/documentation-generator/pull/142)
**Depends on:** `milestones/wizard-narration-mode.md` (PR #141),
`milestones/merge-settings-str-lists.md` (PR #140),
`milestones/generation-numeric-tunables.md` (PR #117)

## Problem

`Config.from_yaml` already requires YAML numbers for
`narration_from_source.temperature` / `max_context_bytes` (#117) and
string `model` (#107). The merge helpers still did:

```python
temperature = float(root.get("temperature", DEFAULT_TEMPERATURE))
max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES))
model = str(root.get("model") or DEFAULT_MODEL)
```

`bool` is a subclass of `int`: `temperature: true` became **1.0**,
`max_context_bytes: true` became a **1-byte** window, `model: true`
became `"True"`. `system_prompt: true` / `class_name: true` became
`"True"` in the LLM prompt.

Wizard generate-narration and `scene-spec-generate` both go through
these merges.

## Goal

Fail closed on present non-number tunables and non-string model /
system_prompt / class_name. Missing/null still uses defaults.
`temperature: 0` still stays 0.

## Done when

- [x] Bool `temperature` / `max_context_bytes` raise
- [x] Bool `model` / `class_name` raise
- [x] Zero temperature still not replaced by the default
- [x] `ruff check src/ tests/`
- [x] `pytest tests/` (794 passed, 1 skipped)
- [x] `docgen benchmark` (no clock change; meets baseline)

## Out of scope

- `Config.from_yaml` already gates these types for CLI load
- Whisper prompt-cap `int(... or 0)` in timing enrichment (separate)
2 changes: 1 addition & 1 deletion milestones/wizard-narration-mode.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Milestone: wizard / LLM narration mode must not silently generate

**Status:** Active
**Status:** Shipped
**PR:** [#141](https://github.com/jmjava/documentation-generator/pull/141)
**Depends on:** `milestones/merge-settings-str-lists.md` (PR #140),
`milestones/wizard-json-object.md` (PR #129)
Expand Down
18 changes: 18 additions & 0 deletions src/docgen/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,24 @@ def require_yaml_number(value: Any, *, label: str, source: str) -> float:
return float(value)


def optional_yaml_number(
block: dict[str, Any],
key: str,
*,
default: float,
label: str,
source: str = "docgen.yaml",
) -> float:
"""Return *default* when *key* is missing/null; otherwise require a YAML number.

Do not ``float(true)`` → ``1.0`` or ``int(true)`` → ``1``.
"""
val = block.get(key)
if val is None:
return float(default)
return require_yaml_number(val, label=label, source=source)


def require_yaml_number_list(value: Any, *, label: str, source: str) -> list[float]:
"""Require a YAML list of numbers so bools/strings do not reach ``int()``."""
if not isinstance(value, list):
Expand Down
55 changes: 48 additions & 7 deletions src/docgen/manim_scene_support.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,13 @@ def merged_scene_generation_settings(cfg: "Config", seg_id: str) -> SceneGenerat
`visual_beats`) are not merged from hint files; set them in committed bundle
``docgen.yaml`` when needed, or rely on TIMING JSON / auto tables in the spec prompt.
"""
from docgen.config import context_path_globs, string_list_block
from docgen.config import (
context_path_globs,
optional_yaml_number,
require_optional_yaml_string,
require_yaml_string,
string_list_block,
)

root = cfg.raw.get("manim_scene_generation")
if not isinstance(root, dict):
Expand All @@ -539,20 +545,55 @@ def merged_scene_generation_settings(cfg: "Config", seg_id: str) -> SceneGenerat
seg, "hints", label=f"manim_scene_generation.segments.{seg_id}.hints"
)

model = str(root.get("model") or DEFAULT_MODEL).strip() or DEFAULT_MODEL
temperature = float(root.get("temperature", DEFAULT_TEMPERATURE))
max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES))
raw_model = root.get("model")
model = (
DEFAULT_MODEL
if raw_model is None
else require_yaml_string(
raw_model, label="manim_scene_generation.model", source="docgen.yaml"
)
)
temperature = optional_yaml_number(
root,
"temperature",
default=DEFAULT_TEMPERATURE,
label="manim_scene_generation.temperature",
)
max_bytes = int(
optional_yaml_number(
root,
"max_context_bytes",
default=DEFAULT_MAX_CONTEXT_BYTES,
label="manim_scene_generation.max_context_bytes",
)
)

sys_override = str(root.get("system_prompt", "")).strip()
seg_sys = str(seg.get("system_prompt", "")).strip()
raw_sys = root.get("system_prompt")
require_optional_yaml_string(
raw_sys, label="manim_scene_generation.system_prompt", source="docgen.yaml"
)
seg_sys_raw = seg.get("system_prompt")
require_optional_yaml_string(
seg_sys_raw,
label=f"manim_scene_generation.segments.{seg_id}.system_prompt",
source="docgen.yaml",
)
sys_override = raw_sys.strip() if isinstance(raw_sys, str) else ""
seg_sys = seg_sys_raw.strip() if isinstance(seg_sys_raw, str) else ""
if seg_sys:
system_prompt = seg_sys
elif sys_override:
system_prompt = sys_override
else:
system_prompt = ""

cls_name = str(seg.get("class_name", "")).strip() or None
raw_cls = seg.get("class_name")
require_optional_yaml_string(
raw_cls,
label=f"manim_scene_generation.segments.{seg_id}.class_name",
source="docgen.yaml",
)
cls_name = raw_cls.strip() or None if isinstance(raw_cls, str) else None

return SceneGenerationSettings(
model=model,
Expand Down
47 changes: 41 additions & 6 deletions src/docgen/narrate_from_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,13 @@ def merged_narration_from_source_settings(cfg: "Config", seg_id: str) -> Narrati

``hints`` are always **authored in YAML by the project owner** (never returned from OpenAI).
"""
from docgen.config import context_path_globs, string_list_block
from docgen.config import (
context_path_globs,
optional_yaml_number,
require_optional_yaml_string,
require_yaml_string,
string_list_block,
)

root = cfg.raw.get("narration_from_source")
if not isinstance(root, dict):
Expand All @@ -79,12 +85,41 @@ def merged_narration_from_source_settings(cfg: "Config", seg_id: str) -> Narrati
seg, "hints", label=f"narration_from_source.segments.{seg_id}.hints"
)

model = str(root.get("model") or DEFAULT_MODEL).strip() or DEFAULT_MODEL
temperature = float(root.get("temperature", DEFAULT_TEMPERATURE))
max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES))
raw_model = root.get("model")
model = (
DEFAULT_MODEL
if raw_model is None
else require_yaml_string(
raw_model, label="narration_from_source.model", source="docgen.yaml"
)
)
temperature = optional_yaml_number(
root,
"temperature",
default=DEFAULT_TEMPERATURE,
label="narration_from_source.temperature",
)
max_bytes = int(
optional_yaml_number(
root,
"max_context_bytes",
default=DEFAULT_MAX_CONTEXT_BYTES,
label="narration_from_source.max_context_bytes",
)
)

sys_override = str(root.get("system_prompt", "")).strip()
seg_sys = str(seg.get("system_prompt", "")).strip()
raw_sys = root.get("system_prompt")
require_optional_yaml_string(
raw_sys, label="narration_from_source.system_prompt", source="docgen.yaml"
)
seg_sys_raw = seg.get("system_prompt")
require_optional_yaml_string(
seg_sys_raw,
label=f"narration_from_source.segments.{seg_id}.system_prompt",
source="docgen.yaml",
)
sys_override = raw_sys.strip() if isinstance(raw_sys, str) else ""
seg_sys = seg_sys_raw.strip() if isinstance(seg_sys_raw, str) else ""
if seg_sys:
system_prompt = seg_sys
elif sys_override:
Expand Down
12 changes: 12 additions & 0 deletions tests/test_manim_scene_support.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,18 @@ def test_settings_rejects_int_hint_and_string_paths(tmp_path: Path) -> None:
merged_scene_generation_settings(cfg, "08")


def test_settings_rejects_bool_temperature(tmp_path: Path) -> None:
cfg = _write_cfg(tmp_path, {"manim_scene_generation": {"temperature": 0.4}})
cfg.raw["manim_scene_generation"]["temperature"] = True
with pytest.raises(ConfigError, match="temperature must be a YAML number"):
merged_scene_generation_settings(cfg, "08")
cfg.raw["manim_scene_generation"]["temperature"] = 0.4
cfg.raw["manim_scene_generation"]["class_name"] = 1
cfg.raw["manim_scene_generation"]["segments"] = {"08": {"class_name": True}}
with pytest.raises(ConfigError, match="class_name must be a YAML string"):
merged_scene_generation_settings(cfg, "08")


def test_settings_zero_temperature_is_not_replaced_by_default(tmp_path: Path) -> None:
cfg = _write_cfg(
tmp_path,
Expand Down
21 changes: 21 additions & 0 deletions tests/test_narrate_from_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,27 @@ def test_merged_settings_rejects_string_hints(tmp_path: Path) -> None:
merged_narration_from_source_settings(cfg, "01")


def test_merged_settings_rejects_bool_temperature(tmp_path: Path) -> None:
"""``float(True)`` used to become temperature 1.0."""
(tmp_path / ".git").mkdir()
(tmp_path / "docgen.yaml").write_text(
yaml.dump({"narration_from_source": {"temperature": 0.65}}),
encoding="utf-8",
)
cfg = Config.from_yaml(tmp_path / "docgen.yaml")
cfg.raw["narration_from_source"]["temperature"] = True
with pytest.raises(ConfigError, match="temperature must be a YAML number"):
merged_narration_from_source_settings(cfg, "01")
cfg.raw["narration_from_source"]["temperature"] = 0.65
cfg.raw["narration_from_source"]["max_context_bytes"] = False
with pytest.raises(ConfigError, match="max_context_bytes must be a YAML number"):
merged_narration_from_source_settings(cfg, "01")
cfg.raw["narration_from_source"]["max_context_bytes"] = 120_000
cfg.raw["narration_from_source"]["model"] = True
with pytest.raises(ConfigError, match="model must be a YAML string"):
merged_narration_from_source_settings(cfg, "01")


def test_collect_source_snippets_respects_extra_paths(tmp_path: Path) -> None:
(tmp_path / ".git").mkdir()
(tmp_path / "docgen.yaml").write_text(
Expand Down