Optionally Disable Expansion if Constraints Violated#1837
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Any reason not to also have this gating on non-template expansion? |
|
| throw new Error( | ||
| `POST /command-expansion/expand-all-sequence-templates: Expansion for simulation dataset ${simulationDatasetId} failed, as there ${ | ||
| (numViolations > 1) ? 'are' : 'is' | ||
| } still ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`, |
There was a problem hiding this comment.
Extremely minor wording nitpick: why the still here? Assuming you don't change your constraints to fit your sim results (which seems like bad practice), you'd need a separate sim run for those violations to not apply, wouldn't you?
| } still ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`, | |
| } ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`, |
| /** | ||
| * ARGUMENTS | ||
| * { | ||
| * modelId: Int!, | ||
| * simulationDatasetId: Int!, | ||
| * seqIds: [Int!]! | ||
| * } | ||
| */ |
There was a problem hiding this comment.
Please update this comment to include the added argument
| /** | |
| * ARGUMENTS | |
| * { | |
| * modelId: Int!, | |
| * simulationDatasetId: Int!, | |
| * seqIds: [Int!]!, | |
| * bypassConstraints: Boolean | |
| * } | |
| */ |
|
|
||
| // 0. Extract stuff from request | ||
| // needed to uniquely identify sequence templates, along with activity type | ||
| const bypassConstraints = req.body.input.bypassConstraints !== undefined ? (req.body.input.bypassConstraints as boolean) : true; |
There was a problem hiding this comment.
Given that the default behavior is to bypass constraints, it makes more sense logically for this argument to be inverted. That is, it's called something like referenceConstraints and defaults to false. As a user, it'd be very weird to have to input variable = false to activate new behavior.
There was a problem hiding this comment.
I'd like to request six e2e tests, which are each of these updated endpoints run under the following conditions:
- there is a constraint run with violations on the sim dataset
- there is a constraint run without violations on the sim dataset
- there are no constraints run on the sim dataset
In all three cases, the endpoint is set to be checking that there are no constraint violations.
4 participants
Description
The issue in question involves adding a warning to the UI to notify planners when there are violations on their plan before expansion.
Here, a 'headless' analogue was added, which throws an error when there are constraint violations during a call to
expandAllTemplatesif they are not explicitly bypassed.The default is to bypass this check, so as to avoid interference with past behavior.
Currently, erroring is only thrown when there are violations for a given simulation ID. Checks that are made on the UI side, like the simulation being up to date, unchecked constraints, and the like, are not done here as erroring for those other issues is not considered necessary.
Verification
They were tested locally, both with and without the UI updates (where removal of the UI update and explicitly setting
bypassConstraintsto false activated this feature).Documentation
None.
Future work
None anticipated.