feat!: Implement step parameter space iteration for chunks #172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mwiebe
force-pushed
the
param-space-chunk-iter
branch
5 times, most recently
from
7ed8d49 to
c9a52e5
Compare
jusiskin
requested changes
Feb 21, 2025
Contributor
jusiskin
left a comment
jusiskin
left a comment
There was a problem hiding this comment.
Great job with this code change.
I haven't finished my review pass yet, but wanted to surface one thing we should add to help clarify an assumption here.
mwiebe
force-pushed
the
param-space-chunk-iter
branch
2 times, most recently
from
139032d to
beeaec2
Compare
leongdl
reviewed
Feb 22, 2025
leongdl
reviewed
Feb 22, 2025
leongdl
left a comment
leongdl
left a comment
There was a problem hiding this comment.
This will take me some time to understand all the logic.
leongdl
reviewed
Feb 25, 2025
| if chunks_default_task_count <= 0: | ||
| chunk_count = 1 | ||
| else: | ||
| chunk_count = (task_count + chunks_default_task_count - 1) // chunks_default_task_count |
There was a problem hiding this comment.
That's an interesting way to implement something almost like ceiling!
Contributor
Author
There was a problem hiding this comment.
If you messed around with graphics code like implementing https://en.wikipedia.org/wiki/Bresenham%27s_line_algorithm#Algorithm_for_integer_arithmetic and similar this stuff is pretty natural :)
mwiebe
force-pushed
the
param-space-chunk-iter
branch
2 times, most recently
from
acabd96 to
63fc330
Compare
leongdl
approved these changes
Feb 25, 2025
leongdl
left a comment
leongdl
left a comment
There was a problem hiding this comment.
I generally understand the logic, but probably need to do a few more thinking passes to walk through the logic.
mwiebe
force-pushed
the
param-space-chunk-iter
branch
from
63fc330 to
2011e71
Compare
jusiskin
approved these changes
Feb 25, 2025
mwiebe
force-pushed
the
param-space-chunk-iter
branch
from
2011e71 to
8e678bd
Compare
* Modify StepParameterSpaceIterator to handle parameter spaces that
include a CHUNK[INT] parameter. The iteration pattern depends on the
values in the 'chunks' object of the parameter definition.
* If targetRuntimeSeconds is 0 or unspecified, split the CHUNK[INT]
dimension into chunks, and then iterate over the whole space with
that selection of chunks.
* If targetRuntimeSeconds is a positive integer, move the CHUNK[INT]
dimension to the innermost loop, and dynamically select the chunk
each time it advances. Let the user of the iterator modify its
chunks_default_task_count to adapt the chunk size over time.
* The StepParameterSpaceIterator object behaved more like a
pseudo-container than an iterator. Change it to act as the iterator
itself (returning 'self' from __iter__() instead of a new object).
* Changed how the special case zero-dimensional iteration works to be
a Node/Iterator pair just like the others, for a more uniform
interface.
* Changes to IntRangeExpr:
* Fix a bug in IntRangeExpr.from_list that wasn't handling the end of a
linear sequence correctly.
* Change the string representation of IntRangeExpr to use "," instead of
"-" when there are just two values.
* Add and update unit tests for all of the modifications.
* Update parameter space validation to ensure only one CHUNK[INT]
parameter is defined.
BREAKING CHANGE: This change includes a few small changes to the public contract
of IntRangeExpr and the StepParameterSpaceIterator. Most code won't depend
on these specifics, but some could.
Signed-off-by: Mark Wiebe <[email protected]>
mwiebe
force-pushed
the
param-space-chunk-iter
branch
from
8e678bd to
f1bfdf3
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the problem/requirement? (What/Why)
To use task chunking as defined by https://github.com/OpenJobDescription/openjd-specifications/blob/mainline/rfcs/0001-task-chunking.md, clients of the library need to be able to iterate over a parameter space based on chunking.
What was the solution? (How)
What is the impact of this change?
Clients such as the openjd CLI command will be able to use chunked iteration of parameter spaces to run job templates that include chunking.
How was this change tested?
Implemented unit tests for the new chunked iteration modes.
Was this change documented?
Updated docstrings about chunked iteration.
Is this a breaking change?
This change includes a few small changes to the public contract of IntRangeExpr and the StepParameterSpaceIterator. Most code won't depend on these specifics, but some could.
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.