Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdded an async data-layer function 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
get_feature_ontologies, consider usingrows.mappings()(as inget_features) and accessingrow["index"]/row["value"]for consistency and to avoid relying on attribute-style access to row fields. - When attaching ontologies in
get_dataset_features, you can simplify the loop by usingfeature.ontology = ontologies.get(feature.index)instead of the explicitif feature.index in ontologiescheck.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_feature_ontologies`, consider using `rows.mappings()` (as in `get_features`) and accessing `row["index"]`/`row["value"]` for consistency and to avoid relying on attribute-style access to row fields.
- When attaching ontologies in `get_dataset_features`, you can simplify the loop by using `feature.ontology = ontologies.get(feature.index)` instead of the explicit `if feature.index in ontologies` check.
## Individual Comments
### Comment 1
<location path="src/database/datasets.py" line_range="134-143" />
<code_context>
return [Feature(**row, nominal_values=None) for row in rows.mappings()]
+def get_feature_ontologies(dataset_id: int, connection: Connection) -> dict[int, list[str]]:
+ rows = connection.execute(
+ text(
+ """
+ SELECT `index`, `value`
+ FROM data_feature_description
+ WHERE `did` = :dataset_id AND `description_type` = 'ontology'
+ """,
+ ),
+ parameters={"dataset_id": dataset_id},
+ )
+ ontologies: dict[int, list[str]] = {}
+ for row in rows:
+ ontologies.setdefault(row.index, []).append(row.value)
+ return ontologies
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `row.index` on a SQLAlchemy Row is likely using the positional index method rather than the `index` column value.
Because `Row` defines `index()` for positional lookup, `row.index` is ambiguous and may not return the `index` column value. Similarly, `row.value` is less explicit than using the mapping interface. To avoid subtle bugs, either iterate over `rows.mappings()` and use `row["index"]` / `row["value"]`, or access `row._mapping["index"]` and `row._mapping["value"]` here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/datasets.py`:
- Around line 134-148: The function get_feature_ontologies currently iterates
over SQLAlchemy Row objects and accesses row.index/row.value which can collide
with Row attributes; change the query iteration to use .mappings() (e.g.,
iterate over rows.mappings()) and access columns via mapping["index"] and
mapping["value"] so the ontologies dict is populated safely; update references
in get_feature_ontologies to use the mapping keys instead of attribute access.
|
Thanks for taking the time to contribute! And sorry for the delay. Normally, yes, you would need to add tests. However, in this case #262 also implemented this feature and did add tests. Because you were first (and because this implementation doesn't sort ontology results -- something that seems useless provided they're returned as a mapping), I will go ahead and allow this, and take the tests from the other PR. That way you can both share credit :) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/datasets.py`:
- Around line 137-151: The get_feature_ontologies function is using an undefined
Connection type and is implemented synchronously while the module and callers
use async connections; change its signature to use AsyncConnection, import
AsyncConnection from sqlalchemy.ext.asyncio (or the project's configured alias),
make the function async def get_feature_ontologies(...), await
connection.execute(...) and iterate the result using async-compatible methods
(e.g., rows = await connection.execute(...); for row in rows.mappings(): or use
rows.fetchall() with await if needed), and update any type hints/imports to
remove the undefined Connection reference so callers passing an AsyncConnection
work correctly.
In `@src/routers/openml/datasets.py`:
- Around line 297-300: The call to get_feature_ontologies is currently invoked
synchronously but should be awaited; update the call in the openml datasets
router to use await when calling
database.datasets.get_feature_ontologies(dataset_id, expdb) (so ontologies
receives the resolved result) after you fix get_feature_ontologies to be async
in src/database/datasets.py; keep the subsequent loop that assigns
feature.ontology = ontologies.get(feature.index) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 05f04624-fb4e-4782-9cf9-e27bacf3c48c
📒 Files selected for processing (3)
src/database/datasets.pysrc/routers/openml/datasets.pysrc/schemas/datasets/openml.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/schemas/datasets/openml.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
=======================================
Coverage ? 54.52%
=======================================
Files ? 34
Lines ? 1458
Branches ? 118
=======================================
Hits ? 795
Misses ? 661
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Hi, I haven't read guild lines yet so pardon me in advance and please let me know if you need any changes - Fixes #237
Few Questions-
CC: @PGijsbers