From d12c00acb6b0f30dea93b4d42d4354fa3ac996af Mon Sep 17 00:00:00 2001 From: belthlemar Date: Mon, 4 Mar 2024 19:22:38 +0100 Subject: [PATCH 1/4] fix(variants): don't raise Recursive error when creating big variant tree --- antarest/study/storage/variantstudy/model/model.py | 8 +++----- .../variant_blueprint/test_variant_manager.py | 13 +++++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/antarest/study/storage/variantstudy/model/model.py b/antarest/study/storage/variantstudy/model/model.py index 1e51032ce4..3d881f6429 100644 --- a/antarest/study/storage/variantstudy/model/model.py +++ b/antarest/study/storage/variantstudy/model/model.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Tuple, Union +from typing import List, NamedTuple, Optional, Tuple, Union from pydantic import BaseModel @@ -26,9 +26,7 @@ class CommandResultDTO(BaseModel): message: str -class VariantTreeDTO(BaseModel): +class VariantTreeDTO(NamedTuple): + # Don't use BaseModel as this could trigger Recursion exceptions, since we're using Pydantic with a version prior to v2. node: StudyMetadataDTO children: List["VariantTreeDTO"] - - -VariantTreeDTO.update_forward_refs() diff --git a/tests/integration/variant_blueprint/test_variant_manager.py b/tests/integration/variant_blueprint/test_variant_manager.py index df3cf590e4..8a300e75da 100644 --- a/tests/integration/variant_blueprint/test_variant_manager.py +++ b/tests/integration/variant_blueprint/test_variant_manager.py @@ -238,3 +238,16 @@ def test_comments(client: TestClient, admin_access_token: str, variant_id: str) # Asserts comments did not disappear res = client.get(f"/v1/studies/{variant_id}/comments", headers=admin_headers) assert res.json() == comment + + +def test_recursive_variant_tree(client: TestClient, admin_access_token: str): + admin_headers = {"Authorization": f"Bearer {admin_access_token}"} + base_study_res = client.post("/v1/studies?name=foo", headers=admin_headers) + base_study_id = base_study_res.json() + parent_id = base_study_res.json() + for k in range(150): + res = client.post(f"/v1/studies/{base_study_id}/variants?name=variant_{k}", headers=admin_headers) + base_study_id = res.json() + # Asserts that we do not trigger a Recursive Exception + res = client.get(f"/v1/studies/{parent_id}/variants", headers=admin_headers) + assert res.status_code == 200 From 4da1feffdfe6233efb348e9417c775669114428e Mon Sep 17 00:00:00 2001 From: belthlemar Date: Tue, 5 Mar 2024 10:04:29 +0100 Subject: [PATCH 2/4] fix(variants): do not use NamedTuple as it changes ResponseModel --- antarest/study/storage/variantstudy/model/model.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/antarest/study/storage/variantstudy/model/model.py b/antarest/study/storage/variantstudy/model/model.py index 3d881f6429..4b2e15e2c1 100644 --- a/antarest/study/storage/variantstudy/model/model.py +++ b/antarest/study/storage/variantstudy/model/model.py @@ -1,4 +1,4 @@ -from typing import List, NamedTuple, Optional, Tuple, Union +from typing import List, Optional, Tuple, Union from pydantic import BaseModel @@ -26,7 +26,8 @@ class CommandResultDTO(BaseModel): message: str -class VariantTreeDTO(NamedTuple): +class VariantTreeDTO: # Don't use BaseModel as this could trigger Recursion exceptions, since we're using Pydantic with a version prior to v2. - node: StudyMetadataDTO - children: List["VariantTreeDTO"] + def __init__(self, node: StudyMetadataDTO, children: List["VariantTreeDTO"]) -> None: + self.node = node + self.children = children From 0ec93b01c7e6c08e88f4cd9e5ea9f0617e2bec70 Mon Sep 17 00:00:00 2001 From: Laurent LAPORTE Date: Sat, 9 Mar 2024 16:18:13 +0100 Subject: [PATCH 3/4] fix(variants): improve documentation and typing --- .../storage/variantstudy/business/utils.py | 9 ++-- .../study/storage/variantstudy/model/model.py | 52 ++++++++++++++++--- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/antarest/study/storage/variantstudy/business/utils.py b/antarest/study/storage/variantstudy/business/utils.py index 6f04601ec5..933c72bed7 100644 --- a/antarest/study/storage/variantstudy/business/utils.py +++ b/antarest/study/storage/variantstudy/business/utils.py @@ -52,10 +52,13 @@ def get_or_create_section(json_ini: JSON, section: str) -> JSON: def remove_none_args(command_dto: CommandDTO) -> CommandDTO: - if isinstance(command_dto.args, list): - command_dto.args = [{k: v for k, v in args.items() if v is not None} for args in command_dto.args] + args = command_dto.args + if isinstance(args, list): + command_dto.args = [{k: v for k, v in args.items() if v is not None} for args in args] + elif isinstance(args, dict): + command_dto.args = {k: v for k, v in args.items() if v is not None} else: - command_dto.args = {k: v for k, v in command_dto.args.items() if v is not None} + raise TypeError(f"Invalid type for args: {type(args)}") return command_dto diff --git a/antarest/study/storage/variantstudy/model/model.py b/antarest/study/storage/variantstudy/model/model.py index 4b2e15e2c1..cd478742b4 100644 --- a/antarest/study/storage/variantstudy/model/model.py +++ b/antarest/study/storage/variantstudy/model/model.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Tuple, Union +import typing as t from pydantic import BaseModel @@ -7,19 +7,46 @@ class GenerationResultInfoDTO(BaseModel): + """ + Result information of a snapshot generation process. + + Attributes: + success: A boolean indicating whether the generation process was successful. + details: A list of tuples containing detailed information about the generation process. + """ + success: bool - details: List[Tuple[str, bool, str]] + details: t.MutableSequence[t.Tuple[str, bool, str]] class CommandDTO(BaseModel): - id: Optional[str] + """ + This class represents a command. + + Attributes: + id: The unique identifier of the command. + action: The action to be performed by the command. + args: The arguments for the command action. + version: The version of the command. + """ + + id: t.Optional[str] action: str - # if args is a list, this mean the command will be mapped to the list of args - args: Union[List[JSON], JSON] + args: t.Union[t.MutableSequence[JSON], JSON] version: int = 1 class CommandResultDTO(BaseModel): + """ + This class represents the result of a command. + + Attributes: + study_id: The unique identifier of the study. + id: The unique identifier of the command. + success: A boolean indicating whether the command was successful. + message: A message detailing the result of the command. + """ + study_id: str id: str success: bool @@ -27,7 +54,16 @@ class CommandResultDTO(BaseModel): class VariantTreeDTO: - # Don't use BaseModel as this could trigger Recursion exceptions, since we're using Pydantic with a version prior to v2. - def __init__(self, node: StudyMetadataDTO, children: List["VariantTreeDTO"]) -> None: + """ + This class represents a variant tree structure. + + Attributes: + node: The metadata of the study (ID, name, version, etc.). + children: A list of variant children. + """ + + def __init__(self, node: StudyMetadataDTO, children: t.MutableSequence["VariantTreeDTO"]) -> None: + # We are intentionally not using Pydantic’s `BaseModel` here to prevent potential + # `RecursionError` exceptions that can occur with Pydantic versions before v2. self.node = node - self.children = children + self.children = children or [] From 4c4de249539daeeab27821f79f6df531240eed97 Mon Sep 17 00:00:00 2001 From: Laurent LAPORTE Date: Sat, 9 Mar 2024 18:28:30 +0100 Subject: [PATCH 4/4] test: change `TestStudySynthesis` to avoid crashes --- tests/integration/studies_blueprint/test_synthesis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/studies_blueprint/test_synthesis.py b/tests/integration/studies_blueprint/test_synthesis.py index 9afd66be9b..059fba2aa7 100644 --- a/tests/integration/studies_blueprint/test_synthesis.py +++ b/tests/integration/studies_blueprint/test_synthesis.py @@ -58,7 +58,7 @@ def test_raw_study( ) assert res.status_code == 200, res.json() duration = time.time() - start - assert 0 <= duration <= 0.1, f"Duration is {duration} seconds" + assert 0 <= duration <= 0.3, f"Duration is {duration} seconds" def test_variant_study( self,