-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix: set prepared threshold to none for migrations #9900
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?
Conversation
WalkthroughAdded connect_args={"prepare_threshold": None} to async_engine_from_config in _run_async_migrations within Alembic env.py so the async migration engine matches the prepare_threshold used elsewhere. No other logic or control flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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. Comment |
|
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.
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)
src/backend/base/langflow/alembic/env.py (1)
94-99
: Guard psycopg-only connect_args; current change can break SQLite/asyncpg at connect timealembic.ini uses sqlite+aiosqlite (src/backend/base/langflow/alembic.ini:66) and env.py passes connect_args={"prepare_threshold": None} into async_engine_from_config (src/backend/base/langflow/alembic/env.py:94-99). Gate prepare_threshold to psycopg drivers only.
-from sqlalchemy.ext.asyncio import async_engine_from_config +from sqlalchemy.ext.asyncio import async_engine_from_config +from sqlalchemy.engine import make_url @@ -async def _run_async_migrations() -> None: - connectable = async_engine_from_config( - config.get_section(config.config_ini_section, {}), - prefix="sqlalchemy.", - poolclass=pool.NullPool, - connect_args={"prepare_threshold": None}, - ) +async def _run_async_migrations() -> None: + db_url = config.get_main_option("sqlalchemy.url") or "" + url = make_url(db_url) + connect_args = {} + # Disable server-side prepares only for psycopg to avoid PgBouncer issues. + if url.get_backend_name() == "postgresql" and "psycopg" in (url.drivername or ""): + connect_args["prepare_threshold"] = None + connectable = async_engine_from_config( + config.get_section(config.config_ini_section, {}), + prefix="sqlalchemy.", + poolclass=pool.NullPool, + connect_args=connect_args or None, + )
🧹 Nitpick comments (1)
src/backend/base/langflow/alembic/env.py (1)
53-60
: prepare_threshold in context.configure is a no-op; remove to avoid confusionAlembic’s context.configure doesn’t consume this option; the effective place is connect_args. Clean it up for clarity.
@@ def run_migrations_offline() -> None: - render_as_batch=True, - prepare_threshold=None, + render_as_batch=True, @@ def _do_run_migrations(connection): - context.configure( - connection=connection, target_metadata=target_metadata, render_as_batch=True, prepare_threshold=None - ) + context.configure( + connection=connection, target_metadata=target_metadata, render_as_batch=True + )Also applies to: 82-84
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/base/langflow/alembic/env.py
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{src/backend/**/*.py,tests/**/*.py,Makefile}
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
{src/backend/**/*.py,tests/**/*.py,Makefile}
: Run make format_backend to format Python code before linting or committing changes
Run make lint to perform linting checks on backend Python code
Files:
src/backend/base/langflow/alembic/env.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 1/2
- GitHub Check: Run Frontend Tests / Playwright Tests - Shard 2/2
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 1
- GitHub Check: Lint Backend / Run Mypy (3.12)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 3
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 5
- GitHub Check: Lint Backend / Run Mypy (3.11)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 4
- GitHub Check: Lint Backend / Run Mypy (3.13)
- GitHub Check: Run Backend Tests / Unit Tests - Python 3.10 - Group 2
- GitHub Check: Run Backend Tests / Integration Tests - Python 3.10
- GitHub Check: Test Starter Templates
- GitHub Check: Update Starter Projects
- GitHub Check: Run Ruff Check and Format
- GitHub Check: Optimize new Python code in this PR
- GitHub Check: Ruff Style Check (3.13)
🔇 Additional comments (1)
src/backend/base/langflow/alembic/env.py (1)
1-3
: Run repo formatting/lint hooksPlease run: make format_backend && make lint to satisfy backend formatting/linting after the import/conditional logic change.
Avoids issue when using pgbouncer with pooled connections. This only affects migrations once, so I am not concerned about hardcoding this.
Summary by CodeRabbit
Bug Fixes
Chores