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: update update_response endpoint to replace values #3711

Merged
merged 7 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ These are the section headers that we use:

- Move `database` commands under `server` group of commands ([#3710](https://github.com/argilla-io/argilla/pull/3710))
- `server` commands only included in the CLI app when `server` extra requirements are installed ([#3710](https://github.com/argilla-io/argilla/pull/3710)).
- Updated `PUT /api/v1/responses/{response_id}` to replace `values` stored with received `values` in request ([#3711](https://github.com/argilla-io/argilla/pull/3711)).

### Fixed

Expand Down
6 changes: 5 additions & 1 deletion src/argilla/server/contexts/datasets.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,11 @@ async def update_response(

async with db.begin_nested():
response = await response.update(
db, values=jsonable_encoder(response_update.values), status=response_update.status, autocommit=False
db,
values=jsonable_encoder(response_update.values),
status=response_update.status,
replace_dict=True,
autocommit=False,
)
await search_engine.update_record_response(response)

Expand Down
13 changes: 9 additions & 4 deletions src/argilla/server/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ def _schema_or_kwargs(schema: Union[Schema, None], values: Dict[str, Any]) -> Di
class CRUDMixin:
__upsertable_columns__: Union[Set[str], None] = None

def fill(self, **kwargs: Any) -> Self:
def fill(self, replace_dict: bool = False, **kwargs: Any) -> Self:
for key, value in kwargs.items():
if not hasattr(self, key):
raise AttributeError(f"Model `{self.__class__.__name__}` has no attribute `{key}`")
# If the value is a dict, set value for each key one by one, as we want to update only the keys that are in
# `value` and not override the whole dict.
if isinstance(value, dict):
if isinstance(value, dict) and not replace_dict:
dict_col = getattr(self, key) or {}
dict_col.update(value)
value = dict_col
Expand Down Expand Up @@ -81,10 +81,15 @@ async def read_by(cls, db: "AsyncSession", **params: Any) -> Union[Self, None]:
return result.scalars().unique().one_or_none()

async def update(
self, db: "AsyncSession", schema: Union[Schema, None] = None, autocommit: bool = True, **kwargs: Any
self,
db: "AsyncSession",
schema: Union[Schema, None] = None,
replace_dict: bool = False,
autocommit: bool = True,
**kwargs: Any,
) -> Self:
_values = _schema_or_kwargs(schema, kwargs)
updated = self.fill(**_values)
updated = self.fill(replace_dict=replace_dict, **_values)
return await updated.save(db, autocommit)

@classmethod
Expand Down
28 changes: 15 additions & 13 deletions src/argilla/server/models/questions.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,25 +118,27 @@ def check_response(self, response: ResponseValue, status: Optional[ResponseStatu
if not isinstance(response.value, list):
raise ValueError(f"This Ranking question expects a list of values, found {type(response.value)}")

# Only if the response is submitted check that all the possible options have been ranked
if status == ResponseStatus.submitted and len(response.value) != len(self.option_values):
raise ValueError(
f"This Ranking question expects a list containing {len(self.option_values)} values, found a list of"
f" {len(response.value)} values"
)

values = []
ranks = []
for response_option in response.value:
values.append(response_option.get("value"))
ranks.append(response_option.get("rank"))

invalid_ranks = _are_all_elements_in_list(ranks, self.rank_values)
if invalid_ranks:
raise ValueError(
f"{invalid_ranks!r} are not valid ranks for this Ranking question.\nValid ranks are:"
f" {self.rank_values!r}"
)
# Only if the response is submitted check that all the possible options have been ranked or that all the
# provided options contains a valid rank
if status == ResponseStatus.submitted:
if len(response.value) != len(self.option_values):
raise ValueError(
f"This Ranking question expects a list containing {len(self.option_values)} values, found a list of"
f" {len(response.value)} values"
)

invalid_ranks = _are_all_elements_in_list(ranks, self.rank_values)
if invalid_ranks:
raise ValueError(
f"{invalid_ranks!r} are not valid ranks for this Ranking question.\nValid ranks are:"
f" {self.rank_values!r}"
)

invalid_values = _are_all_elements_in_list(values, self.option_values)
if invalid_values:
Expand Down
47 changes: 35 additions & 12 deletions tests/unit/server/api/v1/test_responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,33 @@

@pytest.mark.asyncio
class TestSuiteResponses:
@pytest.mark.parametrize(
"response_json",
[
{
"values": {"input_ok": {"value": "yes"}, "output_ok": {"value": "yes"}},
"status": "submitted",
},
{
"values": {"input_ok": {"value": "yes"}},
"status": "submitted",
},
{
"values": {"output_ok": {"value": "yes"}},
"status": "draft",
},
],
)
async def test_update_response(
self, async_client: "AsyncClient", db: "AsyncSession", mock_search_engine: SearchEngine, owner_auth_header: dict
self,
async_client: "AsyncClient",
db: "AsyncSession",
mock_search_engine: SearchEngine,
owner_auth_header: dict,
response_json: dict,
):
dataset = await DatasetFactory.create(status=DatasetStatus.ready)
await TextQuestionFactory.create(name="input_ok", dataset=dataset)
await TextQuestionFactory.create(name="input_ok", dataset=dataset, required=True)
await TextQuestionFactory.create(name="output_ok", dataset=dataset)
record = await RecordFactory.create(dataset=dataset)

Expand All @@ -61,24 +83,17 @@ async def test_update_response(
values={"input_ok": {"value": "no"}, "output_ok": {"value": "no"}},
status=ResponseStatus.submitted,
)
response_json = {
"values": {"input_ok": {"value": "yes"}, "output_ok": {"value": "yes"}},
"status": "submitted",
}

resp = await async_client.put(f"/api/v1/responses/{response.id}", headers=owner_auth_header, json=response_json)

assert resp.status_code == 200
assert (await db.get(Response, response.id)).values == {
"input_ok": {"value": "yes"},
"output_ok": {"value": "yes"},
}
assert (await db.get(Response, response.id)).values == response_json["values"]

resp_body = resp.json()
assert resp_body == {
"id": str(response.id),
"values": {"input_ok": {"value": "yes"}, "output_ok": {"value": "yes"}},
"status": "submitted",
"values": response_json["values"],
"status": response_json["status"],
"record_id": str(response.record_id),
"user_id": str(response.user_id),
"inserted_at": response.inserted_at.isoformat(),
Expand Down Expand Up @@ -175,6 +190,14 @@ async def test_update_response_to_submitted_status(
{"value": "completion-a", "rank": 1},
],
),
(
RankingQuestionFactory,
[
{"value": "completion-a", "rank": 1},
{"value": "completion-b"},
{"value": "completion-c"},
],
),
],
)
@pytest.mark.parametrize("response_status", [ResponseStatus.draft, ResponseStatus.discarded])
Expand Down
22 changes: 17 additions & 5 deletions tests/unit/server/models/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
# limitations under the License.

import asyncio
from copy import copy
from typing import TYPE_CHECKING, Optional
from typing import TYPE_CHECKING, Any, Dict, Optional

import pytest
import pytest_asyncio
from argilla.server.models.base import DatabaseModel
from pydantic import BaseModel
from sqlalchemy import inspect, select
from sqlalchemy import JSON, inspect, select
from sqlalchemy.ext.mutable import MutableDict
from sqlalchemy.orm import Mapped, mapped_column

if TYPE_CHECKING:
Expand All @@ -34,6 +34,7 @@ class Model(DatabaseModel):
str_col: Mapped[Optional[str]] = mapped_column()
int_col: Mapped[Optional[int]] = mapped_column()
external_id: Mapped[Optional[str]] = mapped_column(unique=True)
dict_col: Mapped[Optional[Dict[str, Any]]] = mapped_column(MutableDict.as_mutable(JSON))

__upsertable_columns__ = {"str_col", "int_col"}

Expand Down Expand Up @@ -85,10 +86,21 @@ async def test_database_model_read(self, db: "AsyncSession"):
assert model.int_col == 1

async def test_database_model_update(self, db: "AsyncSession"):
model = await Model.create(db, str_col="unit-test", int_col=1, autocommit=True)
model = await model.update(db, str_col="unit-test-2", int_col=2, autocommit=True)
model = await Model.create(
db, str_col="unit-test", int_col=1, dict_col={"a": 1, "b": 2, "c": 3}, autocommit=True
)

model = await model.update(db, str_col="unit-test-2", int_col=2, dict_col={"a": 10}, autocommit=True)
assert model.str_col == "unit-test-2"
assert model.int_col == 2
assert model.dict_col == {"a": 10, "b": 2, "c": 3}

model = await model.update(
db, str_col="unit-test-2", int_col=2, dict_col={"a": 10}, replace_dict=True, autocommit=True
)
assert model.str_col == "unit-test-2"
assert model.int_col == 2
assert model.dict_col == {"a": 10}

async def test_database_model_upsert_many(self, db: "AsyncSession"):
models = []
Expand Down
Loading