Match parameter tokens exactly in get_used_parameters by arpitjain099 · Pull Request #478 · llnl/maestrowf

arpitjain099 · 2026-07-09T18:17:20Z

_get_used_parameters builds its search pattern as:

_ = r"\{}\({}\.*\w*\)".format(self.token, key)

Here \.* is "zero or more literal dots" and \w* is "zero or more word characters", so after the parameter name it happily consumes more characters. For a parameter named N the pattern matches $(NP), so a step whose command uses only $(NP) reports both N and NP as used:

get_used_parameters(step using only "$(NP)")  ->  {"N", "NP"}   # should be {"NP"}

get_used_parameters feeds Study.get_tasks_per_step, which uses the result to decide whether a step is expanded across parameter combinations and to build the combination workspace/label names. So a prefix collision (common in HPC specs: N/NP, X/X2, SIZE/SIZE_B) can attach the wrong parameters to a step, and a step whose command literally contains a $(N...)-shaped token can be expanded into extra instances.

Fix: require the character after the key to be ) or .<suffix>, and re.escape the key:

_ = r"\{}\({}(\.\w+)?\)".format(self.token, re.escape(key))

$(key), $(key.label), and $(key.name) still match; $(NP) no longer matches key N.

Added tests/test_used_parameters.py. I verified the fix directly under Python 3.13 ($(NP)-only step yields {"NP"}; the exact/.label/.name forms still resolve; the old pattern returns {"N","NP"}). Note: the full pytest suite does not collect on my machine because the repo's conftest.py applies a mark to a fixture, which pytest 8 rejects, unrelated to this change.

_get_used_parameters built the search pattern as r"\{}${}\.*\w*$".format(token, key), where \.* is zero-or-more literal dots and \w* is zero-or-more word characters. For a parameter named N that regex also matches $(NP), so a step whose command uses only $(NP) reports both N and NP as used. get_used_parameters feeds Study.get_tasks_per_step, which uses the result to decide whether a step is expanded across parameter combinations and to build the combination workspace/label. So a prefix collision (common in HPC specs: N vs NP, X vs X2) can attach the wrong parameters to a step or, when a command literally contains a $(N...)-shaped token, expand a step into extra instances. Require the character after the key to be ")" or ".<suffix>", and re.escape the key. Exact, $(key.label), and $(key.name) forms still match. Added a regression test. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

jwhite242 · 2026-07-21T16:19:29Z

Well, that is a great catch; am surprised this hasn't blown up on anybody before!

jwhite242 · 2026-07-21T16:24:39Z

@arpitjain099

Note: the full pytest suite does not collect on my machine because the repo's conftest.py applies a mark to a fixture, which pytest 8 rejects, unrelated to this change

Just to be clear, are you seeing it miss tests that should be able to run on your machine? There are several markers for the tests that require live schedulers (slurm, flux, ...) which skip things on purpose since they don't make sense if you're not on a machine with a scheduler; if you're seeing others getting skipped let us know.

jwhite242

Looks pretty good, only a few other minor things needed:

