Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"
}
2 changes: 1 addition & 1 deletion common/reviews/api/rush-lib.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ export interface IOperationSettings {
outputFolderNames?: string[];
parameterNamesToIgnore?: string[];
sharding?: IRushPhaseSharding;
weight?: number;
weight?: number | `${number}%`;
}

// @internal (undocumented)
Expand Down
2 changes: 1 addition & 1 deletion libraries/rush-lib/src/api/RushProjectConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export interface IOperationSettings {
* How many concurrency units this operation should take up during execution. The maximum concurrent units is
* determined by the -p flag.
*/
weight?: number;
weight?: number | `${number}%`;

/**
* If true, this operation can use cobuilds for orchestration without restoring build cache entries.
Expand Down
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';
Expand Down Expand Up @@ -31,6 +33,29 @@ function weightOperations(
context: ICreateOperationsContext
): Map<Operation, IOperationExecutionResult> {
const { projectConfigurations } = context;
const availableParallelism: number = os.availableParallelism();
Copy link
Contributor

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.


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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deduplicate this against

numberOfCores: number = os.availableParallelism?.() ?? os.cpus().length

if (!percentageRegExp.test(weight)) {
throw new Error(`Expected a percentage string like "100%".`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify the weirdness that strings-are-percentages-but-numbers-are-not:

Since the JSON value is a string, it must be a percentage like "50%"

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseFloat doesn't mind the trailing %. Parsing stops as soon as it hits a character that isn't part of the float.


// Use as much CPU as possible, so we round down the weight here
return Math.floor((percentValue / 100) * availableParallelism);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmichon-msft Is it okay if this rounds down to 0?

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find myself wondering why this layer is here; operation.settings should always already be defined if projectConfigurations.get(project)?.operationSettingsByOperationName.get(phase.name) returns a truthy value.

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);
Expand Down
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);
});
});
15 changes: 12 additions & 3 deletions libraries/rush-lib/src/schemas/rush-project.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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()`."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "The percentage of concurrency units that this operation should take up. The maximum concurrency units is determined by `os.availableParallelism()`."
"description": "The number of concurrency units that this operation should take up, as a percent of `os.availableParallelism()`. At runtime this value will be clamped to the range `[1, rushParallelism]`, where `rushParallelism` is the requested parallelism to the current Rush command. To have this operation consume no concurrency, use the literal value `0`."

},
{
"description": "The number of concurrency units that this operation should take up. The maximum concurrency units is determined by the -p flag.",
Copy link
Contributor

Choose a reason for hiding this comment

The 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.",
"description": "The number of concurrency units that this operation should take up. At runtime this value will be clamped to the range `[0, rushParallelism]`, where `rushParallelism` is the requested parallelism to the current Rush command.",

"type": "integer",
"minimum": 0
}
]
},
"allowCobuildWithoutCache": {
"type": "boolean",
Expand Down