Skip to content

coll/csel: typed algorithm parameters in the selection JSON - #8038

Merged
hzhou merged 10 commits into
pmodels:mainfrom
hzhou-muse:csel-typed-params
Oct 20, 2026
Merged

hzhou merged 10 commits into
pmodels:mainfrom
hzhou-muse:csel-typed-params

Conversation

@hzhou-muse

@hzhou-muse hzhou-muse commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Pull Request Description

A collective selection JSON, the built-in one or a user's MPIR_CVAR_COLL_SELECTION_JSON_FILE, could be wrong without anyone noticing. Algorithm parameters were keys such as "k=2", read with atoi, so a typo silently became 0 and the algorithm ran with it. Thresholds were read the same way. A wrong parameter count, an unknown algorithm, collective, condition or subtree name, or a malformed key hit an assert in MPI_Init. A tuning file that is subtly wrong is worse than one that is rejected.

This PR makes the selection JSON checked input: every value has a type, and every error is reported with its line and column.

Parameters are now written as "name": value, e.g. "algorithm=MPIR_Bcast_intra_tree": {"tree_type": "kary", "k": 4}. Each parameter's type comes from the CVAR in its cvar_params, so the JSON and the CVARs can't drift apart:

CVAR type JSON value
int an integer
boolean true / false
enum one of the CVAR's value names, as a string

The old "k=2" form is no longer accepted. coll_selection.json had one parameter, which is converted.

  • Tree types: the *_TREE_TYPE CVARs for bcast, ibcast, allreduce, iallreduce and ireduce become enum CVARs, with values checked at compile time against MPIR_Tree_type_t. This fixes the topology tree types, which silently ran as kary, and removes a duplicate MPIR_CVAR_BCAST_TREE_TYPE declaration.
  • JSON parser: it accepts integers, true and false, records each key's position, and provides typed getters.
  • Generated code: gen_coll.py emits type-checked parameter parsing. For conditions it emits only a table; the parsing code lives in csel.c.
  • Errors, not asserts: unknown names, missing, unknown or mistyped parameters, and malformed thresholds are all reported as errors. Thresholds must be integers that fit in an int.
  • Structural checks: an algorithm= key must be the only key in its object (before, it leaked the conditions parsed before it and ignored the keys after it), a subtree object must not be empty (before, it asserted at run time), and a subtree name must fit in CSEL_MAX_NAME (before, it was copied without its terminating NUL). ! together with <, <=, > or >= (e.g. !comm_size>8) is rejected as ambiguous; !comm_size(8) still works.
  • Every case covered: every object must end with an "any" key, and a new "error" leaf (e.g. "any": "error" after the built-in collective= list) ends a search on purpose. A search that loops through call= is stopped at run time. Before, a search that matched nothing asserted at run time, and a call= loop hung the collective. Now an "error" leaf or a loop makes the collective return "Collective selection failed to find an algorithm".
  • Docs: Collective_Selection.md and cvar.md are updated.

Author Checklist

  • Provide Description
  • Commits Follow Good Practice
  • Passes All Tests
  • Contribution Agreement

🤖 Generated with Claude Code

@hzhou-muse
hzhou-muse force-pushed the csel-typed-params branch 4 times, most recently from 345400a to 520fd7b Compare October 19, 2026 15:44
hzhou-muse and others added 5 commits October 19, 2026 15:52
The BCAST, IBCAST, ALLREDUCE, IALLREDUCE and IREDUCE TREE_TYPE CVARs are
now enum CVARs, so an unknown tree type is reported when the CVAR is read.
Their values follow the MPIR_Tree_type_t order, checked at compile time,
so they are passed to the algorithms directly. The duplicate declaration
of MPIR_CVAR_BCAST_TREE_TYPE is removed.

Previously they were strings converted by get_tree_type_from_string, which
mapped unknown names, including the topology tree types, to kary.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The built-in JSON parser now accepts integers, true, and false as values.
Each node records the line and column of its key, and holds its value in
a union selected by its type. MPII_Csel_json_get_int, _get_bool, and
_get_enum read a typed value and report a mismatch at that position.

Previously a value could only be an object or a string.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
An algorithm's parameters in the selection JSON are now written as
"name": value, e.g. {"k": 2}. gen_coll.py takes each parameter's type from
its CVAR: an integer for int, true or false for boolean, and one of the
CVAR's value names for enum, such as "tree_type": "knomial_1" or
"ccl": "nccl". A missing, unknown, or mistyped parameter is reported with
its line and column instead of asserting.

Previously parameters were keys such as "k=2" with an empty object as the
value, read with atoi, and the ccl parameter took a raw enum number.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The condition keys in the selection JSON, e.g. "pof2" or "comm_size<8", are
parsed by static functions in csel.c that walk MPIR_Csel_condition_names.
gen_coll.py only emits the condition table, now with
MPIR_Csel_condition_has_thresh to mark the conditions that take a threshold.

Previously gen_coll.py generated MPII_Csel_parse_operator and its threshold
helper as code.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
An unknown subtree, collective, algorithm, or condition key, a malformed
threshold, or top-level keys that mix "name=" with other keys now return an
error with the line and column of the key. Thresholds must be integers that
fit in an int, and a "(N)" threshold needs its closing parenthesis. A leading
"!" is rejected with <, <=, >, and >=, since "!comm_size>8" is ambiguous to
read; "!comm_size(8)" is still accepted.

Previously these cases hit an assert, a malformed threshold was read with
atoi, and the ">" in "!comm_size>8" overrode the "!".

Co-Authored-By: Claude Opus 5.5 <[email protected]>
hzhou-muse and others added 5 commits October 19, 2026 16:05
An "algorithm=" key must be the only key in its object, and a subtree name
must be shorter than CSEL_MAX_NAME; otherwise loading the selection JSON
returns an error with the line and column of the key.

Previously an "algorithm=" key ended its object: condition nodes parsed
before it were leaked and keys after it were ignored. A long subtree name
was copied without its terminating NUL.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A subtree in the selection JSON must have at least one key; an empty object
returns an error with its line and column.

Previously an empty object made a condition with no subtree, which asserted
in MPIR_Csel_search when a collective reached it, or asserted at load time
for an empty named subtree.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A string value "error" in place of a subtree ends the search without an
algorithm, so the collective returns "Collective selection failed to
find an algorithm". It lets a list of cases say that no other case is
expected, e.g. "any": "error" after the collective= list.

Previously a string value had to be "call=<name>".

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Every object other than an algorithm= leaf must end with an "any" key,
and no key may follow it. A search then always ends at an algorithm or
an "error" leaf. The built-in JSON's collective= list ends with
"any": "error", and four builtin_op/!builtin_op pairs end with "any".

Previously a search that matched no case asserted in MPIR_Csel_search,
and keys after "any" were silently unreachable.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A search uses one coll_sig, so it follows the same path each time it
enters a subtree. MPIR_Csel_search now counts call= jumps and returns
no algorithm once it has made more jumps than there are subtrees; the
collective returns "Collective selection failed to find an algorithm".
A call= cycle that conditions never take, e.g. through a collective=
branch for another collective, is still accepted.

Previously such a loop recursed forever and hung the collective.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@hzhou-muse

hzhou-muse commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

test:mpich/ch3/tcp ✔️

@hzhou
hzhou merged commit cd5395e into pmodels:main Oct 20, 2026
8 checks passed
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.

2 participants