tick the dev version in the pyproject.toml (sorry, don't have any ci/bot helpers to smooth this one out yet)
add a changelog fragment as detailed here: https://github.com/llnl/maestrowf/blob/develop/CONTRIBUTING.md

jwhite242 · 2026-07-21T16:26:03Z

        elif isinstance(item, str):
            for key in self.parameters.keys():
-                _ = r"\{}\({}\.*\w*\)".format(self.token, key)
+                _ = r"\{}\({}(\.\w+)?\)".format(self.token, re.escape(key))


Thinking this 're.escape' should probably get added to the variables and such as well (though I won't push for it here), so thanks for pointing that one out

Makes sense. I'll cover the variables and the other token expansions in the separate PR I mentioned, since they use the same \{}${}\...$ construction and would trip on the same regex metacharacters. I'll tag you on it.

Quick follow-up before I opened that PR. I went through the rest of the token handling and it turns out _get_used_parameters (what this PR fixes) is the only place that compiles a parameter name into a regex. The actual substitution of parameters, labels, names, and variables all runs through str.replace() (Combination.substitute in parameters.py, Variable.substitute in variable.py), which is a literal match, so a regex metacharacter in a name has no effect there and there's nothing to escape. The other regexes in the tree (WSREGEX, ALL_COMBOS, the \w+ validators) are static, with the name as the subject rather than part of the pattern.

So I don't think a separate PR is needed after all, and I was wrong earlier to say the same construction showed up in the other expansions. Happy to be corrected if I've missed a spot.

Sounds good, and thanks for digging into that!

…-collision

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

arpitjain099 · 2026-07-21T17:57:44Z

Both done in e38ead4.

Version ticked to 1.2.1dev3. I merged develop in first since the branch was a commit behind and still had dev1, so it now moves cleanly from the current dev2.

Changelog fragment added at changelog.d/20260721_190000_arpitjain099_used_params_prefix_collision.md under Fixed, following the format in the existing fragments.

On the re.escape point, agreed, and thanks for taking a look. The same \{}${}\...$ construction shows up for the other token expansions too, so a parameter name with a regex metacharacter would behave the same way there. Happy to open a separate PR for those if you want them covered, since as you say it is outside what this one is fixing.

One note on verification: I could not run the suite locally, maestrowf is not installed in my environment so conftest.py fails at import on version(__package__). The change here is a version string and a new fragment file, so CI should be the real check.

jwhite242 · 2026-07-21T19:02:22Z

On the re.escape point, agreed, and thanks for taking a look. The same \{}${}\...$ construction shows up for the other token expansions too, so a parameter name with a regex metacharacter would behave the same way there. Happy to open a separate PR for those if you want them covered, since as you say it is outside what this one is fixing.

Sure, if you want to I'll be happy to review it!

One note on verification: I could not run the suite locally, maestrowf is not installed in my environment so conftest.py fails at import on version(__package__). The change here is a version string and a new fragment file, so CI should be the real check.

oh, that's interesting; skip the poetry install part maybe (i.e. running poetry install from within the repo/clone while the virtualenv is active; think you can use poetry to manage virtualenvs but i usually use conda or virtualenvwrapper in my own workflows)? that should get you everything and it does an editable install into the virtualenv (also grabs the scriv tool that can generate those changelog fragments (forget if you can even see those templates without running scriv)

arpitjain099 · 2026-07-21T19:10:03Z

Thanks, that explains the failure. I was running the tests without the editable install, so conftest.py blew up on version(__package__). I'll set the virtualenv up with poetry install from the clone so scriv comes along with it and I can regenerate the fragment properly.

And yeah, I'll open the separate PR for the other token expansions. It's the same \{}${}\...$ shape there, so a parameter name with a regex metacharacter would trip them the same way this one did. I'll wrap the names in re.escape and tag you on it.

jwhite242 self-requested a review July 21, 2026 16:19

jwhite242 reviewed Jul 21, 2026

View reviewed changes

arpitjain099 added 2 commits July 22, 2026 02:56

Merge remote-tracking branch 'up/develop' into fix/used-params-prefix…

54eac04

…-collision

Tick dev version and add a changelog fragment

e38ead4

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

jwhite242 merged commit d75efcf into llnl:develop Jul 29, 2026
14 checks passed

Navigation Menu

Uh oh!

Match parameter tokens exactly in get_used_parameters - #478

Match parameter tokens exactly in get_used_parameters#478
jwhite242 merged 3 commits into
llnl:developfrom
arpitjain099:fix/used-params-prefix-collision

arpitjain099 commented Jul 9, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

jwhite242 left a comment

Uh oh!

jwhite242 Jul 21, 2026

Uh oh!

arpitjain099 Jul 22, 2026

Uh oh!

arpitjain099 Jul 22, 2026

Uh oh!

jwhite242 Jul 29, 2026

Uh oh!

arpitjain099 commented Jul 21, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

arpitjain099 commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

arpitjain099 commented Jul 9, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

jwhite242 left a comment

Choose a reason for hiding this comment

Uh oh!

jwhite242 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

arpitjain099 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

arpitjain099 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

jwhite242 Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

arpitjain099 commented Jul 21, 2026

Uh oh!

jwhite242 commented Jul 21, 2026

Uh oh!

arpitjain099 commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants