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

Fix ICP bug. #4227

Merged
merged 8 commits into from
May 27, 2021
Merged

Fix ICP bug. #4227

merged 8 commits into from
May 27, 2021

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented May 25, 2021 •
edited
Loading

These changes close #4225

$ cylc play --no-detach five --initial-cycle-point="2^2"

                Cylc Workflow Engine 8.0b2.dev
                Copyright (C) 2008-2021 NIWA
                & British Crown (Met Office) & Contributors

2021-05-25T13:15:56+12:00 ERROR - Workflow shutting down - PointParsingError: Incompatible value for
        <class 'cylc.flow.cycling.integer.IntegerPoint'>: 2^2: invalid literal for int() with base 10: '2^2'
2021-05-25T13:15:56+12:00 INFO - DONE

Requirements 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).
  • No dependency changes.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver added the bug Something is wrong :( label May 25, 2021
@hjoliver hjoliver added this to the cylc-8.0b2 milestone May 25, 2021
@hjoliver hjoliver self-assigned this May 25, 2021
@hjoliver hjoliver requested a review from kinow May 25, 2021 01:17
kinow
kinow reviewed May 25, 2021
View reviewed changes
cylc/flow/config.py
if orig_icp is None:
if self.cfg['scheduling']['cycling mode'] == INTEGER_CYCLING_TYPE:
orig_icp = '1'
if self.cycling_type == INTEGER_CYCLING_TYPE:
Copy link
Member

Choose a reason for hiding this comment

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

@hjoliver I think I saw cycling_type in the debugger, and it was None? But there was a cycling mode in the cfg['scheduling'] that had the integer value. Not sure if that's helpful/important for this issue 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

self.cycling_type is initialized in WorkflowConfig.__init__(). It shouldn't be None, but I'll take another look...

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately self.cfg['scheduling']['cycling mode'] was being used incorrectly here. In the integer case it is equal to INTEGER_CYCLING_TYPE but in the date-time case it is equal to the calendar mode, not to ISO8601_CYCLING_TYPE.

@hjoliver
Copy link
Member Author

Unit tests added.

@hjoliver hjoliver requested a review from MetRonnie May 25, 2021 02:02
kinow
kinow approved these changes May 25, 2021
View reviewed changes
@hjoliver
Copy link
Member Author

(Pushed small tweak with skip-ci; all tests passed on previous commit, except patch coverage 83%)

MetRonnie
MetRonnie reviewed May 25, 2021
View reviewed changes
tests/unit/conftest.py Outdated
def cycling_type(monkeypatch):
"""Set the Cylc cycling type and return its value."""
def _cycling_type(integer=True):
ctype = INTEGER_CYCLING_TYPE if integer else ISO8601_CYCLING_TYPE
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't realised the difference between cycling type and mode. If I understand correctly:

  • Type is either integer or iso8601
  • Mode is either integer, gregorian, 360day, 365day etc

Btw, just a heads up this is going to conflict with my PR #4213 - https://github.com/cylc/cylc-flow/pull/4213/files#diff-0cae8a7ee2d37098b1ad84b543d17cfc1e8535eed5fd6abac88c668bfe354cbb

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct, I had forgotten the distinction myself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, just a heads up this is going to conflict with my PR

And unfortunately (for me!) I just merged that PR 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Deconflicted. And I've attempted to straighten out the cycling type vs mode stuff in the process.

MetRonnie
MetRonnie reviewed May 26, 2021
View reviewed changes
tests/unit/conftest.py
Comment on lines +66 to +72
def _cycling_type(
mode: str = 'integer', time_zone: Optional[str] = None
) -> str:
if mode == 'integer':
ctype = INTEGER_CYCLING_TYPE
else:
ctype = ISO8601_CYCLING_TYPE
Copy link
Member

Choose a reason for hiding this comment

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

In a parametrized test, I think it's clearer to specify a parameter as cycling_type = 'integer' vs 'iso8601' rather than 'integer' vs ''

Suggested change
def _cycling_type(
mode: str = 'integer', time_zone: Optional[str] = None
) -> str:
if mode == 'integer':
ctype = INTEGER_CYCLING_TYPE
else:
ctype = ISO8601_CYCLING_TYPE
def _cycling_type(
ctype: str = INTEGER_CYCLING_TYPE, time_zone: Optional[str] = None
) -> str:

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but I was trying to maintain the distinction between the cycling mode user config item, and cycling type, which is internal. And your parameterized tests mostly do this via a mocked config dict, so they should really specify cycling mode, not type ... and then this fixture sets the internal type according to the mode. Finally, the gregorian cycling mode is the default, so I set those values to empty string rather than hard-wiring another magic value.

So how about we leave this PR as-is, and as a low priority follow-up we could refactor the tests to specify cycling type directly, but separately from the mocked config?

MetRonnie
MetRonnie approved these changes May 27, 2021
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.

I have deconflicted the changelog

@MetRonnie MetRonnie merged commit 005c467 into cylc:master May 27, 2021
@hjoliver hjoliver deleted the fix-icp-bug branch May 27, 2021 10:57
@MetRonnie MetRonnie mentioned this pull request Jun 3, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinow kinow kinow approved these changes

@MetRonnie MetRonnie MetRonnie approved these changes

Assignees

@hjoliver hjoliver

Labels
bug Something is wrong :(
Projects
None yet
Milestone
cylc-8.0b2
Development

Successfully merging this pull request may close these issues.

config.py & iso8601.py try to access the isodatetime parser even if cycling mode is integer
3 participants
@hjoliver @kinow @MetRonnie