Remove noisy runahead base point debug log message - #7382
Conversation
|
Aah, we might need to do something about this as it's likely to cause an efficiency bomb in large workflows. (Lucky we haven't released yet) I'm not quite sure why the change was made, posted a question on https://github.com/cylc/cylc-flow/pull/7237/changes#r3603309497 |
|
Sorry! I did flag that change up front, will comment on the original issue. For this issue, if I'm not wrong this is purely an unfortunate placement of debug logging given the main loop runahead computations. It was already "not (very) valuable", just logged less often. (Mind you, exactly what is helpful in debug mode is a big question - it's not something users should normally need to worry about).
I still disagree with this, as per my comments on the original issue. The same risk applies for 8.x or 8.x.y releases, unless we have people actively testing the upcoming 8.x branch prior to its release, which to my knowledge we don't. (But there is still value in separating bug fix and minor releases, in terms of feature changes that could or should change user behaviour vs just fixes). |
@hjoliver, please see the comments on the previous thread where I outline that risk management is not just about testing, that testing is not just done by us, and that we do have a system of pre-deployment testing in use at the MO. But on the point of testing, this PR rather successfully demonstrates that changes do indeed receive additional testing from us developers who often spot issues on master before they make it into a release. If you need examples, a quick search for the phrase |
MetRonnie
changed the title
Hang on, I never said that it's just about our formal test suite, or that testing is just done by us!
Wait: the verbose logging here did not indicate a surprising deeper change: I explicitly flagged the main loop change up front in the other PR description, and had good reasons to think it was safe (maybe my reasons were off, or maybe you will argue that sort of change should only go to master - but that's subjective, and no one suggested it was potentially concerning at all). Of course there is some small chance of happening upon missed bugs due to the longer lead time for minor releases, which perhaps makes me kinda wrong in principle, but I think you're vastly overstating the case - in practice, pretty extensive pre-release testing with real workflows would be needed to get confidence that we have not introduced subtle but important bugs that affect real workflows.
As I stated already on the other PR (and above here too: " unless we have people actively testing the upcoming 8.x branch prior to its release...") - if there is real pre-release testing of upcoming minor releases, I certainly concede that could catch things -but I wasn't aware that we had that (i.e., we don't have it at my site, and I wasn't aware that your site has it). It's still not clear that "we" have that though: is your "pre-deployment testing" for testing upcoming minor releases prior to release, or for testing new releases prior to deployment into operational systems? The latter doesn't stop bugs from being released - which is what we're talking about here. The former could catch missed bugs before release, but it seems surprising that I've never heard about this kind of testing as factoring into our release timing. (E.g., let's delay the release because the branch hasn't been sufficiently tested yet). |
| LOG.debug(f"Runahead: base point {base_point}") | ||
|
|
There was a problem hiding this comment.
Somewhat of an aside, but if you need to follow the runahead computation for debugging purposes it is important to log the runahead base point. The mildly unfortunate thing here - which was not caused by my recent PR - is that if logging it, we should really only do so when the value changes, not every time the method is called. Fine to remove it now anyway.
There was a problem hiding this comment.
For this sort of thing I can't think why you wouldn't be able to just stick temporary logging in while debugging
There was a problem hiding this comment.
Fair enough, but you'd probably not have noticed (i.e. not much of a problem, for debug logging) if this had been logged only when the value changed instead of in every call to the method.
|
@hjoliver, I'll attempt to answer your questions here - plz read all before quote-responding - if still not happy, suggest a call. It is inevitable that all software projects will release bugs from time to time. We try hard not to, but every now and again things slip the net. That's why projects like ours use semver which delineates the "risk" of a release using a version hierarchy. Sites will then upgrade deployments according to risk appetite. E.g, a production system might accept low-risk bugfix releases but not be willing to accept minor releases which are more likely to break things. Semver-type approaches aren't about "spotting bugs before release" (although project working practices can help with this), they're about helping those who deploy a package manage the risk of upgrade in as informed a way as realistically feasible. Note, the changes we make don't have to introduce "bugs" to cause problems. Subtle changes in behaviour can catch teams out, whether for reasonably or not, careful release management helps ensure issues surface at a reasonable time (e.g, during development) rather than an unreasonable time (e.g, 4am on a Sunday morning). This delineation of risk is a standard software development practice. E.g, A production system might enter development with 8.7.0. It might enter service with 8.7.3, it might be retired with 8.7.7. But it will never upgrade to 8.8.x, its replacement will. This waterfall-esque approach is common in Cylc domains as production systems are extremely complex and expensive to test/operate, making CD type (e.g. blue/black) approaches less attractive. Note, the logical alternative to semver type approaches are calver type approaches. You asked about our pre-deployment testing. I stress risk management isn't actually about pre-release testing, but this might help to illustrate how risk management works in practice:
Because of this approach, only mature releases make it to production. Because bugfix releases are inheritability lower risk, bugfix deployments can be fast-tracked through this system. No this does not help catch bugs before release, that's missing the whole point of software versioning! For the sake of argument, imagine that we were just users of Cylc (rather than contributors too), the purpose of risk management to us as consumers of Cylc is to protect ourselves against unexpected behaviour changes. No sensible production system is going to go live with a package which was just released yesterday, that would be madness! Also note, that Cylc is not the only package in a Cylc deployment, other version upgrades also can and do cause problems. It's on account of this that deployments increasingly use environment management solutions, e.g, uv (eqiv pip-lock), pixi (equiv conda-lock), etc. Environment management tools such as Dependabot can apply different levels of scrutiny to major/minor/bugfix releases and can specify minimum-age - helping ensure releases |
|
@oliver-sanders - ugh, it seems wires got crossed (mainly mine 🤦) but not really in the way you think.
As these comments very clearly show, I had thought (wrongly, it seems!) we were talking about preventing the release of bugs. Just above this it had it only just dawned on me that you might be talking about "pre-deployment testing" of releases. Of course I understand that concept, but I hadn't realized you were talking about that, and it does not help with what I thought you were talking about! (Note we do do it here too, albeit less rigorously due to small team constraints, but we don't really call it that, and in the context of this discussion I was initially unsure of what you meant). Maybe I was being dense and/or responding in a hurry without reading carefully, but all my comments should be taken in the context of that misunderstanding. Anyhow, now that we're on the same page: of course I understand 100% that minor releases are inherently more risky than maintenance releases (that is self-evident, maintenance releases just fix bugs in minor releases after all!); and also that "risky" internal changes should not go to a maintenance release (at one end of the spectrum of possible changes that's blindingly obvious, but for lesser fixes it may be subjective). In retrospect, I can see how several of my comments look like I was disagreeing that minor releases are more risky in general. That's unfortunate, I should have been more careful, but again the context is: I was talking about the likelihood of finding (or not) this kind of bug on the minor branch before release, not post-release risk! (Plus, I had argued, with reasons whether you agreed with them or not, that this particular change was not "risky" compared to leaving the known bugs that it fixes in the next maintenance releases.) The first two comments on the original PR, by the way:
Finally,
Maybe so at your site (and maybe elsewhere too) but clearly that's not necessarily true. Additional pre-release testing could help to find the same kind of bugs that pre-deployment testing does, and it would be feasible during the time leading up to the next minor release. However, that's kinda beside the point; I only asked if you had anyone doing pre-release testing because to my knowledge you did not, but what I (wrongly!) thought you were talking about only makes sense if you did. |
|
(Hopefully that's cleared up now 😥 ) |
|
This probably should have gone to 8.6.x given that my original PR did - to great effect 😬 ... I'll cherry-pick it to #7400 |
3 participants
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.