Skip to content

Conversation

@mwiebe
Copy link
Contributor

@mwiebe mwiebe commented Feb 28, 2025

What was the problem/requirement? (What/Why)

The OpenJD CLI lacks a way to validate whether a provided set of parameters are in the parameter space of the step they're for. When using chunks, adding this validation would make it easier to catch errors about whether the chunked parameter is contiguous and needs an interval like "3-3" vs a single value like "3".

With chunking, the step parameter space iterator doesn't have a mechanism, other than editing the input space object, to iterate over each individual task instead of over chunks of tasks. A mechanism like this is useful for scheduler integrations that reason at the task level and do their own chunking.

What was the solution? (How)

  • The in operator and validate_containment function of the StepParameterSpaceIterator will allow the openjd-cli to validate that input task parameters are within the parameter space of steps that it is running. The latter
    • The purpose of this change is to support using chunked iteration from the OpenJD CLI. This validation helps ensure the openjd run command only accepts correct chunk, in addition to adding validation that was marked as a TODO in the openjd-cli codebase. Having validate_containment raise an exception with a message about why the task parameters are not in the parameter space results in better error messages.
  • The chunksize override lets a caller iterate over every task of a parameter space regardless of the space's chunk size or adaptivity. This is useful for callers to evaluate the whole task space so that they can perform their own chunking logic later.
  • Add a property chunks_parameter_name to the StepParameterSpaceIterator so that code using it can easily access the chunked parameter and its properties without having to perform its own loop over all of them.
  • Use an IntRangeExpr instead of a string to hold the range expression of a RangeExpressionTaskParameterDefinition, the class used to instantiate a parameter definition template into a job.
  • Found that negative steps in IntRange have bugs in corner cases. Code that is processing the list of IntRanges includes assumptions that the step is positive. To fix this, chose to normalize the step to be positive instead of adjusting the code to handle persistence of negative steps, as that is much simpler to get right.
    • Note that the IntRangeExpr was already not preserving the input order of a range expression, because it sorted all the components.
  • Removed _BaseMessageError and used ValueError instead as a base class for the exceptions. This reduces custom code and interoperates with Pydantic better.

What is the impact of this change?

With a follow-up OpenJD CLI change, more pleasant CLI usage.

How was this change tested?

Added unit tests for the new functions and operator.

Was this change documented?

Updated docstrings

Is this a breaking change?

BREAKING CHANGE: The IntRangeExpr class now normalizes the steps of
individual range components like "3-1:-2" to be positive like "1-3:2",
so anything depending on the prior behavior needs to be updated.

Does this change impact security?

  • Does the change need to be threat modeled? For example, does it create or modify files/directories that must only be readable by the process owner?
    • If so, then please label this pull request with the "security" label. We'll work with you to analyze the threats.

No


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mwiebe mwiebe requested a review from a team as a code owner February 28, 2025 17:19
Copy link
Contributor

@epmog epmog left a comment

Choose a reason for hiding this comment

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

Looks good! Just the few comments about errors and the changing type

@mwiebe mwiebe force-pushed the param-space-contains branch from 840f2c1 to df7fc05 Compare February 28, 2025 23:06
epmog
epmog previously approved these changes Mar 1, 2025
epmog
epmog previously approved these changes Mar 1, 2025
…rSpaceIterator

* The in operator and validate_containment function of the StepParameterSpaceIterator
  will allow the openjd-cli to validate that input task parameters
  are within the parameter space of steps that it is running. The latter
   * The purpose of this change is to support chunked iteration. This
     validation helps ensure the `openjd run` command only accepts
     correct chunk, in addition to adding validation that was marked as
     a TODO in the openjd-cli codebase. Having validate_containment
     raise an exception with a message about why the task parameters are
     not in the parameter space results in better error messages.
* The chunksize override lets a caller iterate over every task of a
  parameter space regardless of the space's chunk size or adaptivity.
  This is useful for callers to evaluate the whole task space so that
  they can perform their own chunking logic later.
* Add a property chunks_parameter_name to the StepParameterSpaceIterator
  so that code using it can easily access the chunked parameter and its
  properties without having to perform its own loop over all of them.
* Use an IntRangeExpr instead of a string to hold the range expression
  of a RangeExpressionTaskParameterDefinition, the class used to
  instantiate a parameter definition template into a job.
* Found that negative steps in IntRange have bugs in corner cases. Code
  that is processing the list of IntRanges includes assumptions that the
  step is positive. To fix this, chose to normalize the step to be
  positive instead of adjusting the code to handle persistence of
  negative steps, as that is much simpler to get right.
   * Note that the IntRangeExpr was already not preserving the input
     order of a range expression, because it sorted all the components.
* Removed _BaseMessageError and used ValueError instead as a base class
  for the exceptions. This reduces custom code and interoperates with
  Pydantic better.

BREAKING CHANGE: The IntRangeExpr class now normalizes the steps of
    individual range components like "3-1:-2" to be positive like "1-3:2",
    so anything depending on the prior behavior needs to be updated.

Signed-off-by: Mark Wiebe <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2025

@mwiebe mwiebe merged commit b33c6cf into OpenJobDescription:mainline Mar 3, 2025
19 checks passed
@mwiebe mwiebe deleted the param-space-contains branch March 3, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants