feat(binding-constraints): create UpdateConstraints command #2274

smailio · 2024-12-18T15:42:04Z

No description provided.

MartinBelthle

Once all of this will be handled, we could add small unit tests for this command alone

MartinBelthle · 2025-02-28T08:28:05Z

antarest/study/storage/variantstudy/model/command/create_binding_constraint.py

@@ -141,7 +141,7 @@ def get_binding_constraint_config_cls(study_version: StudyVersion) -> Type[Bindi
        return BindingConstraintPropertiesBase


-def create_binding_constraint_config(study_version: StudyVersion, **kwargs: Any) -> BindingConstraintProperties:
+def create_binding_constraint_props(study_version: StudyVersion, **kwargs: Any) -> BindingConstraintProperties:


Minor: Rename create_binding_constraint_properties

MartinBelthle · 2025-02-28T08:31:23Z

tests/integration/study_data_blueprint/test_binding_constraints.py

@@ -169,9 +169,8 @@ def test_update_multiple_binding_constraints(self, client: TestClient, user_acce
        res = client.get(f"/v1/studies/{study_id}/commands")
        assert res.status_code == 200
        json_result = res.json()
-        assert len(json_result) == 2


Change the comments in this test as they are no longer relevant with your change

MartinBelthle · 2025-02-28T08:32:22Z

antarest/study/business/binding_constraint_management.py

+        bcs_output = {}
+        for bc_id, bc_input in bcs_by_ids.items():
+            # check binding constraint id sent by user exist for this study
+            # Note that this check is both done here and when the commmand is applied as well


typo: command with 2 m :)

MartinBelthle · 2025-02-28T08:33:23Z

antarest/study/business/binding_constraint_management.py

-                # The user changed the operator, we have to rename matrices accordingly
-                existing_operator = BindingConstraintOperator(current_value["operator"])
-                update_matrices_names(file_study, bc_id, existing_operator, value.operator)
+            # convert payload sent by user (most likely from table mode) to a ConstraintOutput dict


you can remove the comment in-between parenthesis

MartinBelthle · 2025-02-28T08:37:37Z

antarest/study/business/binding_constraint_management.py

+            dict: The adapted binding constraint data in the output format.
+
+        """
+        bc_json_copy = copy.deepcopy(bc_json)


Not sure to understand why you need a copy here

MartinBelthle · 2025-02-28T08:43:54Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+
+    # Properties of the `UPDATE_BINDING_CONSTRAINT` command:
+    bc_props_by_id: t.Mapping[str, BindingConstraintProperties]
+    study_version: StudyVersion


Don't need this it's already inside the ICommand class

MartinBelthle · 2025-02-28T08:48:35Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+        removed_groups = old_groups - new_groups
+        remove_bc_from_scenario_builder(file_study, removed_groups)
+        file_study.tree.save(bcs_json, bcs_url)
+        return CommandOutput(status=True, message="UpdatedBindingVonstraints command created successfully.")


MartinBelthle · 2025-02-28T08:52:48Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+            action=self.command_name.value, args=json_command, version=self.version, study_version=self.study_version
+        )
+
+    def generate_replacement_matrices(


You can put this method private and static

MartinBelthle · 2025-02-28T08:56:09Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+        study_version: StudyVersion,
+        bc_props: BindingConstraintProperties,
+        operator: BindingConstraintOperator,
+    ) -> t.Iterator[t.Tuple[str, t.Union[t.List[t.List[MatrixData]], str]]]:


You can type this a t.Iterator[t.Tuple[str, list[list[MatrixData]]]] as the matrix is always a list

MartinBelthle · 2025-02-28T08:59:22Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                    bc_id, study_version, bc_props, bc_props.operator
+                ):
+                    # prepare matrix as a dict to save it in the tree
+                    matrix_url = target.split("/")


This seems really complicated, why do'nt you just do file_study.tree.save(data=next_matrix, url=matrix_url) ?
Also if it's that simple you could do it inside the generate_replacement_matrices method to lighten the _apply code which is already not that simple

…ad argument

…pply_config was overriding all bindings

MartinBelthle

Mostly minor remarks

MartinBelthle · 2025-03-05T12:17:32Z

antarest/study/business/binding_constraint_management.py

-        # More efficient way of doing things but using less readable commands.
-        commands = []
-
+        study_version = StudyVersion.parse(study.version)


Don't need to parse anymore as study is now a StudyInterface, you can just write study.version

MartinBelthle · 2025-03-05T12:19:58Z

antarest/study/business/binding_constraint_management.py

+        return bcs_output
+
+    def __convert_constraint_input_to_output(
+        self, bc_json: JSON, bc_input_as_dict: Dict[str, Any], study_version: StudyVersion


Minor: JSON = Dict[str, Any] it's a bit weird to use both typings insidethe same method. I would replace JSon by dict[str, Any]

MartinBelthle · 2025-03-05T12:20:51Z

tests/integration/study_data_blueprint/test_binding_constraints.py

@@ -144,9 +144,9 @@ def test_update_multiple_binding_constraints(self, client: TestClient, user_acce
        res = client.get(f"/v1/studies/{study_id}/commands")
        assert res.status_code == 200
        json_result = res.json()
-        assert len(json_result) == 10
+        assert len(json_result) == 1
        for cmd in json_result:


Minor: You can replace the for loop by a check of [0] as you know there's only one element

MartinBelthle · 2025-03-05T12:22:38Z

tests/variantstudy/model/command/test_update_binding_constraints.py

+
+
+@pytest.fixture
+def file_study():


The name of the fixture does not seem to represent what it achieves

MartinBelthle · 2025-03-05T12:23:56Z

tests/variantstudy/model/command/test_update_binding_constraints.py

+@pytest.fixture
+def binding_constraint_properties():
+    return {
+        "bc_1": ConstraintInput(group="new_group1", operator="greater", time_step="daily"),


Seems to me the objects here should be Properties instead of ConstraintInput

The api is called with ConstraintInput which inherit Properties anyway. It's better to test it with ConstraintInput as it's closer to the reality.

It's really minor but I disagree, the command expects Properties and not Input

ConstraintInput is a Properties

MartinBelthle · 2025-03-05T12:34:58Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+            return existing_constraint
+
+        study_data.bindings = list(map(update_binding_constraint_dto, study_data.bindings))
+        return CommandOutput(status=True, message="_apply_config success !"), {}


The message is not really important but here it's not clear.
You can see in other commands what we do usually

MartinBelthle · 2025-03-05T12:36:40Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                group = bc_props_as_dict.get("group") or existing_constraint.group
+                operator = bc_props_as_dict.get("operator") or existing_constraint.operator
+                time_step = bc_props_as_dict.get("time_step") or existing_constraint.time_step
+                areas = bc_props_as_dict.get("areas") or existing_constraint.areas


Areas and clusters can not be updated as we don't give terms here. You can remove this

MartinBelthle · 2025-03-05T12:39:49Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                )
+            # It's important to use exclude_unset=True. Otherwise we'd override
+            # existing values with the default bc_props values.
+            # Also important to use by_alias=True, so time_step is renamed to type


it's not just for time_step, it's for the filtering also. You can remove this line I think

MartinBelthle · 2025-03-05T12:42:27Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+    operator: BindingConstraintOperator,
+) -> t.Iterator[t.Tuple[str, t.List[t.List[MatrixData]]]]:
+    """
+    Yield one or two (when operator is "BOTH") matrices intialized with default values.


Minor: IMO a better wording would be Yield one (or two when operator is "BOTH") ...
Also, typo: initialized

MartinBelthle · 2025-03-05T12:43:24Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+            BindingConstraintFrequency.DAILY.value: default_bc_weekly_daily_86,
+            BindingConstraintFrequency.WEEKLY.value: default_bc_weekly_daily_86,
+        }[bc_props.time_step].tolist()
+        yield (target, matrix)


