Repository navigation
rfc22: decode unordered input and resolve ambiguities in the text - #569
Merged
Merged
Conversation
|
|
grondo
approved these changes
Nov 6, 2026
grondo
left a comment
grondo
left a comment
Contributor
There was a problem hiding this comment.
Great! I had just one trivial suggestion.
Comment on lines
+52
to
+54
| A range SHALL consist of two ids separated by a hyphen, where the | ||
| first id is less than the second, e.g. ``1-3,5-6,42``. A range SHALL | ||
| represent the first id, the second id, and all ids between them. |
Contributor
There was a problem hiding this comment.
Suggestion: This paragraph describes "A range" so the example should include only a single range, e.g. 1-3. The current example shows a comma-separated list and includes a singleton.
Contributor
|
Queue requested — the merge queue status continues in this comment ↓. |
Problem: RFC 22 permits consecutive ids to be compressed into hyphenated ranges, but does not say what a range denotes or constrain its endpoints. It is unclear whether "3-0", "2-2", or "1-2-3" are valid, and what ids they represent. Define an idset string as a comma-delimited list of elements, each either a single id or a range. Define a range as two ids separated by a hyphen, the first less than the second, which represents both ids and all ids between them. Restate the ascending order requirement in terms of the ids the elements represent. Assisted-by: claude:opus-5.5
Problem: RFC 22 does not say how the empty idset is represented, yet implementations rely on decoding the empty string to the empty set, e.g. the flux-core shell decodes an unset GPU list as "". State that an idset string with no elements, such as the empty string or "[]", represents the empty idset. Assisted-by: claude:opus-5.5
Problem: RFC 22 allows one idset to be written many ways, e.g. "1-4", "1-2,3-4", "1,2-4", and "[1-4]", but names no preferred form and places no requirements on encoders or decoders separately. Idset strings cannot be compared as text, and encoders have no single output to agree on. Add a Syntax heading over the existing rules. Define a normalized idset string as one that represents each id exactly once in ascending order, with ranges, if used, covering each maximal run of consecutive ids. Define the canonical idset string as the normalized string that uses ranges and no brackets, so each idset has exactly one. Require encoders to produce a normalized string and recommend the canonical one, and require decoders to accept any idset string. Allow an implementation to limit the largest id it supports, treating strings with larger ids as invalid. Assisted-by: claude:opus-5.5
Problem: RFC 22 requires the ids in an idset string to be in ascending order, so a string like "6,4,2,0" is invalid even though it unambiguously names a set. MPICH's PMI-1.2 PMI_Barrier_group() emits rank lists in communicator order, which need not be ascending, so Flux cannot decode them as idset strings. Drop the ascending order requirement. Allow elements to appear in any order and the ids they represent to repeat or overlap, and define the idset an idset string represents as the union of its elements. Normalized and canonical idset strings remain ascending, so encoders are unaffected. Assisted-by: claude:opus-5.5
Problem: RFC 22 requires decoders to accept every idset string but says nothing about other strings. A decoder may silently accept malformed input such as "1,,2", so a typo can go unnoticed, and implementations may disagree about what such a string means. Require decoders to reject any string that is not an idset string. Assisted-by: claude:opus-5.5
Problem: RFC 22 has no test vectors, so implementations have no shared reference for which strings are valid, what idset each represents, or which strings are normalized. Add a table of idset strings with the canonical idset string of the idset each represents and whether each is normalized, and a table of strings that are not idset strings. Assisted-by: claude:opus-5.5
Member
Author
|
Thanks! Good catch. I pushed that change and will set MWP. |
Contributor
✅ Merged into
|
|
|
garlick
added a commit
to garlick/flux-core
that referenced
this pull request
Nov 7, 2026
Problem: the idset decoder rejects a string whose elements do not ascend, such as "6,4,2,0", even though it unambiguously names a set. RFC 22 (flux-framework/rfc#569) now allows elements in any order, including repeated and overlapping ids, and defines the idset as the union of the elements. Drop the ascending order check when adding or subtracting elements. Track the maximum id as a running maximum rather than the last element's upper bound, so idsets sized from a pre-pass are large enough. idset_decode_info() sums the sizes of the elements, which overcounts overlapping input. When the elements overlap or do not ascend, re-parse them into an array of ranges, sort it, and merge overlapping and abutting ranges to count exactly. The cost is proportional to the number of elements rather than the id space, so the documented promise that idset_decode_info() and idset_decode_empty() do not create an idset still holds. idset_decode_empty() needs no count at all, only whether any element was parsed. Update tests that expected unordered input to fail. Add tests for unordered and overlapping input. Fixes flux-framework#7888 Assisted-by: claude:opus-5.5
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem: a case came up where decoding an idset from a list of ids presented out of order would be useful
There's really no reason that
idset_decode()couldn't allow this, so relax the RFC so that it can. While there, tighten up some ambiguities in the spec and add test vectors.A few changes will be needed to the idset decoder in flux-core
maxidin the sizing pre-passidset_decode_info()countsOne user-visible change would be that command line idsets like
flux exec -r 2,1would no longer be rejected.