Cylc-flow: Unbound ad_result in cylc.config

[kinow]

Hi,

Yesterday I learned about pyright, a new static analyzer from Microsoft for Python (note: written in TypeScript). Just finished working on pull requests, and raising a few questions on setup.py. But before calling it a day and going home, decided to see what pyright would find in Cylc.

One of the bugs found appears to be that cylc.config contains a function compute_inheritance where there is a for loop, with an `if statement within it.

In that if statement, in one branch it created ad_result, but looks like it is used only in the other branch. Which pyright complains about. I had a look, and I believe the replicate method in the second if branch will be always called with None.

The code appears to be made to work with None, but in this case, it is not clear if it was intentional.

I was not able to quickly replicate it with a suite or unit test, but will take a look at it some day, unless someone beats me to it :+1:

[hjoliver]

git blame says I wrote that method back in 2013. Time :airplane:s But that line was modified late 2015 by @matthewrmshin ... I'll have to try to re-understand.

[hjoliver]

(Matt's change was only PEP8 compliance, so can't blame him :smiling_imp: )

[hjoliver]

@kinow it seems the entire block below line 1059 in your screenshot is never used, because we never override use_simple_method=True.

So we should delete the whole lot. Except that we should probably try to understand the "not simple" method in case we should be using it - the comment claims better efficiency for large suites. :grimacing:

[kinow]

So we should delete the whole lot.

Makes sense! +1

Except that we should probably try to understand the "not simple" method in case we should be using it - the comment claims better efficiency for large suites. grimacing

Ooohh. And this is also a not so simple task I reckon. Tricky to tell what outcome we will have in removing it (or if we are missing the improvements for not using it :rofl: ). Let me know if I can help testing or doing something else :+1:

[matthewrmshin]

We can also try caching the result of replicate (which is presumably the expensive bit?) with functools.lru_cache?

[matthewrmshin]

(Or is that quite hard because replicate has side effect on result?)

[hjoliver]

(haven't had a chance to look at this yet, sorry).

[kinow]

@matthewrmshin

We can also try caching the result of replicate (which is presumably the expensive bit?) with functools.lru_cache?

(Or is that quite hard because replicate has side effect on result?)

I liked the idea too, but looks like replicate has the side effect as you pointed, but it also does not return anything. Perhaps a change in design in replicate - if possible - would allow for functools.lru_cache, and then kill the need of that second else block where the unbound ad_result issue lies right now :confused: :question:

If there is a way to cache replicate, I think this would be a simple and clean way to refactor the code and fix this issue too :+1: