[DEP] Bump Pydantic to V2.6 #116

Mildophin · 2024-04-11T14:31:53Z

Description

This PR updates the version of Pydantic required by pasqal-cloud from V1.10 to V2.6.

Breaking changes

Now pasqal-cloud needs Pydantic >= 2.6.0 to run.

Checklist

The title of the PR follows the right format: [{Label}] {Short Message}. Label examples: IMPROVEMENT, FIX, REFACTORING... Short message is about what your PR changes.

Versioning (PASQAL developers only)

Update the version of pasqal-cloud in _version.py following the changes in your PR and by using semantic versioning.

Documentation

Update CHANGELOG.md with a description explaining briefly the changes to the users.

Tests

Unit tests have been added or adjusted.
Tests were run locally.

Internal tests pipeline (PASQAL developers only)

Update and run the internal tests while targeting the branch of this PR.
If your PR hasn't changed any functionality, it still needs to be validated against internal tests.

After updating the version (PASQAL developers only)

Open a PR on the internal tests that updates the version used for the pasqal-cloud backward compatibility tests.

- fix optional fields - fix batch private field (_client): add __init__

- refacto of ordered_jobs attribute creation - add a minimal version for pydantic - apply pydantic conventions - refacto of how to set _client attribute

pasqal_cloud/batch.py

Mildophin · 2024-04-11T14:50:49Z

pasqal_cloud/batch.py

-    @validator("configuration", pre=True)
+    @jobs.setter
+    def jobs(self, _: Any) -> None:
+        # Does not set 'jobs' as it is built from 'ordered_jobs'


I'm not totally convinced by my explanation, if you have a better manner of explaining it, I am all ears.

What are you trying to explain?

I am trying to explain we can't set jobs as it depending of ordered_jobs, it is just that ordered jobs is a list of dict, and jobs a dict with a different key for each job, made from ordered list. Anyway I might change the logic as it is very confusing, I remember I had to do this trick with the property before Pydantic V2 but things can be easier now. I will have a look.

Quick suggestion, feel free to iterate over it

Suggested change

# Does not set 'jobs' as it is built from 'ordered_jobs'

# Override the jobs setter to be a no-op.

# `jobs` is a read-only attribute which is derived from the `ordered_jobs` key.

setup.py

oliver-gordon · 2024-04-12T11:55:09Z

