Code consistency#358
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @PGijsbers, your pull request is larger than the review limit of 150000 diff characters
for more information, see https://pre-commit.ci
WalkthroughThe pull request migrates database access from Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
===========================================
+ Coverage 77.60% 94.44% +16.83%
===========================================
Files 77 77
Lines 3774 3833 +59
Branches 247 248 +1
===========================================
+ Hits 2929 3620 +691
+ Misses 759 145 -614
+ Partials 86 68 -18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routers/datasets.py (1)
137-156: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winInvalid Python 3 except clause
except DatasetNotFoundError, DatasetNoAccessError:is syntax-invalid and will stopsrc/routers/datasets.pyfrom loading. Useexcept (DatasetNotFoundError, DatasetNoAccessError):instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routers/datasets.py` around lines 137 - 156, Fix the invalid exception syntax in the untag_dataset function by changing the except clause to catch DatasetNotFoundError and DatasetNoAccessError as a tuple: except (DatasetNotFoundError, DatasetNoAccessError):.
🧹 Nitpick comments (1)
src/database/tasks.py (1)
21-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftInconsistent DB-handle parameter naming (
expdbvssession) undermines this PR's consistency goal.Within this same file, most functions take
expdb: AsyncSession(Lines 21, 35, 47, 61, 82, 104-106, 121, 135-137) whileget_tagsandtagtakesession: AsyncSession(Lines 152, 162). This split isn't tied to raw-SQL vs ORM usage either —src/database/setups.py'sget()(also raw SQL viasession.execute) usessession, whilesrc/database/runs.py/flows.py'sget()useexpdb. Since this PR's explicit goal is naming/usage consistency, consider standardizing on one parameter name across all database helper modules.Also applies to: 152-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/tasks.py` around lines 21 - 149, Standardize the database-handle parameter name across the helper modules to match the consistency goal. Rename the inconsistent `expdb`/`session` parameters in `get`, `get_task_types`, `get_task_type`, `get_task_type_name`, `get_task_evaluation_measure`, `get_input_for_task_type`, `get_input_for_task`, `get_task_type_inout_with_template`, `get_tags`, and `tag`, updating all internal references and call sites consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/development/tests.md`:
- Around line 95-106: Update the AsyncConnection.execute calls in
test_python_and_php to use the parameters= keyword instead of params= for both
the dataset INSERT and DELETE statements.
In `@src/routers/tasks.py`:
- Around line 472-474: In the loop building filled_templates, replace the unused
name variable in the input_templates iteration with an underscore while
retaining template for fill_template.
In `@tests/routers/users_delete_test.py`:
- Around line 55-61: Update the stale comment near the user setup in the test to
replace the old user_test fixture reference with userdb_session, and accurately
state that rollback is performed by the dependent userdb_connection fixture.
Keep the comment consistent with the current fixture names and behavior.
---
Outside diff comments:
In `@src/routers/datasets.py`:
- Around line 137-156: Fix the invalid exception syntax in the untag_dataset
function by changing the except clause to catch DatasetNotFoundError and
DatasetNoAccessError as a tuple: except (DatasetNotFoundError,
DatasetNoAccessError):.
---
Nitpick comments:
In `@src/database/tasks.py`:
- Around line 21-149: Standardize the database-handle parameter name across the
helper modules to match the consistency goal. Rename the inconsistent
`expdb`/`session` parameters in `get`, `get_task_types`, `get_task_type`,
`get_task_type_name`, `get_task_evaluation_measure`, `get_input_for_task_type`,
`get_input_for_task`, `get_task_type_inout_with_template`, `get_tags`, and
`tag`, updating all internal references and call sites consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a86116c-015e-494e-8247-fbe17668d0cd
📒 Files selected for processing (57)
docs/development/tests.mdsrc/core/access.pysrc/database/datasets.pysrc/database/evaluations.pysrc/database/flows.pysrc/database/qualities.pysrc/database/runs.pysrc/database/setups.pysrc/database/studies.pysrc/database/tasks.pysrc/database/users.pysrc/routers/datasets.pysrc/routers/dependencies.pysrc/routers/estimation_procedure.pysrc/routers/evaluations.pysrc/routers/flows.pysrc/routers/qualities.pysrc/routers/runs.pysrc/routers/schemas/__init__.pysrc/routers/schemas/core.pysrc/routers/schemas/datasets.pysrc/routers/schemas/flows.pysrc/routers/schemas/runs.pysrc/routers/schemas/setups.pysrc/routers/schemas/study.pysrc/routers/schemas/tasks.pysrc/routers/setups.pysrc/routers/study.pysrc/routers/tasks.pysrc/routers/tasktype.pysrc/routers/users.pytests/conftest.pytests/database/flows_test.pytests/database/runs_test.pytests/dependencies/fetch_user_test.pytests/routers/dataset_tag_test.pytests/routers/dataset_untag_test.pytests/routers/datasets_features_test.pytests/routers/datasets_get_test.pytests/routers/datasets_list_datasets_test.pytests/routers/datasets_qualities_test.pytests/routers/datasets_status_test.pytests/routers/flows_exists_test.pytests/routers/flows_get_test.pytests/routers/qualities_list_test.pytests/routers/runs_get_test.pytests/routers/runs_trace_test.pytests/routers/setups_get_test.pytests/routers/setups_tag_test.pytests/routers/setups_untag_test.pytests/routers/study_attach_test.pytests/routers/tag_test_helper.pytests/routers/task_get_test.pytests/routers/task_list_test.pytests/routers/task_tag_test.pytests/routers/task_type_get_test.pytests/routers/users_delete_test.py
| async def test_python_and_php(py_api: httpx.AsyncClient, php_api: httpx.AsyncClient, expdb_connection: AsyncConnection) -> None: | ||
| await expdb_connection.execute(text("INSERT INTO dataset ..."), params=...) # Insert dataset with id 42 | ||
| await expdb_connection.commit() # We need to persist the data in the database, because the PHP REST API cannot see our transaction | ||
|
|
||
| response = await php_api.get("/datasets/42") # The PHP REST API can see the dataset, because it exists in the database | ||
| response = await py_api.get("/datasets/42") # The Python REST API can see the dataset also | ||
|
|
||
| # We need to clean up after ourselves, otherwise the test has side effects. | ||
| # This isn't a great pattern, prefer instead the use of context managers which will execute the delete statements even if unexpected exceptions occur. | ||
| await expdb_connection.execute(text("DELETE FROM dataset ..."), params=...) | ||
| await expdb_connection.commit() | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check SQLAlchemy version and AsyncConnection.execute signature
rg -n "sqlalchemy" pyproject.toml uv.lock requirements*.txt 2>/dev/null | head -20
python -c "import inspect; from sqlalchemy.ext.asyncio import AsyncConnection; print(inspect.signature(AsyncConnection.execute))" 2>/dev/null || echo "Could not inspect AsyncConnection.execute"Repository: openml/server-api
Length of output: 1929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate dependency declarations for SQLAlchemy
git ls-files | rg '^(pyproject\.toml|uv\.lock|requirements.*\.txt|poetry\.lock|Pipfile.lock)$' || true
for f in pyproject.toml uv.lock requirements.txt requirements-dev.txt requirements*.txt poetry.lock Pipfile.lock; do
if [ -f "$f" ]; then
echo "### $f"
rg -n "sqlalchemy|SQLAlchemy" "$f" || true
fi
done
# Inspect the installed SQLAlchemy signature if available in the environment
python3 - <<'PY'
import inspect
try:
from sqlalchemy.ext.asyncio import AsyncConnection
print(inspect.signature(AsyncConnection.execute))
except Exception as e:
print(f"Could not inspect AsyncConnection.execute: {e}")
PYRepository: openml/server-api
Length of output: 1994
🌐 Web query:
SQLAlchemy AsyncConnection.execute parameters params signature AsyncSession.execute docs
💡 Result:
In SQLAlchemy, the execute method signatures differ between AsyncConnection and AsyncSession, specifically regarding the parameter argument name. AsyncConnection.execute The signature for AsyncConnection.execute is: async def execute(self, statement: Executable, parameters: _CoreAnyExecuteParams | None = None, *, execution_options: CoreExecuteOptionsParameter | None = None) -> CursorResult[Any] The parameter holding the bind values is named parameters [1][2]. It accepts either a single dictionary (for a single execution) or a sequence of dictionaries (for executemany behavior) [1][2]. AsyncSession.execute The signature for AsyncSession.execute uses params instead of parameters: async def execute(self, statement: Executable, params: Optional[_CoreAnyExecuteParams] = None, *, execution_options: OrmExecuteOptionsParameter = util.EMPTY_DICT, bind_arguments: Optional[_BindArguments] = None, kw: Any) -> Result[Unpack[TupleAny]] In the AsyncSession, the argument is named params [3]. This aligns with the interface of the synchronous ORM Session.execute method [3]. Summary of Key Differences Component | Argument Name for Bind Values | Reference --- | --- | --- AsyncConnection.execute | parameters [1] | [1], [4] AsyncSession.execute | params [3] | [3] Both methods are awaitable and return result objects [1][3]. When using these methods, ensure you use the argument name specific to the object type you are interacting with (Connection vs. Session) to avoid keyword argument errors.
Citations:
- 1: https://docs.sqlalchemy.org/en/20/orm/extensions/asyncio.html
- 2: https://www.sqlalchemy.org/docs/orm/extensions/asyncio.html
- 3: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/ext/asyncio/session.py
- 4: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/ext/asyncio/engine.py
Use parameters= for AsyncConnection.execute(). expdb_connection is an AsyncConnection, so lines 96 and 104 should pass parameters=; params= is for AsyncSession.execute() and will raise a TypeError here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/development/tests.md` around lines 95 - 106, Update the
AsyncConnection.execute calls in test_python_and_php to use the parameters=
keyword instead of params= for both the dataset INSERT and DELETE statements.
| filled_templates = [] | ||
| for name, template in input_templates: | ||
| filled_templates.append(await fill_template(template, task, task_inputs, expdb)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused loop variable name (Ruff B007).
name from input_templates is never used inside the loop body; only template is used.
🔧 Proposed fix
filled_templates = []
- for name, template in input_templates:
+ for _name, template in input_templates:
filled_templates.append(await fill_template(template, task, task_inputs, expdb))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filled_templates = [] | |
| for name, template in input_templates: | |
| filled_templates.append(await fill_template(template, task, task_inputs, expdb)) | |
| filled_templates = [] | |
| for _name, template in input_templates: | |
| filled_templates.append(await fill_template(template, task, task_inputs, expdb)) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 473-473: Loop control variable name not used within loop body
(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routers/tasks.py` around lines 472 - 474, In the loop building
filled_templates, replace the unused name variable in the input_templates
iteration with an underscore while retaining template for fill_template.
Source: Linters/SAST tools
| params={"username": username, "email": email, "api_key": api_key}, | ||
| ) | ||
| uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) | ||
| uid_row = await userdb_session.execute(text("SELECT LAST_INSERT_ID() AS id")) | ||
| (new_id,) = uid_row.one() | ||
| await user_test.execute( | ||
| await userdb_session.execute( | ||
| text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), | ||
| parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, | ||
| params={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update stale comment referencing the old user_test fixture name.
Line 64 still references user_test as the fixture that handles rollback, but this PR renamed it to userdb_session. The actual rollback is performed by the userdb_connection fixture that userdb_session depends on.
📝 Proposed fix
return DisposableUser(user_id=new_id, api_key=api_key)
- # No explicit teardown: the ``user_test`` fixture rolls back at the end
+ # No explicit teardown: the ``userdb_connection`` fixture rolls back at the end
# of the test, which removes the rows inserted above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/routers/users_delete_test.py` around lines 55 - 61, Update the stale
comment near the user setup in the test to replace the old user_test fixture
reference with userdb_session, and accurately state that rollback is performed
by the dependent userdb_connection fixture. Keep the comment consistent with the
current fixture names and behavior.
1 participant
Several changes to make the code base more consistent in naming, typing, and use of connection vs session.