[rush] Support percentage-based weights for operationSettings in rush-project.json#5679
[rush] Support percentage-based weights for operationSettings in rush-project.json#5679LPegasus wants to merge 2 commits intomicrosoft:mainfrom
Conversation
LPegasus
requested review from
apostolisms,
bmiddha,
dmichon-msft,
iclanton,
jxanthony and
octogonz
as code owners
| expect(operation.weight).toBe(4); | ||
| }); | ||
|
|
||
| it('use ceiling when converting percentage weight to avoid zero weight', async () => { |
There was a problem hiding this comment.
This says "use ceiling" but above the code is doing Math.floor()
There was a problem hiding this comment.
Updated test name.
| { | ||
| "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 the -p flag." |
There was a problem hiding this comment.
The maximum concurrency units is determined by the -p flag.
Is this correct? It seems like now the maximum percentage is determined based on os.availableParallelism()?
There was a problem hiding this comment.
Fixed description.
| const percentValue: number = parseFloat(weight.slice(0, -1)); | ||
|
|
||
| // Use as much CPU as possible, so we round down the weight here | ||
| return Math.floor((percentValue / 100) * availableParallelism); |
There was a problem hiding this comment.
@dmichon-msft Is it okay if this rounds down to 0?
There was a problem hiding this comment.
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.
| operation.weight = _tryConvertPercentWeight(operationSettings.weight); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `${operation.name} (invalid weight: ${operationSettings.weight}) ${error instanceof Error ? error.message : String(error)}` |
There was a problem hiding this comment.
| `${operation.name} (invalid weight: ${operationSettings.weight}) ${error instanceof Error ? error.message : String(error)}` | |
| `${operation.name} (invalid weight: ${JSON.stringify(operationSettings.weight)}) ${(error as Error).message}` |
Rush Stack coding conventions assume that throw will never throw an object that doesn't implement Error. (This practice is typical of a professional code base, and assuming it avoids a lot of sanity checks in catch statements.)
|
|
||
| function _tryConvertPercentWeight(weight: `${number}%`): number { | ||
| if (!percentageRegExp.test(weight)) { | ||
| throw new Error(`Expected a percentage string like "100%".`); |
There was a problem hiding this comment.
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%"
There was a problem hiding this comment.
Added more comments to clarify the weirdness.
LPegasus
force-pushed
the
rush-pr-issue-5607
branch
from
dd7ca9a to
53b44c6
Compare
1. libraries/rush-lib/src/logic/operations/WeightedOperationPlugin.ts
- Added more comments to explain the percentage weight convert function.
- Use `as` syntax to cast error to Error type for better readability.
2. libraries/rush-lib/src/schemas/rush-project.schema.json
- Updated the description of the weight property to reflect that the maximum concurrency units is determined by `os.availableParallelism()`.
3. libraries/rush-lib/src/logic/operations/test/WeightedOperationPlugin.test.ts
- Fix a test name.
LPegasus
force-pushed
the
rush-pr-issue-5607
branch
from
53b44c6 to
50b9ee3
Compare
| * @param weight | ||
| * @returns | ||
| */ | ||
| function _tryConvertPercentWeight(weight: `${number}%`): number { |
There was a problem hiding this comment.
Deduplicate this against
| context: ICreateOperationsContext | ||
| ): Map<Operation, IOperationExecutionResult> { | ||
| const { projectConfigurations } = context; | ||
| const availableParallelism: number = os.availableParallelism(); |
There was a problem hiding this comment.
I'd consider it cleaner, especially for mocking, to pass this value to the plugin constructor.
| throw new Error(`Expected a percentage string like "100%".`); | ||
| } | ||
|
|
||
| const percentValue: number = parseFloat(weight.slice(0, -1)); |
There was a problem hiding this comment.
parseFloat doesn't mind the trailing %. Parsing stops as soon as it hits a character that isn't part of the float.
| const percentValue: number = parseFloat(weight.slice(0, -1)); | ||
|
|
||
| // Use as much CPU as possible, so we round down the weight here | ||
| return Math.floor((percentValue / 100) * availableParallelism); |
There was a problem hiding this comment.
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.
| const projectConfiguration: RushProjectConfiguration | undefined = projectConfigurations.get(project); | ||
| const operationSettings: IOperationSettings | undefined = | ||
| operation.settings ?? projectConfiguration?.operationSettingsByOperationName.get(phase.name); |
There was a problem hiding this comment.
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.
| { | ||
| "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()`." |
There was a problem hiding this comment.
| "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 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. The maximum concurrency units is determined by the -p flag.", |
There was a problem hiding this comment.
| "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.", |
3 participants
Summary
Add issue #5607 proposal 1 feature.
Details
weightsettings inRushProjectConfiguration.operationSettingspercentage string to integer based onos.availableParallelism().IOperationSettings.weighttype definition.weightfield json schema.How it was tested
Impacted documentation
None