Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Procedural Constraints Action/Database Support #1596

Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

skovati
Copy link
Contributor

@skovati skovati commented Nov 8, 2024

Description

Mirroring the database changes that were made for procedural scheduling support, this PR makes the following changes to the constraints sub-schema in merlin:

  • an auto-generated ID invocation_id was added to constraint_specification and made the new (sole) primary key. This allows for multiple "invocations" of that same constraint definition in the same spec.
  • type enum, uploaded_jar_id, and parameter schema fields were added to constraint_definition to support procedural constraints
  • definition was made nullable in constraint_definition, since it will be null when that constraint's source code is contained in the uploaded jar.
  • constraint_run now holds an additional field, constraint_invocation_id, which replaced (constraint_id, constraint_revision) in the primary key definition. This is so we can have a constraint run for each invocation in a plan spec, which might share constraint definitions and revisions.
  • constraint_run also holds the "as run" arguments that were passed to the constraint during execution in a new json field arguments.
  • merlin server was updated to comply with these schema changes
  • e2e tests were updated to comply as well

Verification

WIP

Documentation

Future work

@skovati
Copy link
Contributor Author

skovati commented Nov 8, 2024

Active works in progress:

  • database migrations
  • comments for new tables
  • verification of migrations against real data

@skovati skovati removed the request for review from Mythicaeda November 8, 2024 17:51
@@ -10,7 +12,7 @@ create table merlin.constraint_run (
requested_at timestamptz not null default now(),

constraint constraint_run_key
primary key (constraint_id, constraint_revision, simulation_dataset_id),
primary key (constraint_invocation_id, simulation_dataset_id),
Copy link
Contributor

@Mythicaeda Mythicaeda Jan 3, 2025

Choose a reason for hiding this comment

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

Blocking: This pkey is not sufficient. If the user decides to check constraints twice, updating the definition of a constraint between checks, they will get a pkey conflict when the action attempts to cache the output of the second definition.

(This comment is here to prevent me from not fixing this)

@Mythicaeda Mythicaeda force-pushed the feature/procedural-constraints-db-support branch from 141cec3 to 9b55bd7 Compare February 12, 2025 21:11
@Mythicaeda Mythicaeda force-pushed the feature/procedural-constraints-db-support branch from 9b55bd7 to 615767e Compare February 12, 2025 21:18
@Mythicaeda Mythicaeda force-pushed the feature/procedural-constraints-db-support branch from 615767e to 770a4c8 Compare February 19, 2025 23:58
@Mythicaeda Mythicaeda force-pushed the feature/procedural-constraints-db-support branch from 563a1d4 to 4e556a3 Compare February 24, 2025 19:36
- named `priority` in the DB to maintain schema parity with scheduling, and because `order` is a reserved keyword in SQL
- fixes #1633
- add a column to track errors that occur when a constraint is run
- replace 'constraint_run' with an association table between 'constraint_request' and 'constraint_results'
- update Hasura metadata
Code Pattern now matches the one used for Procedural Constraints, with the definition stored within a Type interface

Similarly, Constraint and ConstraintRecord have been merged, as they contain the exact same fields.
Include constraint invocation id in violations response
Now gets the type of the constraint, and the JAR path, in the case of procedural constraint.

Changes the ConstraintRecord creation code to use column names to get values rather than depending on the column order of the SQL statement.
Class now has two types: A SuccessType and a FailureType. This allows us to know what type the value is when working with an instance.

`getOrNull` has been replaced with `get`

Single Constructor has been split into a Success Constructor and a Failure Constructor
The code will throw if the Optional is not present, so the wrapping is unnecessary
…ources

Three implementations:

1) EDSLConstraintResult: Formerly ConstraintResult, implementation unchanged aside from adding a toJSON method.
2) DBConstraintResult: A record storing a Constraint Result that originated from the database
3) ProceduralConstraintResult: A record storing a
Changes the output of the plan constraints from a map to a list, as the keyset is only accessed for a `remove` command.

Remove ConstraintRunRecord, as DBConstraintResult replaces it
…s Exception>>

Refactors ConstraintsCompilationError to be an Extension
If a running constraint throws an exception, that exception is now caught and put in the results map
Generalizes serializeConstraintCompilationErrors to handle all Constraint Errors
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4.5% Duplication on New Code (required ≤ 3%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@Mythicaeda Mythicaeda changed the title Procedural Constraints Database Support Procedural Constraints Action/Database Support Mar 13, 2025
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.

Reordering Multiple Invocations of Scheduling Goals Fails DB updates for Procedural Constraints
3 participants