From 32fe53d174b145501c813ff2388179d2250c1e9c Mon Sep 17 00:00:00 2001 From: Robert McMahan Date: Mon, 1 Jul 2024 14:29:58 -0400 Subject: [PATCH] Model Configuration QoL & Usability Updates (#511) * Sorts variables by name in dropdown. * Adjusts how errors are displayed to make requirements more clear. * Auto-select single source variable source when adding new variables. * Minor error verbiage adjustment when no events found. --------- Co-authored-by: Robert McMahan --- backend/controller/ml_model/bigquery.py | 1 - backend/controller/ml_model/views.py | 2 +- .../ml-model-form.component.html | 19 +- .../ml-model-form.component.sass | 10 +- .../ml-model-form/ml-model-form.component.ts | 201 +++++++++++------- 5 files changed, 148 insertions(+), 85 deletions(-) diff --git a/backend/controller/ml_model/bigquery.py b/backend/controller/ml_model/bigquery.py index b39fecef7..1844d9261 100644 --- a/backend/controller/ml_model/bigquery.py +++ b/backend/controller/ml_model/bigquery.py @@ -113,7 +113,6 @@ def get_analytics_variables(self, ) GROUP BY 1,2,3 ORDER BY - count DESC, name ASC, parameter_key ASC; """ diff --git a/backend/controller/ml_model/views.py b/backend/controller/ml_model/views.py index 7d409d6e1..2b7f1b957 100644 --- a/backend/controller/ml_model/views.py +++ b/backend/controller/ml_model/views.py @@ -275,7 +275,7 @@ def get(self): 400, message=( 'GA4 BigQuery Dataset does not include expected events tables.' - ' Check configuration in Settings tab and try again.' + ' Check configuration (project, dataset, and timespan) above and try again.' ), ) variables.extend(analytics_variables) diff --git a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.html b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.html index 14d9c7f4c..063d17e58 100644 --- a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.html +++ b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.html @@ -198,9 +198,9 @@ - + warning @@ -233,6 +233,19 @@ + + +
+ + warning + +
    +
  1. {{ errorMessage }}
    2. +
+
+ + @@ -305,7 +318,7 @@
diff --git a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.sass b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.sass index c9ff5996a..41a415f06 100644 --- a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.sass +++ b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.sass @@ -29,6 +29,10 @@ border: 1px solid #c51162 border-radius: 4px + ol + display: inline-block + vertical-align: middle + .crmi-form-offset padding-left: 220px @@ -114,6 +118,7 @@ border: 1px solid rgba(0, 0, 0, 0.3) border-radius: 4px flex-grow: 1 + flex-shrink: 0 height: auto border-spacing: 10px 0px font-size: 14px @@ -153,7 +158,10 @@ width: 27px min-width: 27px padding: 0px - + #variable-errors + padding-top: 20px + .crmi-form-message-error + margin-bottom: 0px #label .vertical-group diff --git a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.ts b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.ts index 2ea0d5626..c516e908c 100644 --- a/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.ts +++ b/frontend/src/app/ml-models/ml-model-form/ml-model-form.component.ts @@ -27,6 +27,8 @@ import { Variable, BigQueryDataset, Timespan, Source, Destination, Input, Output, Role, Comparison } from 'app/models/ml-model'; +const MIN_NUM_OF_FEATURES = 2; + @Component({ selector: 'app-ml-model-form', templateUrl: './ml-model-form.component.html', @@ -137,6 +139,11 @@ export class MlModelFormComponent implements OnInit { try { const mlModel = await this.mlModelsService.get(id); this.mlModel = plainToClass(MlModel, mlModel as MlModel); + this.mlModel.variables.sort((a: Variable, b: Variable) => { + const aCompare = `${a.role + a.source + a.name}`; + const bCompare = `${b.role + b.source + b.name}`; + return aCompare.localeCompare(bCompare); + }); this.assignMlModelToForm(); } catch (error) { if (error && error.status === 404) { @@ -198,14 +205,11 @@ export class MlModelFormComponent implements OnInit { /** * Return any error for a given control. * - * @param control The name of the control or the control itself. - * @param key The key within the control to lookup the value for (optional). - * @returns The error associated with the control (if any). + * @param control The control itself. + * @returns The error associated with the control (if any) or undefined otherwise. */ - error(control: string|AbstractControl, key: string = ''): string { - const formControl = control instanceof AbstractControl ? control : this.mlModelForm.get(control); - const errors = key ? formControl.get(key).errors : formControl.errors; - return errors ? Object.keys(errors)[0] : ''; + error(control: AbstractControl): string|undefined { + return control.errors ? Object.keys(control.errors)[0] : undefined; } get type() { @@ -297,6 +301,30 @@ export class MlModelFormComponent implements OnInit { return true; } + /** + * Check the model variables selected to ensure the necessary variables have been + * specified and if not return any errors that would require user correction. + * + * @returns List of validation errors if any and otherwise returns undefined. + */ + get variableErrors(): string[]|undefined { + let errors = []; + + const source: Source = this.value('input', 'source'); + const variables: Variable[] = this.value('variables'); + + this.checkMinimumNumberOfFeatures(variables, errors); + this.checkOneLabel(variables, errors); + + if (source.includes(Source.FIRST_PARTY)) { + this.checkUniqueId(variables, errors); + this.checkGCLID(variables, errors); + this.checkTriggerDate(variables, errors); + } + + return errors.length > 0 ? errors : undefined; + } + /** * Dynamically updates required input parameters based on the input source selected. */ @@ -327,8 +355,11 @@ export class MlModelFormComponent implements OnInit { const formVariables = this.mlModelForm.get('variables') as UntypedFormArray; formVariables.push(this._fb.group({ sources: [variableSources], - source: [null] + source: [variableSources.length === 1 ? variableSources[0] : null] })); + if (variableSources.length === 1) { + this.refreshVariables(); + } } /** @@ -340,6 +371,7 @@ export class MlModelFormComponent implements OnInit { removeVariable(index: number) { const formVariables = this.mlModelForm.get('variables') as UntypedFormArray; formVariables.removeAt(index); + this.refreshVariables(); } /** @@ -355,9 +387,6 @@ export class MlModelFormComponent implements OnInit { const dataset = this.value('bigQueryDataset'); const ts = this.value('timespans'); variables = await this.mlModelsService.getVariables(input, dataset, ts); - variables.sort((a: Variable, b: Variable) => { - return a.source.localeCompare(b.source); - }); this.cachedVariables = variables; this.errorMessage = ''; this.refreshVariables(); @@ -441,6 +470,7 @@ export class MlModelFormComponent implements OnInit { variable.comparison = existingVariable.comparison; variable.value = existingVariable.value; } + variable.hint = 'When no key is selected the feature value will be the event count for a given user.'; } if (variable.role === Role.LABEL && variable.source === Source.GOOGLE_ANALYTICS) { @@ -650,20 +680,6 @@ export class MlModelFormComponent implements OnInit { } as Output; } - /** - * Validate the ml model object to ensure everything provided by the user meets at least - * the minimum requirements to build the model out properly. - * - * @throws Errors that encapsulate the reason for validation failures. - */ - validateMlModel() { - const featuresSet = this.mlModel.variables.filter(v => v.role === Role.FEATURE).length >= 2; - const labelSet = this.mlModel.variables.filter(v => v.role === Role.LABEL).length === 1; - if (!featuresSet || !labelSet) { - throw 'At least 2 features and 1 label must be added/selected from the list of variables.'; - } - } - /** * Update the ml model object using the form data and send it to the backend to persist. */ @@ -672,8 +688,6 @@ export class MlModelFormComponent implements OnInit { this.prepareSaveMlModel(); try { - this.validateMlModel(); - if (this.mlModel.id) { await this.mlModelsService.update(this.mlModel); this.router.navigate(['ml-models', this.mlModel.id]); @@ -720,10 +734,7 @@ export class MlModelFormComponent implements OnInit { */ private variableValidator(existingVariables: Variable[]): ValidatorFn { return (control: AbstractControl): ValidationErrors | null => { - const variableSource: Source = control.get('source').value; const variableRole: Role = control.get('role').value; - const source: Source = this.value('input', 'source'); - const destination: Destination = this.value('output', 'destination'); if (variableRole) { if (variableRole !== Role.FEATURE) { @@ -744,63 +755,95 @@ export class MlModelFormComponent implements OnInit { return {valueRequiredForComparison: true} } } - } else { - // duplicative role selection should be handled first and then other errors will show after. - const singleSelectRoles = Object.keys(Role).filter(r => r !== Role.FEATURE); - for (const role of singleSelectRoles) { - const variablesWithRole = existingVariables.filter(v => v.role === role); - if (variablesWithRole.length > 1) { - return null; - } - } + } - const uniqueId: UniqueId = this.value('uniqueId'); - const includesFirstPartyData: boolean = source.includes(Source.FIRST_PARTY); - const includesGoogleAnalyticsData: boolean = source.includes(Source.GOOGLE_ANALYTICS); + return null; + } + } - if (existingVariables.filter(v => v.role === Role.LABEL).length === 0) { - return {labelNotSelected: true}; - } + /** + * Check if the minimum number of features is specified. + * + * @param variables The currently configured/selected variables. + * @param errors A running list of errors found during check calls. + */ + private checkMinimumNumberOfFeatures(variables: Variable[], errors: string[]) { + if (variables.filter(v => v.role === Role.FEATURE).length < MIN_NUM_OF_FEATURES) { + errors.push(`At least ${MIN_NUM_OF_FEATURES} features must be selected/specified.`); + } + } - if (variableSource === Source.FIRST_PARTY) { - // client ID required when unique ID is set as client ID. - if (includesFirstPartyData && uniqueId === UniqueId.CLIENT_ID && existingVariables.filter(v => v.role === Role.CLIENT_ID).length === 0) { - return {clientIdNotSelected: true}; - } + /** + * Check if one and only one label is specified. + * + * @param variables The currently configured/selected variables. + * @param errors A running list of errors found during check calls. + */ + private checkOneLabel(variables: Variable[], errors: string[]) { + if (variables.filter(v => v.role === Role.LABEL).length === 0) { + errors.push('A label must be selected/specified.'); + } else if (variables.filter(v => v.role === Role.LABEL).length > 1) { + errors.push('Only 1 label can be selected/specified.'); + } + } - // user ID required when unique ID is set as user ID. - if (includesFirstPartyData && uniqueId === UniqueId.USER_ID && existingVariables.filter(v => v.role === Role.USER_ID).length === 0) { - return {userIdNotSelected: true}; - } + /** + * Check if proper unique identifier is specified. + * + * @param variables The currently configured/selected variables. + * @param errors A running list of errors found during check calls. + */ + private checkUniqueId(variables: Variable[], errors: string[]) { + const uniqueId: UniqueId = this.value('uniqueId'); - const selectedGCLID: Variable = existingVariables.find(v => v.role === Role.GCLID); + if (uniqueId === UniqueId.CLIENT_ID && variables.filter(v => v.role === Role.CLIENT_ID).length === 0) { + errors.push('A client ID must be selected/specified due to it being selected as the user identifier.'); + } - // GCLID role required for Google Ads destination if it cannot be pulled automatically from Google Analytics. - if (!includesGoogleAnalyticsData && destination === Destination.GOOGLE_ADS_OFFLINE_CONVERSION && !selectedGCLID) { - return {gclidNotSelected: true}; - } + if (uniqueId === UniqueId.USER_ID && variables.filter(v => v.role === Role.USER_ID).length === 0) { + errors.push('A user ID must be selected/specified due to it being selected as the user identifier.'); + } + } - const selectedTriggerDate: Variable = existingVariables.find(v => v.role === Role.TRIGGER_DATE); - - if (includesFirstPartyData && !selectedTriggerDate) { - // check if possible to derive from Google Analytics variables that were selected and if not return validation error. - if (includesGoogleAnalyticsData) { - const selectedTrigger: Variable = existingVariables.find(v => [Role.FIRST_VALUE, Role.TRIGGER_EVENT].includes(v.role)); - const selectedLabel = existingVariables.find(v => v.role === Role.LABEL); - - if ((!selectedTrigger && selectedLabel.source !== Source.GOOGLE_ANALYTICS) || (selectedTrigger && selectedTrigger.source === Source.FIRST_PARTY)) { - return {triggerDateNotSelected: true}; - } - // trigger date role is required in the absence of Google Analytics data to - // understand applicability of data based on configured timespan. - } else { - return {triggerDateRequiredForTimespanAdherence: true}; - } - } - } - } + /** + * Check if GCLID provided when Google Ads is set as the destination + * if the GCLID cannot be pulled automatically from Google Analytics. + * + * @param variables The currently configured/selected variables. + * @param errors A running list of errors found during check calls. + */ + private checkGCLID(variables: Variable[], errors: string[]) { + const source: Source = this.value('input', 'source'); + const destination: Destination = this.value('output', 'destination'); + const selectedGCLID: Variable = variables.find(v => v.role === Role.GCLID); - return null; + if (!source.includes(Source.GOOGLE_ANALYTICS) && destination === Destination.GOOGLE_ADS_OFFLINE_CONVERSION && !selectedGCLID) { + errors.push('A GCLID must be selected/specified due to the specified destination and no way to automatically derive.'); + } + } + + /** + * Check if possible to derive trigger date from Google Analytics variables + * that were selected and if not then add error to the errors list. + * + * @param variables The currently configured/selected variables. + * @param errors A running list of errors found during check calls. + */ + private checkTriggerDate(variables: Variable[], errors: string[]) { + const selectedTriggerDate: Variable = variables.find(v => v.role === Role.TRIGGER_DATE); + + if (!selectedTriggerDate) { + const source: Source = this.value('input', 'source'); + const selectedTrigger: Variable = variables.find(v => [Role.FIRST_VALUE, Role.TRIGGER_EVENT].includes(v.role)); + const selectedLabel = variables.find(v => v.role === Role.LABEL); + + if ( + !source.includes(Source.GOOGLE_ANALYTICS) || + (!selectedTrigger && selectedLabel && selectedLabel.source !== Source.GOOGLE_ANALYTICS) || + (selectedTrigger && selectedTrigger.source === Source.FIRST_PARTY) + ) { + errors.push('A trigger date must be selected/specified due to there being no way to automatically derive.'); + } } }