-
Notifications
You must be signed in to change notification settings - Fork 7
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
Sequence Templates #1642
base: develop
Are you sure you want to change the base?
Sequence Templates #1642
Conversation
</ActivityFilterBuilder> | ||
<div class="sne-controls"> | ||
<div class="sne-filter"> | ||
<input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This input gets out of sync with the input in the filter builder modal. E.g.:
- Fill in the text box next to the new filter button
- Make a filter, but don't give it a name
- Save the filter
The created filter doesn't have the name from step 1, and the text box is still populated.
I think what I expect is that the text box should clear after I click "new". If I'm creating a sequence filter, that value should be used to seed the name of the filter in the text box in the modal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the intent of that text box - it says "Filter text", but I guess it means "filter the items in this list so I can find what I'm looking for" rather than "this is the name of the SequenceFilter"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, ah yeah that is confusing now that I think about it.. that box (which is unfortunately next to that 'New' button) is meant to filter the results in the table underneath. The name of the sequence filter you're creating or viewing is just bound to that entry in the filter builder. Maybe we should update the text in the filter box to say 'Filter items...'?
src/types/sequencing.ts
Outdated
@@ -22,6 +22,18 @@ import type { SequenceTypes } from '../enums/sequencing'; | |||
import type { ArgDelegator } from '../utilities/sequence-editor/extension-points'; | |||
import type { UserId } from './app'; | |||
import type { GlobalType } from './global-type'; | |||
import type { ActivityLayerFilter } from './timeline'; | |||
|
|||
export type SequenceActivityFilter = ActivityLayerFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be correct to say that this is the same as an ActivityLayerFilter
? The purpose of the ActivityLayerFilter
is more geared towards activities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I made SequenceActivityFilter
assuming that it would be somewhat different from ActivityLayerFilter
, but that didn't really end up being the case so I've re-factored everything back to using ActivityLayerFilter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using the SequenceActivityFilters in the timeline somewhere? It looks like you're utilizing the "filter" part of the ActivityLayerFilter
. If it's not really a timeline feature, then I think it would be less confusing to maybe rename and rework ActivityLayerFilter
and its surrounding types to something more reusable for other features by using generic types.
<div class="sne-buttons"> | ||
<button | ||
class="st-button secondary new-button" | ||
style="position: relative; z-index: 1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind avoiding static inline styles? It makes it difficult to maintain when the styles are spread out throughout the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this since I don't believe it's needed!
src/components/timeline/form/TimelineEditor/ActivityFilterBuilder.svelte
Outdated
Show resolved
Hide resolved
Another approach to do the same as the last commit - some issues w. displaying the ActivityFilterBuilder from within the modal
Debugging a naive overlay approach to integrating handlebars into the editor.
Fix Seq-language autocompletion to work outside, and only outside, of template insert fields.
Doesn't include any back-end support, but this does stub out the back end as a store that should be easy-ish to replace with a real version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments and questions. Everything looks fine otherwise!
.env
Outdated
@@ -7,5 +7,6 @@ PUBLIC_HASURA_SERVER_URL=http://localhost:8080/v1/graphql | |||
PUBLIC_HASURA_WEB_SOCKET_URL=ws://localhost:8080/v1/graphql | |||
PUBLIC_AUTH_SSO_ENABLED=false | |||
PUBLIC_TIME_PLUGIN_ENABLED=false | |||
PUBLIC_SEQUENCING_MODE=templating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether this is the default mode is something being deliberated on AERIE main (see here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we settled this over there and Matt updated it here!
<MenuItem on:click={() => showAboutModal()}> | ||
<MenuItem | ||
on:click={e => { | ||
if (e.detail.shiftKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this switch on? I think having this functionality available somewhere in the UI is helpful, but it seemed like from an earlier conversation, the decision was to delegate this to a configuration setting and to leave it at that. Is this something we mean to leave in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a long conversation about this and decided the best way to do it, for now, is through a hidden switch in the UI like this. Hopefully though in the future we'll be able to have a better way to allow the E2E tests to get to both Typescript & Templating sides of expansion
outputStr = expandedTemplate?.expanded_template ?? `No output found for sequence "${expansionSequence.seq_id}"'`; | ||
} else { | ||
effects | ||
.getExpansionSequenceSeqJson(expansionSequence.seq_id, expansionSequence.simulation_dataset_id, user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question to @mattdailis and @JosephVolosin:
This makes use of the expanded_sequences
table, which makes sense - have usages of the getSequenceSeqJson
endpoint been fully phased out in the UI at this point? It seems like we can do away with that endpoint entirely, especially if the UI has demonstrated that it can be replaced with a call to the expanded_sequences
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason we're moving away from getSequenceSeqJson
? FWIW, pulling directly from expanded_sequences
(as defined currently) has the unexpected result that if you've re-run expansion within the same simulation dataset, you end up with an array of results. We could filter that down to only provide the latest one via. the creation_time
field, but just want to make sure we really do want to swap this functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked w. Pranav and Matt on this - querying expanded_sequences
is the correct way to go forward for this PR, and we are OK grabbing the latest expansion based on creation time. Updated on the UI!
<FilterIcon /> | ||
</button> | ||
</div> | ||
<div use:tooltip={{ content: 'Apply Filter', placement: 'top' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This button doesn't do anything if the plan is empty. So creating a filter and then a sequence from that filter with an empty plan, before adding activities, is not possible.
That shouldn't be an issue but might be worth noting, especially if filter reapplication and such become desired behavior later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair question - in general, I'm not 100% certain how we want to handle situations where the action will 'fail' because there is no initial simulation
</script> | ||
|
||
<Modal {height} {width}> | ||
<ModalHeader on:close>Apply Filter To Range</ModalHeader> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should have a more intuitive title like "Create Sequence from Filter", and then "Apply Filter to Range" could be the body text? Just to make it clear that in applying a filter we are creating a sequence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments from a first pass today. I spent a while trying to debug a crash when I try to view an expanded sequence in legacy expansion mode but cut myself off before spending my entire evening on it.
<div use:tooltip={{ content: 'Expand Sequence', placement: 'top' }}> | ||
<button | ||
aria-label={`Expand '${sequenceOrFilter.seq_id}'`} | ||
class="st-button icon" | ||
on:click|stopPropagation={() => { | ||
if (isExpansionSequence(sequenceOrFilter)) { | ||
onExpandSequence(sequenceOrFilter); | ||
} | ||
}} | ||
> | ||
<PlayIcon /> | ||
</button> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's misleading that each sequence gets an "Expand Sequence" button: it seems like it'll only expand that single sequence. Is there a reason why we moved away from a single button to expand the entire plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be honest I think I may have biased this UI decision more towards GSFC use because we generally don't have the idea of 'multiple sequences' - instead one sequence representing our whole load makes more sense. However, I think a 'Run All' button definitely makes sense especially if you're using Typescript/Legacy/Command Expansion mode - so I'm okay with adding it back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK nice, yeah I totally see how we ended up there. I think, though, that even in the case of template expansion, we expand all sequences at once? I think with either legacy or template expansion, one "run" will affect all sequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an Expand All
button for both modes - the row-specific 'expand' buttons will still only expand that sequence, but Expand All
will do all that are configured for the current/latest simulation dataset
@AaronPlave / @duranb, with our new panel for |
…e the word typescript instead of legacy
|
…nsion set is selected
|
|
||
$: sequencesAndFilters = [...$expansionSequences, ...$sequenceFilters]; | ||
|
||
// TODO: Better cleanup of activeSequenceFilterName and activeSequenceFilterId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind elaborating more as to what needs to be cleaned up for activeSequenceFilterName
and activeSequenceFilterId
?
expandedTemplate => expandedTemplate.seq_id === expansionSequence.seq_id, | ||
); | ||
outputStr = expandedTemplate?.expanded_template ?? `No output found for sequence "${expansionSequence.seq_id}"'`; | ||
// TODO: Language - see question in Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind pasting the question directly here if you intend to keep this TODO?
src/types/sequencing.ts
Outdated
@@ -22,6 +22,18 @@ import type { SequenceTypes } from '../enums/sequencing'; | |||
import type { ArgDelegator } from '../utilities/sequence-editor/extension-points'; | |||
import type { UserId } from './app'; | |||
import type { GlobalType } from './global-type'; | |||
import type { ActivityLayerFilter } from './timeline'; | |||
|
|||
export type SequenceActivityFilter = ActivityLayerFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using the SequenceActivityFilters in the timeline somewhere? It looks like you're utilizing the "filter" part of the ActivityLayerFilter
. If it's not really a timeline feature, then I think it would be less confusing to maybe rename and rework ActivityLayerFilter
and its surrounding types to something more reusable for other features by using generic types.
@@ -111,6 +114,11 @@ export class ExpansionRules { | |||
await this.page.goto('/expansion/rules', { waitUntil: 'networkidle' }); | |||
await this.page.waitForTimeout(250); | |||
await expect(this.rulesNavButton).toHaveClass(/selected/); | |||
const typescriptIsToggledOff = await this.page.getByText('COMMAND_EXPANSION_MODE').isVisible(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What specifically is this trying to select? Is this some button or label? It's not immediately clear where this value is being placed in the HTML and I worry that it'll make it hard to debug later on if the text ever changes.
@@ -0,0 +1,17 @@ | |||
import { foldInside, foldNodeProp, indentNodeProp, LRLanguage } from '@codemirror/language'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is handlebars itself a standalone language and is seq-n-handlebars a sub-language?
___REQUIRES_AERIE_PR___="1638"
This PR implements the UI for sequence templates (see
aerie
PR for more information)New Environment Variable
This feature introduces a new environment variable called
PUBLIC_SEQUENCING_MODE
which is set to eithertemplating
orexpansion
depending on the feature the user wants. Command Expansion and Sequence Templating are two ways to get the same outputs, so the user must decide when they build their instance which they'd like to use.Sequence Template Editor
To create/edit/delete sequence templates, there is a new route
/sequence-templates
that brings the user to an editor based on the sequence editor in/sequencing
.The editor window utilizes
CodeMirror
and implements an 'overlay' language to support Handlebars formatting. Additionally, the same parcel/dictionary capabilities from the sequence editor are available for linting, formatting, auto-complete, etc.Plan View - Sequences & Expansion
Within the Plan view, there is a new tab called
Sequences & Expansion
which is used to create sequences and expand them using either command expansion or sequence templating. The functionality from the previous command expansion panel has been rolled into this panel. New sequences and sequence filters can be created through theNew
button next to the table filter.The table shows both expansion sequences and sequence filters. Expansion sequences can be deleted, expanded (using command expansion or sequence templating), and have the expanded output viewed if it exists.
Sequence filters can be deleted, modified after creation, and applied. 'Applying' a sequence filter generates a new expansion sequence with spans that match the filter criteria automatically attached to the sequence ID.
Step-by-step guide
Sequence Templates
(/sequence-templates
) through the Aerie logo in the top-leftNew Template
and enter a 1) Template Name, 2) Template Language (STOL
,SEQN
, orTEXT
), 3) Parcel Id, 4) Model Id, 5) Activity Type. ClickCreate Template
/C FSW_CMD_0 "ON" false 1
). ClickSave
or useCtrl+S
to save the template.Sequences & Expansion
panelNew
and create a new sequence filter. In the filter builder modal, enter a name and at least one filter constraint that will capture the previously added activity directiveCreate Sequence
button that appears, select a time range in the modal that appears that includes the previously added activity directive (or, leave it as the default - the plan time range)Expand Sequence
Show Expanded Sequence