[REFACTOR] `argilla server`: using pydantic v2 #5666

frascuchon · 2024-11-04T13:26:52Z

Description

An alternative work of pydantic v2 upgrade with the latest changes from develop.

The main problems I found:

The Optional values are now required. This will affect how schemas are returned since default values are sets, including them as part of the schema result (record without responses or suggestions, for example)
I found some serialization errors when the response body (an error) contains a ValueError object. The same is working with pydantic <2
Automatic type conversion is not working anymore. This means that many client requests may fail ( values are not automatically converted to str, or some complex structures (record fields may be affected by this). We can still define custom model_validators, but we must apply them carefully.

Closes #4935

Refs #5508

Type of change

Refactor (change restructuring the codebase without changing functionality)

How Has This Been Tested

Checklist

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/)

codecov · 2024-11-05T11:47:24Z

Codecov Report

Attention: Patch coverage is 98.72611% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.35%. Comparing base (91ed8ca) to head (612c721).

Files with missing lines	Patch %	Lines
...-server/src/argilla_server/errors/error_handler.py	93.33%	1 Missing ⚠️
argilla-server/src/argilla_server/jobs/hub_jobs.py	0.00%	1 Missing ⚠️
argilla-server/src/argilla_server/models/mixins.py	66.66%	1 Missing ⚠️
argilla-server/src/argilla_server/utils.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5666      +/-   ##
===========================================
+ Coverage    91.10%   91.35%   +0.24%     
===========================================
  Files          150      146       -4     
  Lines         6253     6292      +39     
===========================================
+ Hits          5697     5748      +51     
+ Misses         556      544      -12

