-
Notifications
You must be signed in to change notification settings - Fork 683
[rush] Support percentage-based weights for operationSettings in rush-project.json #5679
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
Changes from 2 commits
02dea2d
50b9ee3
2fdb23d
ab905d9
afb6812
79279fc
d05a579
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "@microsoft/rush", | ||
| "comment": "Support percentage weight in operationSettings in rush-project.json file.", | ||
| "type": "none" | ||
| } | ||
| ], | ||
| "packageName": "@microsoft/rush" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. | ||
| // See LICENSE in the project root for license information. | ||
|
|
||
| import os from 'node:os'; | ||
|
|
||
| import { Async } from '@rushstack/node-core-library'; | ||
|
|
||
| import type { Operation } from './Operation'; | ||
|
|
@@ -31,6 +33,29 @@ function weightOperations( | |
| context: ICreateOperationsContext | ||
| ): Map<Operation, IOperationExecutionResult> { | ||
| const { projectConfigurations } = context; | ||
| const availableParallelism: number = os.availableParallelism(); | ||
|
|
||
| const percentageRegExp: RegExp = /^[1-9][0-9]*(\.\d+)?%$/; | ||
|
|
||
| /** | ||
| * Since the JSON value is a string, it must be a percentage like "50%", | ||
| * which we convert to a number based on the available parallelism. | ||
| * For example, if the available parallelism (not the -p flag) is 8 and the weight is "50%", | ||
| * then the resulting weight will be 4. | ||
| * | ||
| * @param weight | ||
| * @returns | ||
| */ | ||
| function _tryConvertPercentWeight(weight: `${number}%`): number { | ||
|
octogonz marked this conversation as resolved.
Outdated
|
||
| if (!percentageRegExp.test(weight)) { | ||
| throw new Error(`Expected a percentage string like "100%".`); | ||
|
octogonz marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| const percentValue: number = parseFloat(weight.slice(0, -1)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the current code might be better; it is still correct and doesn't require the reader to have intimate knowledge of quirky API designs that were acceptable when JavaScript was first invented. :) |
||
|
|
||
| // Use as much CPU as possible, so we round down the weight here | ||
| return Math.floor((percentValue / 100) * availableParallelism); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dmichon-msft Is it okay if this rounds down to 0?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be tempted to suggest that if it rounds down to 0, you use the raw value just so it doesn't produce infinite parallelism, but passing floats to the concurrency engine isn't terribly advisable (you can accumulate error from adding and subtracting them). Probably restrict values to [1,Infinity) from percentage weights. If we want to support fractional units we should redefine 1 concurrency unit as some power-of-two fraction of a CPU core (maybe 1/4?) and then propagate that all the way through the system.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to: return Math.max(1, Math.floor((percentValue / 100) * availableParallelism)); |
||
| } | ||
|
|
||
| for (const [operation, record] of operations) { | ||
| const { runner } = record as OperationExecutionRecord; | ||
|
|
@@ -41,8 +66,18 @@ function weightOperations( | |
| const projectConfiguration: RushProjectConfiguration | undefined = projectConfigurations.get(project); | ||
| const operationSettings: IOperationSettings | undefined = | ||
| operation.settings ?? projectConfiguration?.operationSettingsByOperationName.get(phase.name); | ||
|
Comment on lines
42
to
44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find myself wondering why this layer is here;
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems out of scope |
||
| if (operationSettings?.weight) { | ||
| operation.weight = operationSettings.weight; | ||
| if (operationSettings?.weight !== undefined) { | ||
| if (typeof operationSettings.weight === 'number') { | ||
| operation.weight = operationSettings.weight; | ||
| } else if (typeof operationSettings.weight === 'string') { | ||
| try { | ||
| operation.weight = _tryConvertPercentWeight(operationSettings.weight); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `${operation.name} (invalid weight: ${JSON.stringify(operationSettings.weight)}) ${(error as Error).message}` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Async.validateWeightedIterable(operation); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. | ||
| // See LICENSE in the project root for license information. | ||
|
|
||
| import os from 'node:os'; | ||
|
|
||
| import type { IPhase } from '../../../api/CommandLineConfiguration'; | ||
| import type { RushConfigurationProject } from '../../../api/RushConfigurationProject'; | ||
| import type { IOperationSettings, RushProjectConfiguration } from '../../../api/RushProjectConfiguration'; | ||
| import { | ||
| type IExecuteOperationsContext, | ||
| PhasedCommandHooks | ||
| } from '../../../pluginFramework/PhasedCommandHooks'; | ||
| import type { IOperationExecutionResult } from '../IOperationExecutionResult'; | ||
| import { Operation } from '../Operation'; | ||
| import { WeightedOperationPlugin } from '../WeightedOperationPlugin'; | ||
| import { MockOperationRunner } from './MockOperationRunner'; | ||
|
|
||
| const MOCK_PHASE: IPhase = { | ||
| name: '_phase:test', | ||
| allowWarningsOnSuccess: false, | ||
| associatedParameters: new Set(), | ||
| dependencies: { | ||
| self: new Set(), | ||
| upstream: new Set() | ||
| }, | ||
| isSynthetic: false, | ||
| logFilenameIdentifier: '_phase_test', | ||
| missingScriptBehavior: 'silent' | ||
| }; | ||
|
|
||
| function createProject(packageName: string): RushConfigurationProject { | ||
| return { | ||
| packageName | ||
| } as RushConfigurationProject; | ||
| } | ||
|
|
||
| function createOperation(options: { | ||
| project: RushConfigurationProject; | ||
| settings?: IOperationSettings; | ||
| isNoOp?: boolean; | ||
| }): Operation { | ||
| const { project, settings, isNoOp } = options; | ||
| return new Operation({ | ||
| phase: MOCK_PHASE, | ||
| project, | ||
| settings, | ||
| runner: new MockOperationRunner(`${project.packageName} (${MOCK_PHASE.name})`, undefined, false, isNoOp), | ||
| logFilenameIdentifier: `${project.packageName}_phase_test` | ||
| }); | ||
| } | ||
|
|
||
| function createExecutionRecords(operation: Operation): Map<Operation, IOperationExecutionResult> { | ||
| return new Map([ | ||
| [ | ||
| operation, | ||
| { | ||
| operation, | ||
| runner: operation.runner | ||
| } as unknown as IOperationExecutionResult | ||
| ] | ||
| ]); | ||
| } | ||
|
|
||
| function createContext( | ||
| projectConfigurations: ReadonlyMap<RushConfigurationProject, RushProjectConfiguration>, | ||
| parallelism: number = os.availableParallelism() | ||
| ): IExecuteOperationsContext { | ||
| return { | ||
| projectConfigurations, | ||
| parallelism | ||
| } as IExecuteOperationsContext; | ||
| } | ||
|
|
||
| async function applyWeightPluginAsync( | ||
| operations: Map<Operation, IOperationExecutionResult>, | ||
| context: IExecuteOperationsContext | ||
| ): Promise<void> { | ||
| const hooks: PhasedCommandHooks = new PhasedCommandHooks(); | ||
| new WeightedOperationPlugin().apply(hooks); | ||
| await hooks.beforeExecuteOperations.promise(operations, context); | ||
| } | ||
|
|
||
| function mockAvailableParallelism(value: number): jest.SpyInstance<number, []> { | ||
| return jest.spyOn(os, 'availableParallelism').mockReturnValue(value); | ||
| } | ||
|
|
||
| describe(WeightedOperationPlugin.name, () => { | ||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('applies numeric weight from operation settings', async () => { | ||
| const project: RushConfigurationProject = createProject('project-number'); | ||
| const operation: Operation = createOperation({ | ||
| project, | ||
| settings: { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: 7 | ||
| } | ||
| }); | ||
|
|
||
| await applyWeightPluginAsync( | ||
| createExecutionRecords(operation), | ||
| createContext(new Map(), /* Set parallelism to ensure -p does not affect weight calculation */ 1) | ||
| ); | ||
|
|
||
| expect(operation.weight).toBe(7); | ||
| }); | ||
|
|
||
| it('converts percentage weight using available parallelism', async () => { | ||
| mockAvailableParallelism(10); | ||
|
|
||
| const project: RushConfigurationProject = createProject('project-percent'); | ||
| const operation: Operation = createOperation({ | ||
| project, | ||
| settings: { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: '25%' | ||
| } as IOperationSettings | ||
| }); | ||
|
|
||
| await applyWeightPluginAsync(createExecutionRecords(operation), createContext(new Map())); | ||
|
|
||
| expect(operation.weight).toBe(2); | ||
| }); | ||
|
|
||
| it('reads weight from rush-project configuration when operation settings are undefined', async () => { | ||
| mockAvailableParallelism(8); | ||
|
|
||
| const project: RushConfigurationProject = createProject('project-config'); | ||
| const operation: Operation = createOperation({ project }); | ||
| const projectConfiguration: RushProjectConfiguration = { | ||
| operationSettingsByOperationName: new Map([ | ||
| [ | ||
| MOCK_PHASE.name, | ||
| { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: '50%' | ||
| } as IOperationSettings | ||
| ] | ||
| ]) | ||
| } as unknown as RushProjectConfiguration; | ||
|
|
||
| await applyWeightPluginAsync( | ||
| createExecutionRecords(operation), | ||
| createContext(new Map([[project, projectConfiguration]])) | ||
| ); | ||
|
|
||
| expect(operation.weight).toBe(4); | ||
| }); | ||
|
|
||
| it('use floor when converting percentage weight to avoid zero weight', async () => { | ||
| mockAvailableParallelism(16); | ||
|
|
||
| const project: RushConfigurationProject = createProject('project-floor'); | ||
| const operation: Operation = createOperation({ | ||
| project, | ||
| settings: { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: '33.3333%' | ||
| } as IOperationSettings | ||
| }); | ||
|
|
||
| await applyWeightPluginAsync(createExecutionRecords(operation), createContext(new Map())); | ||
|
|
||
| expect(operation.weight).toBe(5); | ||
| }); | ||
|
|
||
| it('forces NO-OP operation weight to zero ignore weight settings', async () => { | ||
| const project: RushConfigurationProject = createProject('project-no-op'); | ||
| const operation: Operation = createOperation({ | ||
| project, | ||
| isNoOp: true, | ||
| settings: { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: 100 | ||
| } | ||
| }); | ||
|
|
||
| await applyWeightPluginAsync(createExecutionRecords(operation), createContext(new Map())); | ||
|
|
||
| expect(operation.weight).toBe(0); | ||
| }); | ||
|
|
||
| it('throws for invalid percentage weight format', async () => { | ||
| mockAvailableParallelism(16); | ||
|
|
||
| const project: RushConfigurationProject = createProject('project-invalid'); | ||
| const operation: Operation = createOperation({ | ||
| project, | ||
| // @ts-expect-error Testing invalid input | ||
| settings: { | ||
| operationName: MOCK_PHASE.name, | ||
| weight: '12.5a%' | ||
| } as IOperationSettings | ||
| }); | ||
|
|
||
| await expect( | ||
| applyWeightPluginAsync(createExecutionRecords(operation), createContext(new Map())) | ||
| ).rejects.toThrow(/invalid weight: "12.5a%"/i); | ||
| }); | ||
| }); |
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'd consider it cleaner, especially for mocking, to pass this value to the plugin constructor.