-
Notifications
You must be signed in to change notification settings - Fork 662
[FIX] Seed challenge_llm from the exported tool's resolved value so challenge-enabled deploys pass #2209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[FIX] Seed challenge_llm from the exported tool's resolved value so challenge-enabled deploys pass #2209
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,9 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from adapter_processor_v2.adapter_processor import AdapterProcessor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from adapter_processor_v2.models import AdapterInstance | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from prompt_studio.prompt_studio_registry_v2.constants import PromptStudioRegistryKeys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from prompt_studio.prompt_studio_registry_v2.prompt_studio_registry_helper import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PromptStudioRegistryHelper, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from rest_framework.serializers import ListField, Serializer, UUIDField, ValidationError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from workflow_manager.workflow_v2.constants import WorkflowKey | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from workflow_manager.workflow_v2.models.workflow import Workflow | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -170,11 +173,26 @@ def create(self, validated_data: dict[str, Any]) -> Any: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validated_data[TIKey.PK] = uuid.uuid4() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: Use version from tool props | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validated_data[TIKey.VERSION] = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tool_settings = ToolProcessor.get_default_settings(tool) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # `get_default_settings` seeds an adapter-valued property with "" when | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the spec carries no default (e.g. challenge_llm). The exported tool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # already resolved a real value for it, so prefer that - otherwise the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # instance stores "", which fails deployment validation against the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # adapter enum. Only spec-declared keys are overlaid; the export's | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # settings are a superset (llm, vector-db, ...) and the rest are not | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # part of the instance schema. Non-Prompt-Studio tools resolve to {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # here, making this a no-op for them. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolved_settings = PromptStudioRegistryHelper.get_resolved_settings( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prompt_registry_id=str(tool_uid) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for key in tool_settings: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if key in resolved_settings: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tool_settings[key] = resolved_settings[key] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validated_data[TIKey.METADATA] = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: Review and remove tool instance ID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkflowKey.WF_TOOL_INSTANCE_ID: str(validated_data[TIKey.PK]), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PromptStudioRegistryKeys.PROMPT_REGISTRY_ID: str(tool_uid), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| **ToolProcessor.get_default_settings(tool), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| **tool_settings, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+176
to
+195
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n 'single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION' backend/prompt_studio/prompt_studio_registry_v2/Repository: Zipstack/unstract Length of output: 702 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== ToolInstanceV2 serializer relevant section =="
rg -n "get_resolved_settings|get_default_settings|tool_settings|single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION|METADATA|TIKey" backend/tool_instance_v2/serializers.py
echo
echo "== prompt_studio_registry_helper relevant exports =="
sed -n '280,325p' backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py
sed -n '50,90p' backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py
echo
echo "== constants =="
rg -n "class JsonSchemaKey|ENABLE_SINGLE_PASS_EXTRACTION|SINGLE_PASS|single_pass_extraction_mode|ENABLE_HIGHLIGHT|ENABLE_WORD_CONFIDENCE|CHALLENGE_LLM|ENABLE_CHALLENGE|SUMMARIZE_AS_SOURCE" backend/prompt_studio/prompt_studio_registry_v2/constants.pyRepository: Zipstack/unstract Length of output: 5802 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== tool_instance serializer diff/stat =="
git diff -- backend/tool_instance_v2/serializers.py | sed -n '1,120p' || true
git diff -- backend/tool_instance_v2/ --stat || true
echo
echo "== relevant tests mentioning single_pass_extraction_mode / ENABLE_SINGLE_PASS_EXTRACTION =="
rg -n "single_pass_extraction_mode|ENABLE_SINGLE_PASS_EXTRACTION|enable_single_pass_extraction" backend tests | sed -n '1,160p'
echo
echo "== read-only structural/behavioral verifier for overlay keys =="
python3 - <<'PY'
from pathlib import Path
import ast, re
spec_file = Path("backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py")
serializer_file = Path("backend/tool_instance_v2/serializers.py")
const_file = Path("backend/prompt_studio/prompt_studio_registry_v2/constants.py")
tree = ast.parse(spec_file.read_text())
export_props_found = []
default_props_found = []
for node in ast.walk(tree):
if isinstance(node, ast.Dict):
# find dicts whose keys include ENABLE_SINGLE_PASS_EXTRACTION or single_pass_extraction_mode
keys = [v.value if isinstance(v, ast.Constant) else None for v in [k for k in node.keys]]
if "enable_single_pass_extraction" in keys or "single_pass_extraction_mode" in keys:
if node.lineno < 150 and any(isinstance(k, ast.Constant) and k.value in ("single_pass_extraction_mode", "enable_challenge") for k in node.keys):
export_props_found = keys[:10]
elif node.lineno > 250 and node.lineno < 350:
export_props_found = keys[:25]
if node.col_offset == 0 and node.lineno < 120:
default_props_found = [v.value if isinstance(v, ast.Constant) else None for v in [k for k in node.keys]]
serializer_text = serializer_file.read_text()
const_tree = ast.parse(const_file.read_text())
const_dict = {}
for node in ast.walk(const_tree):
if isinstance(node, ast.Assign):
for t in node.targets:
if isinstance(t, ast.Name) and t.id in (
"CHALLENGE_LLM",
"ENABLE_CHALLENGE",
"SUMMARIZE_AS_SOURCE",
"ENABLE_SINGLE_PASS_EXTRACTION",
"ENABLE_HIGHLIGHT",
"ENABLE_WORD_CONFIDENCE",
):
const_dict[t.id] = node.value.value if isinstance(node.value, ast.Constant) else None
export_alias = const_dict["ENABLE_SINGLE_PASS_EXTRACTION"]
spec_key = "single_pass_extraction_mode"
overlay_match = bool(re.search(r"for key in tool_settings:(\s*\n\s*if key in resolved_settings:|.\n\s*if export_key in resolved_settings:)", serializer_text))
print("export_setting_key=", export_alias)
print("spec_property_key=", spec_key)
print("export_setting_matches_spec_property=", export_alias == spec_key)
print("overlay_checks_resolved_settings_key=", overlay_match)
print("overlapping_props=", [k for k in sorted(export_props_found or []) if k in sorted(default_props_found or [])])
PYRepository: Zipstack/unstract Length of output: 2922 Overlay silently drops
🔧 One way to close the gap+ # frame_export_json exports a couple of settings under a different
+ # key than the spec property name.
+ _EXPORT_KEY_ALIASES = {"single_pass_extraction_mode": "enable_single_pass_extraction"}
for key in tool_settings:
- if key in resolved_settings:
- tool_settings[key] = resolved_settings[key]
+ export_key = _EXPORT_KEY_ALIASES.get(key, key)
+ if export_key in resolved_settings:
+ tool_settings[key] = resolved_settings[export_key]📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if TIKey.STEP not in validated_data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validated_data[TIKey.STEP] = workflow.tool_instances.count() + 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
New helper logs a WARNING on every non-Prompt-Studio tool-instance creation.
get_resolved_settingsis called unconditionally fromToolInstanceSerializer.create()for every tool type viastr(tool_uid).prompt_registry_idis a UUID primary key, so any non-Prompt-Studiotool_uid(the common case — built-in/registry tools) will failPromptStudioRegistry.objects.get(pk=...)and hit theexcept Exceptionbranch, logging a warning on every single such creation. The docstring frames "missing settings" as an expected no-op, but the implementation logs it as a warning regardless, which will flood production logs and mask genuine errors.Short-circuit on an invalid UUID before touching the DB/logger:
🔇 Proposed fix
Note: the existing test double for this function (
test_get_resolved_settings_reads_the_tool_settings_block) would still pass unchanged since it only exercises the DB-lookup branch.🧰 Tools
🪛 Ruff (0.15.21)
[warning] 162-162: Do not catch blind exception:
Exception(BLE001)
🤖 Prompt for AI Agents
Source: Linters/SAST tools