Flag	Coverage Δ
argilla-server	`91.35% <98.72%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Description  Closes #5633 **Type of change**  - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) **How Has This Been Tested**  **Checklist**  - 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: José Francisco Calvo <jose@argilla.io> Co-authored-by: José Francisco Calvo <josefranciscocalvo@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

jfcalvo

Masive effort! 💪

jfcalvo · 2024-11-18T16:16:53Z

argilla-server/.env.dev

@@ -1,2 +1,2 @@
 ALEMBIC_CONFIG=src/argilla_server/alembic.ini
-ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla.db?check_same_thread=False
+ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla.dev.db?check_same_thread=False


For .env.test we have the following:

ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla-test.db?check_same_thread=False

So I guess we should do the same here:

Suggested change

ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla.dev.db?check_same_thread=False

ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla-dev.db?check_same_thread=False

jfcalvo · 2024-11-18T16:18:11Z

argilla-server/pyproject.toml

@@ -22,7 +22,8 @@ maintainers = [{ name = "argilla", email = "contact@argilla.io" }]
 dependencies = [
    # Basic dependencies
    "fastapi ~= 0.115.0",
-    "pydantic ~= 1.10.18",
+    "pydantic ~= 2.9.0",


Can we upgrade to the latest version?

Suggested change

"pydantic ~= 2.9.0",

"pydantic ~= 2.9.2",

Don't forget to do pdm install after change this, so the lock file is updated.

jfcalvo · 2024-11-18T16:19:15Z

argilla-server/pyproject.toml

@@ -22,7 +22,8 @@ maintainers = [{ name = "argilla", email = "contact@argilla.io" }]
 dependencies = [
    # Basic dependencies
    "fastapi ~= 0.115.0",
-    "pydantic ~= 1.10.18",
+    "pydantic ~= 2.9.0",
+    "pydantic-settings ~= 2.6.0",


Something similar for pydantic-settings:

Suggested change

"pydantic-settings ~= 2.6.0",

"pydantic-settings ~= 2.6.1",

Don't forget to do pdm install after change this, so the lock file is updated.

jfcalvo · 2024-11-18T16:30:41Z

argilla-server/src/argilla_server/api/schemas/v1/chat.py

-    role: str = Field(..., min_role_length=MIN_ROLE_LENGTH, max_length=MAX_ROLE_LENGTH, regex=MAX_ROLE_REGEX)
-    content: str = Field(..., min_message_length=MIN_MESSAGE_LENGTH, max_length=MAX_MESSAGE_LENGTH)


This means we didn't have a test checking this right (for empty strings)? Should we add one?

jfcalvo · 2024-11-18T17:10:25Z

argilla-server/src/argilla_server/api/schemas/v1/suggestions.py

-        le=SCORE_LESS_THAN_OR_EQUAL,
-        description="The score assigned to the suggestion",
-    )
+    score: Optional[Union[float, List[float]]] = Field(None, description="The score assigned to the suggestion")


Instead of adding custom validations maybe we can define custom types like SuggestionScore.

Take a look to what I did here: https://github.com/argilla-io/argilla/pull/5508/files#diff-a9277ada64762a8062c1e132567edba6ce2aec3e279d1ded0452b9557bfd6b26

jfcalvo · 2024-11-18T17:14:09Z

argilla-server/src/argilla_server/api/schemas/v1/users.py

    password: str = Field(min_length=USER_PASSWORD_MIN_LENGTH, max_length=USER_PASSWORD_MAX_LENGTH)
+    first_name: constr(min_length=1, strip_whitespace=True)
+    last_name: Optional[constr(min_length=1, strip_whitespace=True)] = None


Maybe we should start using annotated str with StringConstraints instead of using the old constr.

This applies to all the constr used in the PR.

So something like:

last_name: Optional[Annotated[str, StringConstraints(min_length=1, strip_whitespace=True)]] = None

You can take a look to https://github.com/argilla-io/argilla/pull/5508/files#diff-0e72eee9f8cea03b100e007fe81d5a97c5ebea93955187f083bb908a4cee2e29R48

jfcalvo · 2024-11-18T17:16:28Z

argilla-server/src/argilla_server/api/schemas/v1/workspaces.py

@@ -16,7 +16,8 @@
 from typing import List
 from uuid import UUID

-from argilla_server.pydantic_v1 import BaseModel, Field
+from pydantic import BaseModel, Field, ConfigDict
+from starlette.config import Config


Why is this import needed?

frascuchon requested a review from jfcalvo November 4, 2024 13:26

frascuchon added 10 commits November 5, 2024 12:49

config: Update to pydantic v2

50b0e82

refactor: Update API schemas

2f53019

refactor: Import pydantic module

1a8539d

chore: Remove unused files

34d5e30

refactor: Convert to settingschema

798db75

chore: Remove v2 logic for APP

18e721e

refactor: Review and adapt error handler

43330fe

tests: Adapt tests messages

a3050fa

fix: Add coerce 2 string for terms filter

fb16594

chore: Redefine TextQuery as BaseModel with coerce

ff30ee1

frascuchon force-pushed the refactor/argilla-server/using-pydanticV2 branch from f14425f to ff30ee1 Compare November 5, 2024 12:17

frascuchon and others added 4 commits November 5, 2024 13:23

refactor: review validator code

79cfc93

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

6aec507

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

d305ae4

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

145ea38

frascuchon changed the title ~~[DRAFT] [REFACTOR] argilla server: using pydantic v2~~ [REFACTOR] argilla server: using pydantic v2 Nov 6, 2024

frascuchon and others added 2 commits November 6, 2024 16:49

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

489c265

frascuchon force-pushed the develop branch from f346a84 to 229809e Compare November 6, 2024 16:16

frascuchon and others added 6 commits November 8, 2024 09:53

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

06a666e

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

9d758ab

refactor: Migrate validatorsand deprecated methods/functions

2c0ae2e

chore: Same with tests

3d5f745

update pdm.lock

72975ff

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

eb4234c

frascuchon added this to the v2.5.0 milestone Nov 14, 2024

Merge branch 'develop' into refactor/argilla-server/using-pydanticV2

612c721

jfcalvo requested changes Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] `argilla server`: using pydantic v2 #5666

[REFACTOR] `argilla server`: using pydantic v2 #5666

frascuchon commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

jfcalvo left a comment

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

jfcalvo Nov 18, 2024

	ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla.dev.db?check_same_thread=False
	ARGILLA_DATABASE_URL=sqlite+aiosqlite:///${HOME}/.argilla/argilla-dev.db?check_same_thread=False

		role: str = Field(..., min_role_length=MIN_ROLE_LENGTH, max_length=MAX_ROLE_LENGTH, regex=MAX_ROLE_REGEX)
		content: str = Field(..., min_message_length=MIN_MESSAGE_LENGTH, max_length=MAX_MESSAGE_LENGTH)

[REFACTOR] argilla server: using pydantic v2 #5666

Are you sure you want to change the base?

[REFACTOR] argilla server: using pydantic v2 #5666

Conversation

frascuchon commented Nov 4, 2024 • edited Loading

Description

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

jfcalvo left a comment

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

jfcalvo Nov 18, 2024

Choose a reason for hiding this comment

[REFACTOR] `argilla server`: using pydantic v2 #5666

[REFACTOR] `argilla server`: using pydantic v2 #5666

frascuchon commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading