Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions deployment/hasura/metadata/actions.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,15 @@ type Mutation {
}

type Mutation {
expandAllActivities(expansionSetId: Int!, simulationDatasetId: Int!): ExpandAllActivitiesResponse
expandAllActivities(expansionSetId: Int!, simulationDatasetId: Int!, bypassConstraints: Boolean): ExpandAllActivitiesResponse
}

type Mutation {
expandAllTemplates(
seqIds: [String!]!,
simulationDatasetId: Int!,
modelId: Int!
modelId: Int!,
bypassConstraints: Boolean,
): ExpandAllSequencesResponse
}

Expand Down
120 changes: 119 additions & 1 deletion sequencing-server/src/routes/command-expansion.ts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd like to request six e2e tests, which are each of these updated endpoints run under the following conditions:

  1. there is a constraint run with violations on the sim dataset
  2. there is a constraint run without violations on the sim dataset
  3. 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.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { UserCodeError } from '@nasa-jpl/aerie-ts-user-code-runner';
import pgFormat from 'pg-format';
import type { Context } from '../app.js';
import { db, piscina, promiseThrottler, typeCheckingCache } from './../app.js';
import { db, graphqlClient, piscina, promiseThrottler, typeCheckingCache } from './../app.js';
import { Result } from '@nasa-jpl/aerie-ts-user-code-runner/build/utils/monads.js';
import express from 'express';
import { serializeWithTemporal } from './../utils/temporalSerializers.js';
Expand All @@ -25,6 +25,7 @@ import { stringifyActivity } from '../lib/mustache/util/activity.js';
import { stolBuilder } from '../builders/stolBuilder.js';
import { concatBuilder } from "../builders/concatBuilder.js";
import { SequencingLanguage } from '../lib/mustache/enums/language.js';
import { gql } from 'graphql-request';

const logger = getLogger('app');

Expand Down Expand Up @@ -359,6 +360,7 @@ commandExpansionRouter.post('/expand-all-sequence-templates', async (req, res, n

// 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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

const modelId = req.body.input.modelId as number;
const simulationDatasetId = req.body.input.simulationDatasetId as number;
const seqIds = (req.body.input.seqIds as number[]).filter((val, index, arr) => arr.indexOf(val) == index); // remove duplicates, if they're even possible
Expand All @@ -367,6 +369,64 @@ commandExpansionRouter.post('/expand-all-sequence-templates', async (req, res, n
simulationDatasetId
}

// 0b. [OPTIONAL] Verify that for the given simulationDatasetId, constraints are up to date
if (!bypassConstraints) {
// We only block on *violations*. We intentionally don't fail on stale sims,
// unchecked constraints, or constraint errors — only on confirmed violations
// in the most recent constraint request for this simulation dataset.
const { constraint_request } = await graphqlClient.request<{
constraint_request: {
constraints_run: {
results: {
errors: object,
results: {
gaps: object[],
violations: object[]
}
}
}[]
}[]
}>(
gql`
query GetLatestConstraintViolations($simulationDatasetId: Int!) {
constraint_request (
where: { simulation_dataset_id: { _eq: $simulationDatasetId } }
order_by: { requested_at: desc }
limit: 1
) {
constraints_run {
results {
results
}
}
}
}
`,
{ simulationDatasetId },
);

const latestRequest = constraint_request[0];
if (latestRequest === undefined) {
throw new Error(
`POST /command-expansion/expand-all-sequence-templates: Expansion for simulation dataset ${simulationDatasetId} failed, as constraints haven't been checked yet.`,
);
}

const numViolations = latestRequest.constraints_run.reduce(
(total, run) => total + (run.results?.results?.violations?.length ?? 0),
0,
);

if (numViolations > 0) {
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' : ''}.`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Suggested change
} still ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`,
} ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`,

);
}
}


// 1. Load simulated activities and templates
const [sequenceTemplates, filteredSimulatedActivitiesBySeqId] = await Promise.all([
context.sequenceTemplateDataLoader.load({ modelId }),
Expand Down Expand Up @@ -591,13 +651,71 @@ commandExpansionRouter.post('/expand-all-activity-instances', async (req, res, n
const context: Context = res.locals['context'];

// Query for expansion set data
const bypassConstraints = req.body.input.bypassConstraints !== undefined ? (req.body.input.bypassConstraints as boolean) : true;
const expansionSetId = req.body.input.expansionSetId as number;
const simulationDatasetId = req.body.input.simulationDatasetId as number;
const [expansionSet, simulatedActivities] = await Promise.all([
context.expansionSetDataLoader.load({ expansionSetId }),
context.simulatedActivitiesDataLoader.load({ simulationDatasetId }),
]);

// [OPTIONAL] Verify that for the given simulationDatasetId, constraints are up to date
if (!bypassConstraints) {
// We only block on *violations*. We intentionally don't fail on stale sims,
// unchecked constraints, or constraint errors — only on confirmed violations
// in the most recent constraint request for this simulation dataset.
const { constraint_request } = await graphqlClient.request<{
constraint_request: {
constraints_run: {
results: {
errors: object,
results: {
gaps: object[],
violations: object[]
}
}
}[]
}[]
}>(
gql`
query GetLatestConstraintViolations($simulationDatasetId: Int!) {
constraint_request (
where: { simulation_dataset_id: { _eq: $simulationDatasetId } }
order_by: { requested_at: desc }
limit: 1
) {
constraints_run {
results {
results
}
}
}
}
`,
{ simulationDatasetId },
);

const latestRequest = constraint_request[0];
if (latestRequest === undefined) {
throw new Error(
`POST /command-expansion/expand-all-activity-instances: Expansion for simulation dataset ${simulationDatasetId} failed, as constraints haven't been checked yet.`,
);
}

const numViolations = latestRequest.constraints_run.reduce(
(total, run) => total + (run.results?.results?.violations?.length ?? 0),
0,
);

if (numViolations > 0) {
throw new Error(
`POST /command-expansion/expand-all-activity-instances: Expansion for simulation dataset ${simulationDatasetId} failed, as there ${
(numViolations > 1) ? 'are' : 'is'
} still ${numViolations} violation${(numViolations > 1) ? 's' : ''}.`,
);
}
}

const missionModelId = expansionSet.missionModel.id;
const commandTypes = expansionSet.parcel.command_dictionary.commandTypesTypeScript;

Expand Down
Loading