Tidy & add type annotations #4213

MetRonnie · 2021-05-14T10:46:55Z

This is a small change with no associated Issue.

While tackling #3743, I started adding type annotations to help me understand the existing code. Then somehow ended up tidying some of the methods in the cyclers classes

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).
Applied any dependency changes to both setup.py and
conda-environment.yml.
Already covered by existing tests.
No change log entry required (invisible to users).
No documentation update required.

MetRonnie · 2021-05-14T10:52:42Z

cylc/flow/cycling/__init__.py

+    def __hash__(self) -> int:
+        return hash(self.value)


I don't think __hash__ was being implemented in IntegerInterval or ISO8601Interval before this, so two ostensibly equal intervals would have had different hashes (i.e. this change replaces per-instance hashes with per-value hashes)

MetRonnie · 2021-05-14T10:54:08Z

cylc/flow/cycling/loader.py

+@overload
+def get_point(value: str, cycling_type: Optional[str] = None) -> PointBase: ...
+
+
+@overload
+def get_point(value: None, cycling_type: Optional[str] = None) -> None: ...
+
+
 def get_point(
-    value: str, cycling_type: Optional[str] = None
+    value: Optional[str], cycling_type: Optional[str] = None
 ) -> Optional[PointBase]:


This is how you type annotate a function whose return value depends on an input arg

kinow

pycodestyle not happy?

./cylc/flow/cycling/loader.py:67:1: E704 multiple statements on one line (def)
./cylc/flow/cycling/loader.py:71:1: E704 multiple statements on one line (def)
./cylc/flow/cycling/loader.py:146:1: E704 multiple statements on one line (def)
./cylc/flow/cycling/loader.py:152:1: E704 multiple statements on one line (def)

MetRonnie · 2021-05-14T12:00:51Z

https://www.flake8rules.com/rules/E704.html

I'm proposing we add this rule to the ignored list. What do you think @hjoliver ?

Also, we're running pycodestyle and flake8 for some reason - we only need flake8 as it includes pycodestyle

hjoliver · 2021-05-17T00:54:39Z

Also, we're running pycodestyle and flake8 for some reason - we only need flake8 as it includes pycodestyle

#3999 (comment)

(Not merged yet)

cylc/flow/cycling/loader.py

hjoliver · 2021-05-17T01:03:46Z

https://www.flake8rules.com/rules/E704.html

I'm proposing we add this rule to the ignored list. What do you think @hjoliver ?

It's generally a sensible rule, so I'd rather not ignore it. The code looks OK as suggested in my previous comment, no?

MetRonnie · 2021-05-17T09:09:04Z

Ok, if you prefer

MetRonnie · 2021-05-17T09:38:35Z

It would be good to remove pycodestyle sooner, as at the moment, flake8 is ignoring more rules than pycodestyle alone

hjoliver · 2021-05-17T12:59:44Z

It would be good to remove pycodestyle sooner, as at the moment, flake8 is ignoring more rules than pycodestyle alone

OK, feel free to remove it here 👍

MetRonnie · 2021-05-17T14:44:11Z

Waiting on #4217 which fixes the manylinux tests

MetRonnie · 2021-05-19T16:23:39Z

cylc/flow/cycling/__init__.py

+    @property
    @abstractmethod
-    def TYPE(self):
-        return self._TYPE
+    def TYPE(self) -> str:
+        """Cycling type of this point."""


This is how you do an abstract property in Python 3.3+
https://stackoverflow.com/a/48710068/3217306

MetRonnie · 2021-05-19T16:25:00Z

cylc/flow/cycling/integer.py

-CYCLER_TYPE_SORT_KEY_INTEGER = "a"
+CYCLER_TYPE_SORT_KEY_INTEGER = 0


Sorting by integer seems more natural than by string. Not that I particularly understand why we would ever compare Integer and ISO8601 cycling things with each other

We could conceivably allow other cycling types in the future (e.g. to cycle over a list of named datasets). They could all be represented as strings, so long as integer cycles are also represented as strings (but compared as integers). The Point and Interval base class docstrings in cylc/flow/cycling/__init__.py say that derived classes should be "string based".