Minor: My IDE says you can just write yield target, matrix

smailio · 2025-02-18T14:17:34Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                            status=False,
+                            message=f"Path '{target}' does not exist.",
+                        )
+                    except AssertionError as e:


remove except asserion error

smailio · 2025-02-18T14:17:46Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                            status=False,
+                            message=f"Path '{target}' does not target a matrix.",
+                        )
+                    except Exception as e:


remove this as well

smailio · 2025-02-18T14:19:44Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                            target_matrix = target_matrix[element]
+                        target_matrix[matrix_url[-1]] = next_matrix
+                        file_study.tree.save(replace_matrix_data)
+                    except (KeyError, ChildNotFoundError) as e:


remove this

smailio · 2025-02-18T14:24:07Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+        study_file_target = "input/bindingconstraints/bindingconstraints"
+        url = study_file_target.split("/")
+        tree_node = file_study.tree.get_node(url)
+        if not isinstance(tree_node, IniFileNode):


remove this file

smailio · 2025-02-18T14:31:25Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+
+        excluded_fields = set(ICommand.model_fields)
+        json_command = self.model_dump(mode="json", exclude=excluded_fields, exclude_unset=True)
+        # for key in json_command:


remove bc_props_by_id key

smailio · 2025-02-18T14:35:28Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+    bc_props_by_id: t.Mapping[str, BindingConstraintProperties]
+
+    def _apply_config(self, study_data: FileStudyTreeConfig) -> t.Tuple[CommandOutput, t.Dict[str, t.Any]]:
+        # index = next(i for i, bc in enumerate(study_data.bindings) if bc.id == self.id)


use same code that update a single constraint. This is usefull to update objects in cache.

