-
Notifications
You must be signed in to change notification settings - Fork 678
[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
base: main
Are you sure you want to change the base?
Changes from all commits
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 { | ||||
|
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. Deduplicate this against
|
||||
| if (!percentageRegExp.test(weight)) { | ||||
| throw new Error(`Expected a percentage string like "100%".`); | ||||
|
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. Clarify the weirdness that strings-are-percentages-but-numbers-are-not:
Contributor
Author
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. Added more comments to clarify the weirdness. |
||||
| } | ||||
|
|
||||
| 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.
|
||||
|
|
||||
| // 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. |
||||
| } | ||||
|
|
||||
| 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
66
to
68
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; |
||||
| 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); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -106,9 +106,18 @@ | |||||
| ] | ||||||
| }, | ||||||
| "weight": { | ||||||
| "description": "The number of concurrency units that this operation should take up. The maximum concurrency units is determined by the -p flag.", | ||||||
| "type": "integer", | ||||||
| "minimum": 0 | ||||||
| "oneOf": [ | ||||||
| { | ||||||
| "type": "string", | ||||||
| "pattern": "^[1-9][0-9]*(\\.\\d+)?%$", | ||||||
| "description": "The percentage of concurrency units that this operation should take up. The maximum concurrency units is determined by `os.availableParallelism()`." | ||||||
|
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.
Suggested change
|
||||||
| }, | ||||||
| { | ||||||
| "description": "The number of concurrency units that this operation should take up. The maximum concurrency units is determined by the -p flag.", | ||||||
|
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.
Suggested change
|
||||||
| "type": "integer", | ||||||
| "minimum": 0 | ||||||
| } | ||||||
| ] | ||||||
| }, | ||||||
| "allowCobuildWithoutCache": { | ||||||
| "type": "boolean", | ||||||
|
|
||||||
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.