[ENHANCEMENT] argilla-server: List records endpoint using db
#5170
+157
−76
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…5013) # Description This PR is the first one related with distribution task feature, adding the following changes: * Added `distribution` JSON column to `datasets` table: * This column is non-nullable so a value is always required when a dataset is created. * By default old datasets will have the value `{"strategy": "overlap", "min_submitted": 1}`. * Added `distribution` attribute to `DatasetCreate` schema: * None is not a valid value. * If no value is specified for this attribute `DatasetOverlapDistributionCreate` with `min_submitted` to `1` is used. * `DatasetOverlapDistributionCreate` only allows values greater or equal than `1` for `min_submitted` attributed. * Now the context `create_dataset` function is receiving a dictionary instead of `DatasetCreate` schema. * Moved dataset creation validations to a new `DatasetCreateValidator` class. Update of `distribution` attribute for datasets will be done in a different issue. Closes #5005 **Type of change** (Please delete options that are not relevant. Remember to title the PR according to the type of change) - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Refactor (change restructuring the codebase without changing functionality) - [ ] Improvement (change adding some improvement to an existing functionality) - [ ] Documentation update **How Has This Been Tested** (Please describe the tests that you ran to verify your changes. And ideally, reference `tests`) - [x] Adding new tests and passing old ones. - [x] Check that migration works as expected with old datasets and SQLite. - [x] Check that migration works as expected with old datasets and PostgreSQL. **Checklist** - [ ] I added relevant documentation - [ ] follows the style guidelines of this project - [ ] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Paco Aranda <[email protected]>
…nly (#5148) # Description Add changes to `responses_submitted` relationship to avoid problems with existent `responses` relationship and avoid a warning message that SQLAlchemy was reporting. Refs #5000 **Type of change** - Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** - [x] Warning is not showing anymore. - [x] Test are passing. **Checklist** - I added relevant documentation - follows the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
# Description
This PR adds changes to the endpoints to get the dataset progress and
current user metrics in the following way:
## `GET /datasets/:dataset_id/progress`
I have changed the endpoint to support the new business logic behind the
distribution task. Responding with only `completed` and `pending` type
of records and using `total` as the sum of the two types of records.
Old response without distribution task:
```json
{
"total": 8,
"submitted": 2,
"discarded": 2,
"conflicting": 1,
"pending": 3
}
```
New response with the changes from this PR supporting distribution task:
* The `completed` attribute will have the count of all the records with
status as `completed` for the dataset.
* The `pending` attribute will have the count of all the records with
status as `pending` for the dataset.
* The `total` attribute will have the sum of the `completed` and
`pending` attributes.
```json
{
"total": 5
"completed": 2,
"pending": 3,
}
```
@damianpumar some changes are required on the frontend to support this
new endpoint structure.
## `GET /me/datasets/:dataset_id/metrics`
Old response without distribution task:
```json
{
"records": {
"count": 7
},
"responses": {
"count": 4,
"submitted": 1,
"discarded": 2,
"draft": 1
}
}
```
New response with the changes from this PR supporting distribution task:
* `records` section has been eliminated because is not necessary
anymore.
* `responses` `count` section has been renamed to `total`.
* `pending` section has been added to the `responses` section.
```json
{
"responses": {
"total": 7,
"submitted": 1,
"discarded": 2,
"draft": 1,
"pending": 3
}
}
```
The logic behind these attributes is the following:
* `total` is the sum of `submitted`, `discarded`, `draft` and `pending`
attribute values.
* `submitted` is the count of all responses belonging to the current
user in the specified dataset with `submitted` status.
* `discarded` is the count of all responses belonging to the current
user in the specified dataset with `discarded` status.
* `draft` is the count of all responses belonging to the current user in
the specified dataset with `draft` status.
* `pending` is the count of all records with `pending` status for the
dataset that has not responses belonging to the current user.
@damianpumar some changes are required on the frontend to support this
new endpoint structure as well.
Closes #5139
**Type of change**
- Breaking change (fix or feature that would cause existing
functionality to not work as expected)
**How Has This Been Tested**
- [x] Modifying existent tests.
- [x] Running test suite with SQLite and PostgreSQL.
**Checklist**
- I added relevant documentation
- follows the style guidelines of this project
- I did a self-review of my code
- I made corresponding changes to the documentation
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Paco Aranda <[email protected]>
Co-authored-by: Damián Pumar <[email protected]>
#5153) <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> This PR is the first of a series of PRs for cleaning the listing-records-related endpoints. This PR removes the `GET /api/v1/me/datasets/:dataset_id/records` endpoint since the only client was the frontend application and now is using the equivalent search endpoint. **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - follows the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
…aram (#5156) # Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> > [!NOTE] > This PR must be merged after #5153 This PR removes support for filtering using metadata as a query param: - This filter is not available anymore for list endpoints - The metadata filter can be defined as part of the request body for search filters. **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Refactor (change restructuring the codebase without changing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - follows the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
…upport (#5163) # Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> > [!NOTE] > This PR must be merged after #5156 This PR removes support for filtering records with response_status query param: - This filter is removed for listing records endpoints - The response status filter is available for search endpoints using the filter request body. **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Refactor (change restructuring the codebase without changing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - I followed the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: José Francisco Calvo <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5170 +/- ##
===========================================
+ Coverage 91.14% 91.23% +0.08%
===========================================
Files 145 145
Lines 5951 5964 +13
===========================================
+ Hits 5424 5441 +17
+ Misses 527 523 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
frascuchon
changed the title
argilla-server: Llist records endpoint using dbargilla-server: List records endpoint using db
|
As discussed we will review this PR again once we solve database sessions with different isolation levels for distribution task. |
…notated datasets (#5171) # Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> This PR changes the current validator when updating the distribution task to allow updating the distribution task settings for datasets with records without ANY response. cc @nataliaElv **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - I followed the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
…5166) # Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> This PR removes support of `sort_by` query param for list/search records endpoints. **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Breaking change (fix or feature that would cause existing functionality to not work as expected) - Refactor (change restructuring the codebase without changing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - I followed the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
Base automatically changed from
refactor/argilla-server/remove-sort-query-param
to
refactor/cleaning-list-records-endpoints
July 8, 2024 16:25
frascuchon
changed the title
argilla-server: List records endpoint using dbargilla-server: List records endpoint using db
for more information, see https://pre-commit.ci
jfcalvo
requested changes
Aug 30, 2024
argilla-server/src/argilla_server/api/handlers/v1/datasets/records.py
Outdated
Show resolved
Hide resolved
argilla-server/src/argilla_server/api/handlers/v1/datasets/records.py
Outdated
Show resolved
Hide resolved
… of github.com:argilla-io/argilla into refactor/argilla-server/list-records-endpoint-using-db
jfcalvo
requested changes
Sep 9, 2024
for more information, see https://pre-commit.ci
jfcalvo
approved these changes
Sep 23, 2024
frascuchon
deleted the
refactor/argilla-server/list-records-endpoint-using-db
branch
frascuchon
added a commit
that referenced
this pull request
Sep 24, 2024
# Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> The error resolved by this PR arises after merging the PR #5170. `VectorSettings` are not loaded properly when loading record vectors. This error was not found when running tests due to the mocked db session used for tests. **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Bug fix (non-breaking change which fixes an issue) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - I followed the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR rewrites the current list dataset records endpoint to use the DB instead of the search engine since no filtering is applied to the endpoint.
The PR introduces a new abstraction layer to manage DB internals: repository. With this layer, we have all db methods related to a resource in a single place, which helps to maintainability and reusability.DB Query details have been moved to the Record model class, simplifying the context flows.
Type of change
How Has This Been Tested
Checklist