smailio · 2025-02-18T14:38:39Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+        # input_bc_props = create_binding_constraint_config(study_version, **bc_input_as_dict)
+
+        bc_props_by_id = {
+            key: create_binding_constraint_config(study_version, **value) for key, value in bc_by_id.items()


rename this method

smailio · 2025-03-07T08:33:05Z

tests/variantstudy/model/command/test_update_binding_constraints.py

+@pytest.fixture
+def binding_constraint_properties():
+    return {
+        "bc_1": ConstraintInput(group="new_group1", operator="greater", time_step="daily"),


The api is called with ConstraintInput which inherit Properties anyway. It's better to test it with ConstraintInput as it's closer to the reality.

MartinBelthle

Didn't read everything I assume you took my last comments into account. Only minor remarks / opinions, after that we're good I think

MartinBelthle · 2025-03-07T14:36:51Z

tests/variantstudy/model/command/test_update_binding_constraints.py

+@pytest.fixture
+def binding_constraint_properties():
+    return {
+        "bc_1": ConstraintInput(group="new_group1", operator="greater", time_step="daily"),


It's really minor but I disagree, the command expects Properties and not Input

MartinBelthle · 2025-03-07T14:41:14Z

antarest/study/storage/variantstudy/model/command/update_binding_constraints.py

+                )
+            return existing_constraint
+
+        existing_ids = {b.id for b in study_data.bindings}


I find this really hard to read but we'll rewrite this when we'll remove the _apply_config and it will be easier to write

I just create a set, to check my elements exists in this set it's a common way to do.

MartinBelthle · 2025-03-07T14:44:34Z

antarest/study/business/binding_constraint_management.py

-                    bc_id, study.version, value, output.operator, self._command_context
-                )
-                commands.extend(replace_matrix_commands)
+            # convert ConstraintInput to dict, UpdateBindingConstraints will expect a dict as input


The command expects Properties not a dict or a ConstraintInput object. IMO either you give the constraintInput as is and you remove the comment or you modify the comment to say you're giving Properties and you really give properties

Anis SMAIL added 5 commits December 5, 2024 10:39

feat(command): wip

97066d1

feat(command): wip

b0fb6f1

feat(commands): wip

fbbd196

feat(commands): wip, first green test

0319e93

feat(command): add unimplemented remove update binding constraints

8501514

pull-request-size bot added the size/L label Dec 18, 2024

feat(command): wip test

c6b5db9

smailio changed the title ~~Feature/2374 binding constraints command~~ feat(binding-constraints): create UpdateConstraint command Jan 30, 2025

Anis SMAIL added 5 commits January 31, 2025 10:41

Merge branch 'dev' into feature/2374-binding-constraints-command

4df8643

wip

4fc03b0

wip

1562fbe

wip

e396701

Merge branch 'dev' into feature/2374-binding-constraints-command

5a955ca

smailio changed the title ~~feat(binding-constraints): create UpdateConstraint command~~ feat(binding-constraints): create UpdateConstraints command Feb 17, 2025

Anis SMAIL added 4 commits February 18, 2025 15:52

wip

9759baa

wip

320ed79

Merge branch 'dev' into feature/2374-binding-constraints-command

3df1b17

fix liscence header

f26b3b7

pull-request-size bot added size/XL and removed size/L labels Feb 26, 2025

Anis SMAIL added 2 commits February 26, 2025 16:33

fix tests broken after merge

8650d0a

fix

569933a

pull-request-size bot added size/L and removed size/XL labels Feb 26, 2025

Anis SMAIL added 6 commits February 27, 2025 10:14

wip

5bab5b1

wip

9d262d4

fix test

8843fab

fix

9d13bfb

Merge branch 'dev' into feature/2374-binding-constraints-command

dd4f4d9

refactor

db67be5

reword refactor

e39bb8f

smailio marked this pull request as ready for review February 27, 2025 15:34

remove useless comment

1ac9835

smailio requested a review from a team February 27, 2025 16:01

MartinBelthle requested changes Feb 28, 2025

View reviewed changes

Anis SMAIL added 3 commits March 3, 2025 10:54

wip

ab7daaf

make update binding constraint accept both dict and constraint input …

029399e

…ad argument

fix update binding constraint, it used type instead of time_step + _a…

a612b22

…pply_config was overriding all bindings

pull-request-size bot added size/XL and removed size/L labels Mar 4, 2025

Anis SMAIL added 5 commits March 5, 2025 01:53

more tests

271f8ea

fix error when writing matrix

533eace

add liscence header to test file

2fdc367

merge

de15d8e

finally fix matrix writing

46c43f6

MartinBelthle requested changes Mar 5, 2025

View reviewed changes

Anis SMAIL added 3 commits March 6, 2025 10:41

fix PR

2ba2df4

fix more PR

d1491a5

fix output

aa4cf18

smailio commented Mar 7, 2025

View reviewed changes

MartinBelthle requested changes Mar 7, 2025

View reviewed changes

remove useless comment

ffb5bd3

feat(binding-constraints): create UpdateConstraints command #2274

Are you sure you want to change the base?

feat(binding-constraints): create UpdateConstraints command #2274

Conversation

smailio commented Dec 18, 2024

MartinBelthle left a 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

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

MartinBelthle left a comment

Choose a reason for hiding this comment

MartinBelthle Mar 5, 2025 • 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

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

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

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

MartinBelthle left a 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

MartinBelthle Mar 5, 2025 •

edited

Loading