However, (as your comment refers to?) this TYPE_SORT_KEY seems to be used as a proxy for instance comparison when comparing points of different cycling types ... and I don't understand why we'd want to do that either. But if we did do it, this change would break the comp function? [update: I see you changed the ISO8601 type_sort_key to an int too ... now I'm even more confused!] Any thoughts @oliver-sanders ? (git blame points to Ben on this, but I think it predates your involvement...).

Yes, I changed both integer's & iso8601's TYPE_SORT_KEYs to ints. This is how they are used (on master)

cylc-flow/cylc/flow/cycling/__init__.py

Lines 110 to 118 in 87e22ab

def __cmp__(self, other):

# Compare to other point.

if other is None:

return -1

if self.TYPE != other.TYPE:

return cmp(self.TYPE_SORT_KEY, other.TYPE_SORT_KEY)

if self.value == other.value:

return 0

return self.cmp_(other)

MetRonnie · 2021-05-19T17:16:07Z

Hmm, MacOS functional test failing; looks like it's happened on a previous run too. @oliver-sanders can I ask you to check this branch out and run tests/f/restart/28-execution-timeout.t?

oliver-sanders · 2021-05-20T09:39:37Z

I can't replicate it, however, the actions artefact captures the issue, the subprocess pool encountered a PermissionError whilst trying to kill a process group. On MacOS this can happen for zombie processes, MacOS fails loudly, I think Linux might fail silently masking the bug? Running a test.

MetRonnie · 2021-05-20T16:52:16Z

MacOS failure seems unrelated, marking as ready for review

hjoliver · 2021-05-23T22:44:49Z

.github/workflows/test_fast.yml

@@ -71,7 +71,6 @@ jobs:

      - name: style
        run: |
-          pycodestyle


cylc/flow/config.py

kinow

Looks good to me. Used PyCharm integration to run MyPy, it found 23 issues, but none related to the changes here (and could well be false-positives). 👍

cylc/flow/workflow_db_mgr.py

hjoliver

👍

MetRonnie added this to the cylc-8.0.0 milestone May 14, 2021

MetRonnie requested review from kinow and wxtim May 14, 2021 10:46

MetRonnie self-assigned this May 14, 2021

MetRonnie commented May 14, 2021

View reviewed changes

kinow reviewed May 14, 2021

View reviewed changes

MetRonnie marked this pull request as draft May 14, 2021 12:00

hjoliver reviewed May 17, 2021

View reviewed changes

cylc/flow/cycling/loader.py Outdated Show resolved Hide resolved

MetRonnie force-pushed the dev branch from c77805f to 47dd3e3 Compare May 17, 2021 09:29

MetRonnie force-pushed the dev branch from 47dd3e3 to b2da419 Compare May 17, 2021 09:38

MetRonnie closed this May 18, 2021

MetRonnie reopened this May 18, 2021

MetRonnie added 7 commits May 19, 2021 17:07

Type annotations + tidy

546ae6a

Refactor test

22c5952

Fix hashing of IntervalBase

74d7d4b

Remove pycodestyle

d1d7aef

Slight refactor of cycling type stuff

f77c36c

Type annotations for WorkflowDatabaseManager

7c5c002

Show mypy error codes by default

0517e0e

MetRonnie force-pushed the dev branch from b2da419 to 0517e0e Compare May 19, 2021 16:19

MetRonnie commented May 19, 2021

View reviewed changes

oliver-sanders mentioned this pull request May 20, 2021

subprocpool: handle PermissionError on killpg #4221

Merged

7 tasks

MetRonnie marked this pull request as ready for review May 20, 2021 16:51

MetRonnie requested a review from kinow May 21, 2021 14:36

hjoliver reviewed May 23, 2021

View reviewed changes

Respond to code review

6f2e7a7

kinow approved these changes May 25, 2021

View reviewed changes

cylc/flow/workflow_db_mgr.py Show resolved Hide resolved

MetRonnie mentioned this pull request May 25, 2021

Fix ICP bug. #4227

Merged

7 tasks

hjoliver approved these changes May 26, 2021

View reviewed changes

hjoliver merged commit 6d2d55c into cylc:master May 26, 2021

MetRonnie deleted the dev branch May 26, 2021 08:33

dpmatthews modified the milestones: cylc-8.0.0, cylc-8.0b2 Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy & add type annotations #4213

Tidy & add type annotations #4213

MetRonnie commented May 14, 2021

MetRonnie May 14, 2021 •

edited

Loading

MetRonnie May 14, 2021 •

edited

Loading

kinow left a comment

MetRonnie commented May 14, 2021 •

edited

Loading

hjoliver commented May 17, 2021

hjoliver commented May 17, 2021

MetRonnie commented May 17, 2021

MetRonnie commented May 17, 2021

hjoliver commented May 17, 2021

MetRonnie commented May 17, 2021

MetRonnie May 19, 2021

MetRonnie May 19, 2021 •

edited

Loading

hjoliver May 23, 2021 •

edited

Loading

MetRonnie May 24, 2021 •

edited

Loading

MetRonnie commented May 19, 2021

oliver-sanders commented May 20, 2021

MetRonnie commented May 20, 2021

hjoliver May 23, 2021

kinow left a comment

hjoliver left a comment

		CYCLER_TYPE_SORT_KEY_INTEGER = "a"
		CYCLER_TYPE_SORT_KEY_INTEGER = 0

	def __cmp__(self, other):
	# Compare to other point.
	if other is None:
	return -1
	if self.TYPE != other.TYPE:
	return cmp(self.TYPE_SORT_KEY, other.TYPE_SORT_KEY)
	if self.value == other.value:
	return 0
	return self.cmp_(other)

Tidy & add type annotations #4213

Tidy & add type annotations #4213

Conversation

MetRonnie commented May 14, 2021

MetRonnie May 14, 2021 • edited Loading

Choose a reason for hiding this comment

MetRonnie May 14, 2021 • edited Loading

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

MetRonnie commented May 14, 2021 • edited Loading

hjoliver commented May 17, 2021

hjoliver commented May 17, 2021

MetRonnie commented May 17, 2021

MetRonnie commented May 17, 2021

hjoliver commented May 17, 2021

MetRonnie commented May 17, 2021

MetRonnie May 19, 2021

Choose a reason for hiding this comment

MetRonnie May 19, 2021 • edited Loading

Choose a reason for hiding this comment

hjoliver May 23, 2021 • edited Loading

Choose a reason for hiding this comment

MetRonnie May 24, 2021 • edited Loading

Choose a reason for hiding this comment

MetRonnie commented May 19, 2021

oliver-sanders commented May 20, 2021

MetRonnie commented May 20, 2021

hjoliver May 23, 2021

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie May 14, 2021 •

edited

Loading

MetRonnie May 14, 2021 •

edited

Loading

MetRonnie commented May 14, 2021 •

edited

Loading

MetRonnie May 19, 2021 •

edited

Loading

hjoliver May 23, 2021 •

edited

Loading

MetRonnie May 24, 2021 •

edited

Loading