Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
Copy Markdown
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 {
Comment thread
octogonz marked this conversation as resolved.
Outdated
if (!percentageRegExp.test(weight)) {
throw new Error(`Expected a percentage string like "100%".`);
Comment thread
octogonz marked this conversation as resolved.
Outdated
}

const percentValue: number = parseFloat(weight.slice(0, -1));
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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;
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 42 to 44
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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);
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()`."
Comment thread
LPegasus marked this conversation as resolved.
Outdated
},
{
"description": "The number of concurrency units that this operation should take up. The maximum concurrency units is determined by the -p flag.",
Comment thread
LPegasus marked this conversation as resolved.
Outdated
"type": "integer",
"minimum": 0
}
]
},
"allowCobuildWithoutCache": {
"type": "boolean",
Expand Down