Skip to content

Comments

78: Add parameter relation config to simulator workflow in workflow_c…#83

Merged
lfse-slafleur merged 6 commits intomainfrom
78-add-variable-conditional-relations-in-workflow-definition-including-smaller-and-greater-than
Apr 15, 2025
Merged

78: Add parameter relation config to simulator workflow in workflow_c…#83
lfse-slafleur merged 6 commits intomainfrom
78-add-variable-conditional-relations-in-workflow-definition-including-smaller-and-greater-than

Conversation

@lfse-slafleur
Copy link
Member

…onfig.json.

TODO: Still need to update SDK once released

…kflow-definition-including-smaller-and-greater-than
"key_name": "end_time"
}
],
"parameter_relations": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would parameter_constraints be a suitable name? (I was thinking of how such a case would be handled in the world of SQL.)

Also wondering if instead of declaring parameter_relations at this json level (which is completely fine), would it be more intuitive for users to define it at the parameter level. For instance:

{
      "parameter_type": "datetime",
      "key_name": "start_time",
      "constraints: [
           {
              "relation": "smaller",
              "key_2": "end_time"
            }
        ]
      "
},

Although not sure if it would make it more complicated in SDK code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, both with the 'constraints' naming and placing inside a parameter description. I would name key_2 as other_parameter.

It would be more intuitive and you can control which of the two parameters is constraint, and therefore gets the warning. For instance, if end_time is not larger than start_time I only want a warning on the end_time parameter for the mapeditor case. In some other situation you would want both parameters to get a warning.

"key_name": "end_time"
}
],
"parameter_relations": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, both with the 'constraints' naming and placing inside a parameter description. I would name key_2 as other_parameter.

It would be more intuitive and you can control which of the two parameters is constraint, and therefore gets the warning. For instance, if end_time is not larger than start_time I only want a warning on the end_time parameter for the mapeditor case. In some other situation you would want both parameters to get a warning.

@lfse-slafleur lfse-slafleur marked this pull request as ready for review April 14, 2025 15:11
@@ -1,5 +1,5 @@
#
# This file is autogenerated by pip-compile with Python 3.11
# This file is autogenerated by pip-compile with Python 3.10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Dockerfile we use 3.11, should this also be used to compile the requirements?

@@ -1,5 +1,5 @@
#
# This file is autogenerated by pip-compile with Python 3.11
# This file is autogenerated by pip-compile with Python 3.10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use python 3.11?

@lfse-slafleur lfse-slafleur merged commit 7edca96 into main Apr 15, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add variable conditional relations in workflow definition including smaller and greater than

3 participants