pasqal_cloud/workload.py

    def result_link_to_result(
-        cls, result: Optional[Dict[str, Any]], values: Dict[str, Any]
+        cls, v: Optional[Dict[str, Any]], info: ValidationInfo


Why the single letter variable changes?

To match pydantic way of using their feature: https://docs.pydantic.dev/latest/concepts/validators/

As I mentioned above I don't think it's advantageous to use v rather than being explicit with result

oliver-gordon · 2024-04-12T11:57:55Z

pasqal_cloud/batch.py

-    @validator("configuration", pre=True)
+    @jobs.setter
+    def jobs(self, _: Any) -> None:
+        # Does not set 'jobs' as it is built from 'ordered_jobs'


What are you trying to explain?

oliver-gordon · 2024-04-12T11:58:03Z

pasqal_cloud/batch.py

-        configuration: Union[Dict[str, Any], BaseConfig, None],
-        values: Dict[str, Any],
+        v: Union[Dict[str, Any], BaseConfig, None],
+        info: ValidationInfo,


Why the variable name changes?

Variable name changed for the same reason as I explained on your first comment, to match Pydantic documentation.

It's ok in the documentation to keep things generic but I think for our purpose here it's more readable and maintainable to keep configuration and not v.

oliver-gordon · 2024-04-12T11:59:52Z

pasqal_cloud/batch.py

-        values["ordered_jobs"] = ordered_jobs_list
-        return values
+        jobs_received = data.get("jobs", [])
+        data["ordered_jobs"] = [{**job, "_client": data["_client"]} for job in jobs_received]


If you really want to reduce this you can replace jobs_received with data.get("jobs", []).

You aren't saving much in the way of readability

I feel like it will be too much in this case if I replace jobs_received, too many information on the same line. I think this format is clear and compact enough. Anyway I think this is more of a question of preference. If you think it is not relevant I can change it.

The 2nd line at a glance is obscure so not sure readability or clarity means anything there. I'm just pointing out you can reduce it further if we want, you do what you prefer

Augustinio

LGTM aside from the loss of explicitness in the method parameters.

Augustinio · 2024-04-15T13:26:35Z

pasqal_cloud/batch.py

-        configuration: Union[Dict[str, Any], BaseConfig, None],
-        values: Dict[str, Any],
+        v: Union[Dict[str, Any], BaseConfig, None],
+        info: ValidationInfo,


It's ok in the documentation to keep things generic but I think for our purpose here it's more readable and maintainable to keep configuration and not v.

Augustinio · 2024-04-15T13:28:07Z

pasqal_cloud/workload.py

    def result_link_to_result(
-        cls, result: Optional[Dict[str, Any]], values: Dict[str, Any]
+        cls, v: Optional[Dict[str, Any]], info: ValidationInfo


As I mentioned above I don't think it's advantageous to use v rather than being explicit with result

MatthieuMoreau0

Looks good overall, a couple small remarks

pasqal_cloud/batch.py

MatthieuMoreau0 · 2024-04-15T16:50:58Z

pasqal_cloud/batch.py

-    @validator("configuration", pre=True)
+    @jobs.setter
+    def jobs(self, _: Any) -> None:
+        # Does not set 'jobs' as it is built from 'ordered_jobs'


Quick suggestion, feel free to iterate over it

Suggested change

# Does not set 'jobs' as it is built from 'ordered_jobs'

# Override the jobs setter to be a no-op.

# `jobs` is a read-only attribute which is derived from the `ordered_jobs` key.

pasqal_cloud/job.py

Mildophin and others added 8 commits April 9, 2024 16:33

Update of setup requirements + Batch model validators

d25e42d

- field_validator: use ValidationInfo

bbabbc7

- fix optional fields - fix batch private field (_client): add __init__

Update of the workload and job model

ad305e7

Update of all models + typos + formatting

9cd72bf

Pydantic config implementation changed

0109fc6

update of mypy version + correct hinting

5e0cfb6

- doc update

f5c128a

- refacto of ordered_jobs attribute creation - add a minimal version for pydantic - apply pydantic conventions - refacto of how to set _client attribute

change version and update changelog

a5aaa0b

Mildophin self-assigned this Apr 11, 2024

fix extra parameter

1e6f5fd

Mildophin commented Apr 11, 2024

View reviewed changes

pasqal_cloud/batch.py Show resolved Hide resolved

Mildophin added 3 commits April 11, 2024 16:40

update doc

2073a29

fix hinting missing type and return type

bf71a08

edit month on changelog + doc update

613c9ba

Mildophin commented Apr 11, 2024

View reviewed changes

Mildophin force-pushed the mv/update-to-pydantic-V2 branch from e6da099 to 68eb594 Compare April 11, 2024 14:54

Mildophin commented Apr 11, 2024

View reviewed changes

setup.py Show resolved Hide resolved

Mildophin added 3 commits April 11, 2024 17:11

edit pydantic version required

41df4dd

make changelog explanation clearer

583c176

update required version of pydantic V2

53546a3

Mildophin force-pushed the mv/update-to-pydantic-V2 branch from c806a0e to 53546a3 Compare April 11, 2024 15:22

Mildophin changed the title ~~[DEP] Bump Pydantic to V2~~ [DEP] Bump Pydantic to V2.6 Apr 11, 2024

Mildophin marked this pull request as ready for review April 11, 2024 15:26

Mildophin requested review from MatthieuMoreau0, MohamedAminMallek, oliver-gordon and Augustinio April 11, 2024 15:26

Mildophin linked an issue Apr 11, 2024 that may be closed by this pull request

Pasqal-Cloud dependency with Pydantic (Forschungszentrum Jülich) #114

Closed

oliver-gordon reviewed Apr 12, 2024

View reviewed changes

Augustinio requested changes Apr 15, 2024

View reviewed changes

MatthieuMoreau0 approved these changes Apr 15, 2024

View reviewed changes

MohamedAminMallek approved these changes Apr 16, 2024

View reviewed changes

Mildophin added 2 commits April 16, 2024 11:03

sorting imports

4dad41e

update doc

1698073

Mildophin requested review from MatthieuMoreau0 and Augustinio April 16, 2024 12:45

Augustinio approved these changes Apr 16, 2024

View reviewed changes

MatthieuMoreau0 approved these changes Apr 16, 2024

View reviewed changes

Mildophin merged commit 9d7bceb into dev Apr 16, 2024
5 checks passed

Mildophin deleted the mv/update-to-pydantic-V2 branch April 16, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEP] Bump Pydantic to V2.6 #116

[DEP] Bump Pydantic to V2.6 #116

Mildophin commented Apr 11, 2024 •

edited

Loading

Mildophin Apr 11, 2024

oliver-gordon Apr 12, 2024

Mildophin Apr 12, 2024

MatthieuMoreau0 Apr 15, 2024

oliver-gordon Apr 12, 2024

Mildophin Apr 12, 2024

Augustinio Apr 15, 2024 •

edited

Loading

oliver-gordon Apr 12, 2024

oliver-gordon Apr 12, 2024

Mildophin Apr 12, 2024 •

edited

Loading

Augustinio Apr 15, 2024

oliver-gordon Apr 12, 2024

Mildophin Apr 12, 2024

oliver-gordon Apr 16, 2024

Augustinio left a comment

Augustinio Apr 15, 2024

Augustinio Apr 15, 2024 •

edited

Loading

MatthieuMoreau0 left a comment

MatthieuMoreau0 Apr 15, 2024

	# Does not set 'jobs' as it is built from 'ordered_jobs'
	# Override the jobs setter to be a no-op.
	# `jobs` is a read-only attribute which is derived from the `ordered_jobs` key.

[DEP] Bump Pydantic to V2.6 #116

[DEP] Bump Pydantic to V2.6 #116

Conversation

Mildophin commented Apr 11, 2024 • edited Loading

Description

Breaking changes

Checklist

Versioning (PASQAL developers only)

Documentation

Tests

Internal tests pipeline (PASQAL developers only)

After updating the version (PASQAL developers only)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustinio Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mildophin Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustinio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Augustinio Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

MatthieuMoreau0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mildophin commented Apr 11, 2024 •

edited

Loading

Augustinio Apr 15, 2024 •

edited

Loading

Mildophin Apr 12, 2024 •

edited

Loading

Augustinio Apr 15, 2024 •

edited

Loading