Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Assume task parameters with underscores to be strings. #6571

Open
wants to merge 4 commits into
base: 8.4.x
Choose a base branch
from
Open

Assume task parameters with underscores to be strings. #6571

wants to merge 4 commits into from

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jan 23, 2025 •
edited
Loading

Summary

PEP-515 allowed integers to be defined with underscores from python 3.6.
This changes the behaviour of Cylc task parameters, as "123_456" can now be interpreted as an integer.

n.b. I've tried to get 100% coverage on the method under test.

I'm going to have a quick play around with the code to ensure that we've not missed anything else about this change that needs addressing.

Bug reported on internal VivaEngage

(how to replicate)

# flow.cylc
[task parameters]
    chunks = 084_132

[scheduler]
    allow implicit tasks = True

[scheduling]
    initial cycle point = now
    [[graph]]
        R1 = f<chunks>
$ cylc graph <sourcedir> --ref
graph
node "20250123T1026Z/f_chunks84132" "f_chunks84132\n20250123T1026Z"
stop

Where one would expect

- node "20250123T1026Z/f_chunks84132" "f_chunks84132\n20250123T1026Z"
+ node "20250123T1026Z/f_chunks_084_132" "f_chunks_084_132\n20250123T1026Z"

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.
  • Double checked PEP-515
  • Used parameter generation to check the method.

@wxtim wxtim self-assigned this Jan 23, 2025
oliver-sanders
oliver-sanders reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM.

This could do with a changelog. We might want to put it on the 8.4.x branch as it's more of a fix than a feature.

cylc/flow/parsec/validate.py Outdated Show resolved Hide resolved
@wxtim wxtim changed the base branch from master to 8.4.x January 23, 2025 10:28
@wxtim wxtim added this to the 8.4.1 milestone Jan 23, 2025
@wxtim wxtim added bug Something is wrong :( small labels Jan 23, 2025
wxtim
wxtim commented Jan 23, 2025
View reviewed changes
cylc/flow/parsec/validate.py
>>> CylcConfigValidator.coerce_parameter_list('084_132', None)
['084_132']

>>> CylcConfigValidator.coerce_parameter_list('072, a', None)
Copy link
Member Author

Choose a reason for hiding this comment

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

This Fixes a coverage hole.

wxtim
wxtim commented Jan 23, 2025
View reviewed changes
tests/unit/parsec/test_validate.py
@@ -699,7 +699,7 @@ def test_coerce_parameter_list():
('-15, -10, -5, -1..1', [-15, -10, -5, -1, 0, 1])]:
assert validator.coerce_parameter_list(value, ['whatever']) == result
# The bad
for value in ['foo/bar', 'p1, 1..10', '2..3, 4, p']:
for value in ['foo/bar', 'p1, 1..10', '2..3, 4, p', 'x:,']:
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix a coverage hole.

@wxtim wxtim requested a review from oliver-sanders January 23, 2025 12:42
@wxtim
Assume task parameters with underscores to be strings.
fec1f3c 
PEP-515 allowed integers to be defined with underscores from python 3.6.
This changes the behaviour of Cylc task parameters, as
`"123_456"` can now be interpreted as an integer.
@wxtim wxtim force-pushed the fix.underscores_in_integers branch from a18e896 to fec1f3c Compare January 23, 2025 14:48
MetRonnie
MetRonnie reviewed Jan 23, 2025
View reviewed changes
changes.d/6571.fix.md Outdated Show resolved Hide resolved
wxtim and others added 2 commits January 24, 2025 09:17
MetRonnie
MetRonnie approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

LGTM, tested, got 1 suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
bug Something is wrong :( small
Projects
None yet
Milestone
8.4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@wxtim @oliver-sanders @MetRonnie