7.8.x patch 1 #2947
7.8.x patch 1 #2947
Conversation
|
Can we get #2938 in? |
|
Yes. We can raise another PR on the 7.8.x branch. |
|
matthewrmshin
force-pushed
the
7.8.x-patch
branch
from
5415b17
to
2e792b1
Compare
| if cylc.flags.debug: | ||
| raise | ||
| sys.exit(str(exc)) | ||
| main() |
There was a problem hiding this comment.
I guess I didn't review the original PR to master, but why this change? (My old "users should see an error message, not a traceback" argument is probably wrong, but we should be consistent, or remove this from all the commands, no?)
There was a problem hiding this comment.
No, your original assumption was not wrong. However:
- Time has changed and many users are now familiar enough with Python that the trace back is no longer scary - and it helps us and them when they send us bug reports.
- Catching known exception (those with nice messages) here is good, but catching all generic exceptions here is not so good - especially when you have those cryptic system errors - which is much better being a trace back.
You are right. I should changed everything.
There was a problem hiding this comment.
Yes, I'm happy to have this removed from all commands.
There was a problem hiding this comment.
Word of warning: In Python3 when you catch an exception its context is not forgotten which results in multi-part traceback which gets really messy really quick.
For example a pattern we use a lot in Cylc is to catch a nasty OSError or KeyError or whatever and raise a more user-friendly CylcException, ConfigError or whatnot. This pattern is at Python3 fundamentally broken as you will end up raising both the original exception as well as the new one.
For example say we are trying to catch ZeroDivisionError and hide it from the user replacing it with a more informative Exception...
>>> try:
... 1/0
... except ZeroDivisionError:
... # raise a completely different exception
... raise Exception('foo')
...
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 4, in <module>
Exception: foo
matthewrmshin
force-pushed
the
7.8.x-patch
branch
from
2e792b1
to
4508816
Compare
On file system such as an NFS mounted file system, we may have a delay before the file becomes visible from other process running on other host. We already have a 1st fsync to ensure that the data of the contact file is written to disk. This change adds a 2nd fsync to ensure that the file metadata of the contact file is written as well - by doing an fsync on its directory.
On SSH call with login shell, sites/users may have logic in profile
scripts writing to STDOUT. This causes the logic that expects JSON to
fail to load the output. This change attempts to drop all lines before
it finds the first open curly bracket `{`, which should be good enough
to denote the start of JSON content in most cases.
So, cylc logger will always have a handler. CylcOptionParser will ensure that commands will have a valid logging handler. Scheduler will do likewise for daemonized suites. Use CylcOptionParser for all remaining user commands: * cylc cycle-point * cylc documentation * cylc get-gui-config * cylc get-site-config This ensures that these commands will have logging handlers set.
matthewrmshin
force-pushed
the
7.8.x-patch
branch
from
4508816
to
65fff77
Compare
|
For future changes that need to target 7.8.x and 8.0, IMO we should raise duplicate separate PRs rather than a combo cherry-pick branch - easier to keep track of what's been merged. Or ask the PR author to do that. (I think this particular combo PR was necessary for historical reasons). |
|
@kinow - can you do a quick sanity check and merge on this (tomorrow); if @oliver-sanders doesn't overnight. |
There was a problem hiding this comment.
Changes look good, I remembered a few of them. Also checked out branch locally and tested:
cylc run --no-detach --debug --verbose fivecylc validate fivecylc scanwith suitefiverunningcylc cat-log --mode=tail fivewith suitefiverunningcylc stopwith suitefiverunningcylc guiand then launch suitefivepytest
All good from what I could tell.
|
Spare time after Spanish classes, so reviewed this one @hjoliver . Leave to @oliver-sanders do the final check & merge 👍 |
That fix can still go in as another PR - no need to block this merge, is there? |
|
This is patch 1. There can be many more. |
Cherry picked: #2916, #2937, #2941, #2943, #2944.