Conversation
📊 Test coverage report
|
There was a problem hiding this comment.
Pull request overview
Updates the learning-parameter rendering logic to only show parameters whose dependency conditions are satisfied (and omits dependent parameters that don’t apply), including in the trained model “Learning Parameters” view.
Changes:
- Renamed and repurposed the dependency utility to filter (and effectively re-order) learning parameters based on
depends_on. - Updated the train-model advanced settings list to use the new dependency filtering utility.
- Applied the same filtering to the trained model “Learning Parameters” display.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/utils.ts | Renames the utility and updates recursive usage to filter dependent parameters. |
| application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/utils.test.ts | Renames the test suite and updates expectations to match the new function name/behavior. |
| application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/learning-parameters-list.component.tsx | Switches to the new filtering utility when deriving displayed learning parameters. |
| application/ui/src/features/models/model-listing/model-training-parameters/model-training-parameters.component.tsx | Filters learning parameters in the trained-model view prior to rendering. |
Comments suppressed due to low confidence (3)
application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/utils.ts:62
filterDependentParametersis doing more than filtering: it also reorders dependents to appear after their controlling parameter and can drop parameters (dependents) entirely. To avoid misleading callers, either (a) rename the function to reflect ordering as well (e.g.,orderAndFilterDependentParameters), or (b) adjust implementation to only filter while preserving original order.
export const filterDependentParameters = (
parameters: LearningConfigurationParameters[]
): LearningConfigurationParameters[] => {
return parameters
.reduce<LearningConfigurationParameters[][]>((acc, curr) => {
if (isParameter(curr) && curr.depends_on == null) {
const parametersDependingOnCurr = parameters.filter((parameter) => {
return (
isParameter(parameter) &&
parameter.depends_on != null &&
parameter.depends_on[curr.key] === curr.value
);
});
return [...acc, [curr, ...parametersDependingOnCurr]];
}
application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/utils.ts:59
- This matching logic can incorrectly include the same dependent parameter multiple times when
depends_oncontains multiple keys (it will match once per satisfied key as differentcurrare processed). It also doesn’t enforce that all dependency predicates are met (common fordepends_onobjects with multiple entries). Consider evaluating each dependent parameter against the full set of current parameter values (require alldepends_onentries to match) and ensure each parameter is emitted at most once.
const parametersDependingOnCurr = parameters.filter((parameter) => {
return (
isParameter(parameter) &&
parameter.depends_on != null &&
parameter.depends_on[curr.key] === curr.value
);
});
application/ui/src/features/models/train-model/advanced-settings/training/learning-parameters/utils.ts:53
- This is O(n²) due to
parameters.filter(...)running for each independent parameter. If parameter counts can grow (or if this runs frequently), consider pre-indexing dependents (e.g., build a map from dependency key/value to dependents) and then emitting in a single pass to reduce repeated scans.
return parameters
.reduce<LearningConfigurationParameters[][]>((acc, curr) => {
if (isParameter(curr) && curr.depends_on == null) {
const parametersDependingOnCurr = parameters.filter((parameter) => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isParameterGroup(curr)) { | ||
| const groupedParameters = reorderDependentParameters(curr.parameters); | ||
| const groupedParameters = filterDependentParameters(curr.parameters); | ||
|
|
There was a problem hiding this comment.
After filtering, parameter groups can become empty, but they are still emitted. In TrainingConfigurationParametersList, empty groups will still render a group row, preventing the "No parameters." empty-state from showing and producing group headers with no children. Consider omitting groups whose groupedParameters.length === 0 (or filtering them out at the end) so the UI reflects an actually-empty parameter set.
| if (groupedParameters.length === 0) { | |
| return acc; | |
| } |
|
|
||
| import { TrainingConfigurationParameter } from '../../../../constants/shared-types'; | ||
| import { useGetModelTrainingConfiguration } from '../../hooks/api/use-get-model-training-configuration.hook'; | ||
| import { filterDependentParameters } from '../../train-model/advanced-settings/training/learning-parameters/utils'; |
There was a problem hiding this comment.
This introduces a cross-feature dependency from model-listing into train-model/advanced-settings/.... To reduce coupling (and avoid potential circular dependencies as these areas evolve), consider relocating filterDependentParameters into a shared/domain-level module (e.g., a training-configuration utils file) that both features can import from.
| }); | ||
|
|
||
| describe('reorderDependentParameters', () => { | ||
| describe('filterDependentParameters', () => { |
There was a problem hiding this comment.
Given the updated behavior, add tests for dependency objects with multiple keys (to ensure dependents are included only when all predicates match and only once), and for filtered-out empty parameter groups (to ensure groups with no remaining children are omitted if that’s the intended UI behavior).
Docker Image SizesCPU
CUDA
XPU
|
Summary
How to test
Checklist