Conversation
tulayha
left a comment
tulayha
left a comment
There was a problem hiding this comment.
Keeping this focused on planning is fine, but accepted plans must stay within supported behavior. Please address the two inline findings and keep temporal/evidence steps disabled until execution supports them.
Please also remove the unrelated pytest dev-group and lockfile changes; the repository already provides a test extra.
| kind: Literal["search_moments"] = "search_moments" | ||
| modality: Identifier | ||
| query: SearchQuery | ||
| occurrence_mode: OccurrenceMode = OccurrenceMode.best |
There was a problem hiding this comment.
first and all pass plan validation, but execution ignores this field and performs the usual ranked search. Please implement the selected mode or reject unsupported modes so the existing fallback is used. Add coverage for this behavior.
There was a problem hiding this comment.
fixed first/all now reject at plan validation and fall back to the existing ranked search, best stays supported. added test coverage for both the rejection and fallback paths..........
| evidence_steps = sum( | ||
| isinstance(step, EvidenceRequestPlanStep) for step in plan.steps | ||
| ) | ||
| if not temporal_relations and temporal_steps > 0: |
There was a problem hiding this comment.
This checks whether temporal steps are enabled, but does not validate their target modality. A plan with target_modality="nonexistent" passes when temporal planning is enabled. Please check it against search_modalities and cover rejection and fallback.
There was a problem hiding this comment.
fixed......... target_modality on TemporalRelationPlanStep now gets checked against search_modalities, unknown modality gets rejected using the same fallback pattern as the rest of the file. added tests for rejection + fallback here too
… target_modality - Reject first/all occurrence modes, fall back to ranked search since execution doesn't implement them yet - Validate TemporalRelationPlanStep.target_modality against search_modalities, reject unknown modalities with existing fallback - Add test coverage for both rejection paths - Remove unrelated pytest dev-group from pyproject.toml/uv.lock Addresses review comments from @tulayha on grayhatdevelopers#147
|
@tulayha also removed the unrelated pytest dev-group + lockfile changes from pyproject.toml/uv.lock, using the existing test extra now like u mentioned ruff clean, 71 passed............ lmk if anything else needs work |
2 participants
Related issue
Related to #84
Summary
Extends the local-model query-planning path with support for temporal
relations, occurrence modes, and evidence-delivery requests, per the
examples listed in #84.
OccurrenceMode(first/best/all) toSearchMomentsPlanStep.TemporalRelationPlanStepfor before/after/during queries betweentwo searches.
EvidenceRequestPlanStepfor requesting a board or exactclips/frames as evidence.
temporal_relations_allowed,evidence_requests_allowed) toQueryPlanningRequest, defaulting toFalse._default_plan,_valid_plan, andGroundedQueryService.planin
query_service.pyto accept and validate the new step types, withthe same default-
Falseopt-in behavior.This is an internal, application-layer change: no CLI, HTTP, MCP, or
frontend surface calls the new parameters yet, and existing callers are
unaffected since all new parameters default to
False.Out of scope for this PR (left as follow-up per #84, to keep this PR
focused on one outcome):
plan, for reproducibility.
descriptions.
Validation
uv run --no-sync ruff check .All checks passed!
65 passed in 3.43s
The full
pytest -qsuite could not be run end-to-end locally: severalunrelated test modules fail to collect because of pre-existing missing
optional dependencies (
torch,streamlit,fastapi,httpx,cryptography,alembic,numpy, etc.) unrelated to this change, anduv sync --extra local-workertimed out downloadingtorchdue to alocal network issue. These gaps were present before this change as well.