feat(wren-ai-service): add global date instruction to all SQL pipelines #1899
Conversation
WalkthroughAdds configuration-driven current_time propagation into multiple generation pipeline prompts and prompt_builder.run calls. Several prompt() signatures now accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Pipeline Caller
participant PromptFn as prompt()
participant Config as Configuration
participant Builder as PromptBuilder
Caller->>PromptFn: prompt(..., configuration=Configuration())
activate PromptFn
PromptFn->>Config: show_current_time()
Config-->>PromptFn: "YYYY-MM-DD HH:MM:SS"
PromptFn->>Builder: run(..., current_time=<value>)
activate Builder
Note over Builder: Templates include "Current Time: {{ current_time }}"
Builder-->>PromptFn: Rendered prompts
deactivate Builder
PromptFn-->>Caller: Prompt payload
deactivate PromptFn
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-ai-service/src/pipelines/generation/utils/sql.py (3)
3-3: Use a timezone-aware datetime importRelying on naive datetimes can mis-anchor “today” at runtime depending on server locale/timezone. Import timezone to anchor to UTC (or a configured tz).
-from datetime import datetime +from datetime import datetime, timezone
488-500: Anchor “today” to a specific timezone and add ISO-8601 for locale safetyThis achieves the PR goal while avoiding ambiguity:
- Use UTC to avoid server-local timezone drift.
- Include ISO-8601 alongside the human-readable form (strftime’s month name is locale-dependent).
- Filter out None/empty user instructions to avoid polluting the instruction list.
- - # Always add global date instruction - today = datetime.now().strftime("%B %d, %Y") # e.g., "August 18, 2025" - global_instruction = f"Today's date is {today}. When users ask about 'today', 'current', 'latest', or use relative time expressions like 'this month', 'this year', use this date as the reference point." - _instructions.append(global_instruction) - - # Add user-provided instructions - if instructions: - _instructions += [ - instruction.get("instruction") for instruction in instructions - ] + # Always add global date instruction (UTC, include ISO for clarity) + today_date = datetime.now(timezone.utc).date() + human = today_date.strftime("%B %d, %Y") # e.g., "August 18, 2025" + iso = today_date.isoformat() + global_instruction = ( + f"Today's date is {human} (ISO: {iso}, timezone: UTC). " + "When users ask about 'today', 'current', 'latest', or use relative time expressions like 'this month' or 'this year', use this date as the reference point." + ) + _instructions.append(global_instruction) + + # Add user-provided instructions (filter out empty/None) + if instructions: + _instructions += [ + ins for ins in (i.get("instruction") for i in instructions) if ins + ]If a different timezone (e.g., project/user/data-source) is desired, we can parameterize this or read from config without breaking callers.
488-500: Make date instructions timezone-aware and validate instruction list
- In wren-ai-service/src/pipelines/generation/utils/sql.py (around lines 488–500), replace the naive
datetime.now()with an explicit timezone. For example:- today = datetime.now().strftime("%B %d, %Y") + from zoneinfo import ZoneInfo # or use pytz + tz = ZoneInfo(configuration.timezone) # e.g., "UTC" or your project/user locale + today = datetime.now(tz).strftime("%B %d, %Y")- Audit all callers of
construct_instructions(e.g. in followup_sql_generation.py:105, sql_generation_reasoning.py:70, sql_generation.py:101, sql_regeneration.py:114, intent_classification.py:285, followup_sql_generation_reasoning.py:81) to confirm they always pass a list of dicts containing the"instruction"key.
• If there’s any doubt, add a guard such as:cleaned = [ inst["instruction"] for inst in (instructions or []) if isinstance(inst, dict) and "instruction" in inst ] _instructions += cleaned- Ensure your deployment config or user settings expose a canonical timezone (UTC or otherwise) so this change remains consistent across environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wren-ai-service/src/pipelines/generation/utils/sql.py(2 hunks)
🔇 Additional comments (1)
wren-ai-service/src/pipelines/generation/utils/sql.py (1)
488-494: LGTM: Global date context added first, matching PR objectivesPlacing the date instruction at the top ensures consistent temporal grounding across pipelines. This should improve handling of “today/this month/current year”.
- Current Time parameter added to all SQL generation templates
- Consistent temporal context across all SQL pipelines
- Uses configuration.show_current_time() for proper formatting
- Improves handling of temporal queries ("today", "this month", etc.)
Extends existing current_time pattern to intent classification,
sql_generation, sql_correction, followup_sql_generation, and sql_regeneration.
OmarAhmed-A
force-pushed
the
feat/global-date-instructions
branch
from
195e6a9 to
c1fa959
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
wren-ai-service/src/pipelines/generation/intent_classification.py (2)
290-291: Guard against None configuration before calling show_current_time().
configurationis annotated as optional in this module, but it’s dereferenced unconditionally here. Ifprompt()is ever invoked outside the pipeline withconfiguration=None, this will raise.Minimal, localized hardening:
- current_time=configuration.show_current_time(), + current_time=(configuration or Configuration()).show_current_time(),Additionally, consider normalizing
configurationat function entry (outside this hunk) to also protectlanguage=configuration.language:def prompt(..., configuration: Configuration | None = None) -> dict: configuration = configuration or Configuration() _prompt = prompt_builder.run( query=query, language=configuration.language, ... current_time=configuration.show_current_time(), )And for consistency with other modules in this PR, you may also switch the signature default to
configuration: Configuration = Configuration().
159-160: Date format currently doesn’t match PR requirement (example: “August 18, 2025”).The PR calls for a clear, unambiguous date format; however,
Configuration.show_current_time()returnsYYYY-MM-DD Weekday HH:MM:SS. Consider switching to a month-name format and including explicit timezone for clarity.Proposed update to
src/web/v1/services/__init__.py(outside this file):def show_current_time(self): tz = pytz.timezone(self.timezone.name) now = datetime.now(tz) # Example: "August 19, 2025 14:03:25 UTC (America/Los_Angeles)" return f"{now.strftime('%B %d, %Y %H:%M:%S')} {now.tzname()} ({self.timezone.name})"If you prefer to only surface the date (as your example suggests), use:
return f"{now.strftime('%B %d, %Y')} ({self.timezone.name})"Please confirm which variant you want and I can push a patch.
wren-ai-service/src/pipelines/generation/sql_correction.py (1)
58-59: Optional: align the displayed date format with the PR’s “August 18, 2025” guidance.The current
show_current_time()usesYYYY-MM-DD Weekday HH:MM:SS. If you want to avoid ambiguity, consider switching to a month-name format and include the timezone string.Would you like me to open a follow-up PR to adjust
show_current_time()accordingly?wren-ai-service/src/pipelines/generation/sql_generation.py (1)
74-75: Optional: update date rendering to match the PR’s “August 18, 2025” style.If you want to strictly follow the example, update
show_current_time()to output a month-name date and explicit timezone string.wren-ai-service/src/pipelines/generation/followup_sql_generation.py (1)
80-81: Optional: match date format to requirement.Consider switching to a month-name date format with timezone to align with the PR description.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
wren-ai-service/src/pipelines/generation/followup_sql_generation.py(4 hunks)wren-ai-service/src/pipelines/generation/intent_classification.py(2 hunks)wren-ai-service/src/pipelines/generation/sql_correction.py(3 hunks)wren-ai-service/src/pipelines/generation/sql_generation.py(4 hunks)wren-ai-service/src/pipelines/generation/sql_regeneration.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
wren-ai-service/src/pipelines/generation/sql_correction.py (2)
wren-ai-service/src/web/v1/services/__init__.py (2)
Configuration(26-41)show_current_time(31-38)wren-ai-service/src/pipelines/generation/sql_question.py (1)
run(115-128)
wren-ai-service/src/pipelines/generation/intent_classification.py (1)
wren-ai-service/src/web/v1/services/__init__.py (1)
show_current_time(31-38)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (1)
wren-ai-service/src/web/v1/services/__init__.py (2)
Configuration(26-41)show_current_time(31-38)
wren-ai-service/src/pipelines/generation/followup_sql_generation.py (1)
wren-ai-service/src/web/v1/services/__init__.py (2)
Configuration(26-41)show_current_time(31-38)
wren-ai-service/src/pipelines/generation/sql_generation.py (2)
wren-ai-service/src/web/v1/services/__init__.py (2)
Configuration(26-41)show_current_time(31-38)wren-ai-service/src/providers/__init__.py (1)
Configuration(301-303)
🪛 Ruff (0.12.2)
wren-ai-service/src/pipelines/generation/sql_regeneration.py
26-26: Redefinition of unused Configuration from line 25
Remove definition: Configuration
(F811)
🔇 Additional comments (5)
wren-ai-service/src/pipelines/generation/intent_classification.py (1)
159-162: Good addition: injecting current time into the prompt improves temporal intent handling.Including
Current Time: {{ current_time }}directly in the user prompt aligns with the PR objective and will help the model interpret relative-time phrases.wren-ai-service/src/pipelines/generation/sql_correction.py (1)
58-61: LGTM: current time wiring and configuration passthrough are correct.
Current Time: {{ current_time }}added to the template.prompt()signature now acceptsconfigurationwith a sane default.prompt_builder.runis fedcurrent_time=configuration.show_current_time().These changes are coherent and align with the PR objective.
Also applies to: 70-76
wren-ai-service/src/pipelines/generation/sql_regeneration.py (1)
93-96: LGTM: time-aware prompt and configuration plumbed through.
- Added
Current Time: {{ current_time }}to the template.prompt()now acceptsconfiguration: Configuration = Configuration().prompt_builder.runcorrectly passescurrent_time.This matches the pattern used across the other pipelines in this PR.
Also applies to: 112-113, 128-129
wren-ai-service/src/pipelines/generation/sql_generation.py (1)
25-26: LGTM: consistent current time integration.
- Imported
Configuration.- Added
Current Time: {{ current_time }}to the template.prompt()acceptsconfigurationwith a default and forwardscurrent_timeto the builder.This is consistent with the broader changes and should improve handling of relative-time queries.
Also applies to: 74-76, 98-99, 114-115
wren-ai-service/src/pipelines/generation/followup_sql_generation.py (1)
26-27: LGTM: follow-up generation now time-aware.
- Imported
Configuration.- Injected
Current Time: {{ current_time }}into the follow-up template.prompt()acceptsconfigurationand propagatescurrent_time.Matches the pattern across other generation pipelines in this PR.
Also applies to: 80-81, 102-103, 118-119
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (1)
139-139: Critical:regenerate_sql(and other pipelines) return a tuple, breaking theirpost_processsteps
All of these functions are annotated to return adictbut currently do:return await generator(prompt=prompt.get("prompt")), generator_nameThis causes
AttributeErrorinpost_processwhen calling.get("replies")on a tuple.Files requiring the same fix:
- src/pipelines/generation/sql_regeneration.py:139
- src/pipelines/generation/chart_generation.py:91
- src/pipelines/generation/chart_adjustment.py:120
- src/pipelines/generation/sql_correction.py:85
- src/pipelines/generation/question_recommendation.py:49
- src/pipelines/generation/intent_classification.py:298
- src/pipelines/generation/sql_tables_extraction.py:71
- src/pipelines/generation/semantics_description.py:75
- src/pipelines/generation/relationship_recommendation.py:62
- src/pipelines/generation/sql_question.py:67
- src/pipelines/generation/sql_generation.py:126
- src/pipelines/generation/sql_generation_reasoning.py:84
Apply this patch in each place to return only the LLM result:
- return await generator(prompt=prompt.get("prompt")), generator_name + return await generator(prompt=prompt.get("prompt"))
♻️ Duplicate comments (1)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (1)
25-26: Fix broken import: stray diff marker and duplicate/indented import cause SyntaxError (and F811).Line 25 contains a leading "-" (leftover diff marker) and Line 26 is indented. Both trigger SyntaxError per static analysis, and the duplicate import trips Ruff F811. Keep a single, properly unindented import.
Apply this diff:
-from src.web.v1.services import Configuration - from src.web.v1.services import Configuration +from src.web.v1.services import Configuration
🧹 Nitpick comments (2)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (2)
111-114: Avoid instantiating Configuration as a default argument (shared mutable default).Using a Pydantic/BaseModel instance as a default can lead to shared state across invocations. Prefer None + instantiate inside.
Apply this diff:
- configuration: Configuration = Configuration(), + configuration: Configuration | None = None, ) -> dict: - _prompt = prompt_builder.run( + if configuration is None: + configuration = Configuration() + _prompt = prompt_builder.run(
187-201: Optional: Threadconfigurationthrough the pipeline to allow non-default timezone.Right now the
promptnode has aconfigurationparam, butSQLRegeneration.run()doesn’t accept/pass it, so callers can’t override the default UTC/timezone. If you want request- or project-scoped timezones, acceptconfiguration: Configuration | None = Noneinrun()and pass it intoinputs.Example:
# Signature async def run(..., sql_functions: list[SqlFunction] | None = None, configuration: Configuration | None = None,): # Inputs inputs={ ... "sql_functions": sql_functions, "configuration": configuration or Configuration(), **self._components, **self._configs, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wren-ai-service/src/pipelines/generation/sql_regeneration.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (1)
wren-ai-service/src/web/v1/services/__init__.py (2)
Configuration(26-41)show_current_time(31-38)
🪛 Ruff (0.12.2)
wren-ai-service/src/pipelines/generation/sql_regeneration.py
25-25: SyntaxError: Expected an identifier, but found a keyword 'from' that cannot be used here
25-25: SyntaxError: Simple statements must be separated by newlines or semicolons
25-25: SyntaxError: Simple statements must be separated by newlines or semicolons
26-26: SyntaxError: Unexpected indentation
🔇 Additional comments (2)
wren-ai-service/src/pipelines/generation/sql_regeneration.py (2)
92-92: Good addition: explicit “Current Time” context in the user prompt.This aligns with the PR objective to make relative time references reliable across SQL pipelines.
127-127: Correct propagation of formatted current time.Passing
current_time=configuration.show_current_time()ensures consistent, timezone-aware formatting.
|
@OmarAhmed-A there is conflict with the main branch and please make sure the pr is the latest to the main branch, thanks! |
|
@OmarAhmed-A thanks for raising the pr! Since the update include several time related instructions in multiple pipelines, I suggest you try different kinds of questions that include time, see how it works and share with us. I could do this on my end here too, but not my high priority as of now. Welcome to discuss the testing results with me in discord. Thanks |
2 participants
Extends existing current_time pattern to intent classification,
sql_generation, sql_correction, followup_sql_generation, and sql_regeneration.
Fixes #1898
Summary by CodeRabbit