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

Align with fractal-server=1.2.5 #481

Merged
merged 20 commits into from
May 10, 2023
Merged

Conversation

tcompa
Copy link
Collaborator

@tcompa tcompa commented May 10, 2023

No description provided.

@tcompa tcompa linked an issue May 10, 2023 that may be closed by this pull request
@tcompa
Copy link
Collaborator Author

tcompa commented May 10, 2023

CI passes with 1.2.4, but it fails with 1.2.5.
The likely candidate for this behavior, on the fractal-server side, is this diff ($ git diff 1.2.4 1.2.5 fractal_server/app/db/__init__.py):

diff --git a/fractal_server/app/db/__init__.py b/fractal_server/app/db/__init__.py
index 9a3cf371..69dedc2f 100644
--- a/fractal_server/app/db/__init__.py
+++ b/fractal_server/app/db/__init__.py
@@ -10,6 +10,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.ext.asyncio import create_async_engine
 from sqlalchemy.orm import Session as DBSyncSession
 from sqlalchemy.orm import sessionmaker
+from sqlalchemy.pool import StaticPool
 
 from ...config import get_settings
 from ...logger import set_logger
@@ -52,18 +53,28 @@ class DB:
                 "the database cannot be guaranteed."
             )
 
+        # Set some sqlite-specific options
+        if settings.DB_ENGINE == "sqlite":
+            engine_kwargs_async = dict(poolclass=StaticPool)
+            engine_kwargs_sync = dict(
+                poolclass=StaticPool,
+                connect_args={"check_same_thread": False},
+            )
+        else:
+            engine_kwargs_async = {}
+            engine_kwargs_sync = {}
+
         cls._engine_async = create_async_engine(
-            settings.DATABASE_URL, echo=settings.DB_ECHO, future=True
+            settings.DATABASE_URL,
+            echo=settings.DB_ECHO,
+            future=True,
+            **engine_kwargs_async,
         )
         cls._engine_sync = create_engine(
             settings.DATABASE_SYNC_URL,
             echo=settings.DB_ECHO,
             future=True,
-            connect_args=(
-                {"check_same_thread": False}
-                if settings.DB_ENGINE == "sqlite"
-                else {}
-            ),
+            **engine_kwargs_sync,
         )
 
         cls._async_session_maker = sessionmaker(

@tcompa
Copy link
Collaborator Author

tcompa commented May 10, 2023

EDIT:
1.2.4 fails with the usual "twophase" error, which is known on the fractal-server side (ref fractal-analytics-platform/fractal-server#661). This is actually expected.

@tcompa
Copy link
Collaborator Author

tcompa commented May 10, 2023

What is unexpected, however, is that 1.2.5 also fails - with a bunch of httpx.ReadTimeout. This is clearly a CI problem, since going through a manual test (startup a fractal-server instance, and use the fractal client as in fractal-demos) does work.

@github-actions
Copy link

Coverage report

The coverage rate went from 92.35% to 92.44% ⬆️
The branch rate is 83%.

None of the new lines are part of the tested code. Therefore, there is no coverage data about them.

@tcompa
Copy link
Collaborator Author

tcompa commented May 10, 2023

For the record, if we switched back from StaticPool to NullPool (in the sqlite engine), tests would pass.

EDIT: more precisely, to make the tests pass it is enough to switch to NullPool for the sync db engine.

tcompa added a commit that referenced this pull request May 10, 2023
This should solve an issue described in #481, where the use of StaticPool DB connections was breaking something in the fractal client CI
@tcompa tcompa merged commit 1e2f4ab into main May 10, 2023
@tcompa tcompa deleted the 480-align-with-fractal-server=125 branch May 10, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align with fractal-server=1.2.5
1 participant