Skip to content
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

feat: Centralize database connection settings and deprecate old parameters #5960

Merged
merged 6 commits into from
Jan 29, 2025

Conversation

ogabrielluiz
Copy link
Contributor

@ogabrielluiz ogabrielluiz commented Jan 27, 2025

This pull request includes several changes to the DatabaseService and Settings classes to improve database connection handling and configuration. The main changes involve the introduction of new connection settings, deprecating old settings, and refactoring the code to use the new settings.

Database connection handling improvements:

  • Added support for asynchronous SQlite connections by using AsyncAdaptedQueuePool(src/backend/base/langflow/services/database/service.py).
  • Introduced the _build_connection_kwargs method to merge deprecated settings with new db_connection_settings and log warnings for deprecated settings (src/backend/base/langflow/services/database/service.py).
  • Refactored the _create_engine method to use the new db_connection_settings and handle connection pool settings appropriately (src/backend/base/langflow/services/database/service.py).

Configuration updates:

  • Deprecated pool_size and max_overflow settings in favor of db_connection_settings (src/backend/base/langflow/services/settings/base.py).
  • Added db_connection_settings to the Settings class to consolidate common database connection settings (src/backend/base/langflow/services/settings/base.py).

Introduce a new `db_connection_settings` dictionary to centralize database connection parameters. Mark `pool_size` and `max_overflow` as deprecated, recommending the use of the new configuration dictionary instead.
Add a method to build connection kwargs that merges deprecated settings with the new db_connection_settings, providing a more flexible and backwards-compatible approach to database connection configuration.
@ogabrielluiz ogabrielluiz requested a review from cbornet January 27, 2025 18:52
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Jan 27, 2025
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 27, 2025
Explicitly set AsyncAdaptedQueuePool for SQLite connections to address potential async engine configuration issues. This ensures proper pool handling when creating database connections, particularly for SQLite databases.
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 27, 2025
Copy link

codspeed-hq bot commented Jan 27, 2025

CodSpeed Performance Report

Merging #5960 will improve performances by 54.9%

Comparing add-poll-config-to-db-service (71ad5ec) with main (3e2e2ce)

Summary

⚡ 6 improvements
✅ 3 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_build_flow_from_request_data 325.6 ms 289.4 ms +12.51%
test_invalid_run_with_input_type_chat 27 ms 17.4 ms +54.9%
test_successful_run_with_input_type_any 172.6 ms 145.8 ms +18.36%
test_successful_run_with_input_type_text 163.9 ms 142.9 ms +14.72%
test_successful_run_with_output_type_any 154.7 ms 132.7 ms +16.62%
test_successful_run_with_output_type_debug 164.2 ms 142.7 ms +15.07%

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 28, 2025
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 28, 2025
@ogabrielluiz ogabrielluiz added this pull request to the merge queue Jan 29, 2025
@ogabrielluiz ogabrielluiz removed this pull request from the merge queue due to the queue being cleared Jan 29, 2025
@ogabrielluiz ogabrielluiz added this pull request to the merge queue Jan 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2025
@ogabrielluiz ogabrielluiz added this pull request to the merge queue Jan 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2025
Enhance test coverage for `load_bundles_from_urls()` by introducing a mock fixture to simulate zip file content and mocking HTTP requests. This allows testing the bundle loading mechanism without making actual network calls.
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 29, 2025
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 29, 2025
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 29, 2025
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 29, 2025
…erage

Refactor `test_detect_github_url` to use AsyncMock and patch for more robust testing of GitHub URL detection, including verification of API calls and handling of different URL scenarios.
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Jan 29, 2025
@ogabrielluiz ogabrielluiz added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit e0f5cfc Jan 29, 2025
23 checks passed
@ogabrielluiz ogabrielluiz deleted the add-poll-config-to-db-service branch January 29, 2025 11:51
# Even though the docs say this is the default, it raises an error
# if we don't specify it.
# https://docs.sqlalchemy.org/en/20/errors.html#pool-class-cannot-be-used-with-asyncio-engine-or-vice-versa
pool = AsyncAdaptedQueuePool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: could do

kwargs["poolclass"] = AsyncAdaptedQueuePool

and remove pool = None in the else branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants