Stop traceback when `ParsecError` occurs during config load on workflow run #4720

MetRonnie · 2022-02-28T15:29:03Z

These changes close #4216

ParsecErrors that occur during config load (either on workflow start or reload) no longer cause traceback, instead just giving a one line message, because we expect these to occur.

ParsecErrors at any other time during workflow run are unexpected and still give traceback.

Additionally, the Scheduler won't crash if a ParsecError occurs during reload (not sure if this was already fixed, but anyway it is fixed now for sure).

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.cfg and conda-environment.yml.
Appropriate tests are included (integration)
Appropriate change log entry included.
No documentation update required.

MetRonnie · 2022-02-28T17:45:10Z

Codecov upload and MacOS tests being particularly flaky today. Tests passing otherwise

wxtim

Overall happy

Read PR
- Made a couple of comments about grammar &c, mostly reflecting dissatisfaction with my own previous work!
Manually tested as many scenarios as I could think of
Run tests locally
Looked at testing of this change.
- I'm a little surprised that this change didn't affect tests/unit/parsec/test_validate.py, but I think it's probably a sign that the tests have been formatted to be robust against small changes to exception messages which I think is a good thing ™.

cylc/flow/parsec/fileparse.py

CHANGES.md

Add more specific info to error messages

MetRonnie · 2022-03-02T12:54:41Z

Changes in response to Tim: https://github.com/cylc/cylc-flow/compare/eddf71b0b41a3d71dd1b8ef96fb8729512c2fcfe..5d7e1d9da1c90f7b50bb0c7ad08adf466014f1cd

also viewable by clicking on "force pushed" above where it says

MetRonnie force-pushed the parsec-error-tb branch from eddf71b to 5d7e1d9

oliver-sanders · 2022-03-02T17:06:13Z

tests/integration/utils/flow_tools.py

    finally:
+        yield caplog


What's the logic of moving the yield, feels wrong.

(is it so you can do nasty things like setting up schedulers which are doomed to fail?)

Yes. Otherwise you won't get the caplog yielded

But you have a good point... this now breaks the teardown if an assertion fails in the test. Converted to draft while I explore a couple of solutions

Note to self: useful info: https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager

Should be addressed now - https://github.com/cylc/cylc-flow/compare/5d7e1d9da1c90f7b50bb0c7ad08adf466014f1cd..04164177e63578bf69578ecf4f36f76e740ffe97

tests/integration/utils/flow_tools.py

Also ensure caplog instances always yielded

MetRonnie · 2022-03-03T13:18:39Z

Test failures due to GH Actions flakiness 😒

cylc/flow/scheduler.py

oliver-sanders · 2022-03-04T09:21:25Z

cylc/flow/scheduler.py

+        try:
+            self.load_flow_file()
+        except ParsecError as exc:
+            exc.schd_expected = True


Could you add a comment explaining what the exc.schd_expected is about. Not so keen on the name schd_expected but can't think of anything better.

cylc/flow/scheduler.py

tests/integration/test_examples.py

tests/integration/test_scheduler.py

MetRonnie · 2022-03-04T11:31:49Z

Will push up a commit addressing your review comments after your response to my response

oliver-sanders

Looks good, I'm going to try some manual testing, need to consider CylcConfigError too?

oliver-sanders · 2022-03-31T09:58:54Z

@MetRonnie waiting on CylcConfigError which should probably be given the same treatment?

#4720 (comment)

MetRonnie · 2022-03-31T10:09:55Z

CylcConfigErrors are CylcErrors so don't need any special treatment.#

E.g. try out

diff --git a/cylc/flow/config.py b/cylc/flow/config.py
index 44e7b2a6e..7f8054e43 100644
--- a/cylc/flow/config.py
+++ b/cylc/flow/config.py
@@ -1958,6 +1958,8 @@ class WorkflowConfig:
 
     def load_graph(self):
         """Parse and load dependency graph."""
+        from cylc.flow.exceptions import CylcConfigError
+        raise CylcConfigError("Moo")
         LOG.debug("Parsing the dependency graph")
 
         # Generate a map of *task* members of each family.

followed by cylc play

oliver-sanders · 2022-03-31T14:53:13Z

CylcConfigErrors are CylcErrors so don't need any special treatment.

Ok, I'll resolve that comment then.

oliver-sanders

Tested, works nicely, couldn't break it 👍.

MetRonnie · 2022-03-31T15:14:13Z

Ah, I was looking at the wrong comment re: CylcConfigError. Bottom line is:

No special treatment is needed during run/restart
Special treatment is needed during reload, unless we want to make all CylcErrors "expected" during reload?

wxtim

Happy with changes since last I looked.

MetRonnie added the could be better Not exactly a bug, but not ideal. label Feb 28, 2022

MetRonnie added this to the cylc-8.0rc2 milestone Feb 28, 2022

MetRonnie self-assigned this Feb 28, 2022

MetRonnie force-pushed the parsec-error-tb branch from 8ccce2b to 00d8732 Compare February 28, 2022 16:24

MetRonnie requested review from oliver-sanders and wxtim February 28, 2022 16:24

wxtim reviewed Mar 1, 2022

View reviewed changes

cylc/flow/parsec/fileparse.py Outdated Show resolved Hide resolved

cylc/flow/parsec/fileparse.py Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

MetRonnie mentioned this pull request Mar 2, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

MetRonnie added 4 commits March 2, 2022 12:52

Tidy

2dee1e9

Fix traceback for ParsecErrors during scheduler config load

f80d17e

Improve parsec illegal value errors

3d9ee95

Add more specific info to error messages

Integration tests: modify flow_writer to allow top level settings

f808a65

MetRonnie force-pushed the parsec-error-tb branch from eddf71b to 5d7e1d9 Compare March 2, 2022 12:53

oliver-sanders reviewed Mar 2, 2022

View reviewed changes

tests/integration/utils/flow_tools.py Outdated Show resolved Hide resolved

MetRonnie marked this pull request as draft March 2, 2022 22:41

MetRonnie added 3 commits March 3, 2022 12:40

Simplify integration test workflow runner

d41335d

Also ensure caplog instances always yielded

Add test for ParsecError traceback

a917e65

Update changelog

0416417

MetRonnie force-pushed the parsec-error-tb branch from 5d7e1d9 to 0416417 Compare March 3, 2022 12:44

MetRonnie marked this pull request as ready for review March 3, 2022 12:45

oliver-sanders reviewed Mar 4, 2022

View reviewed changes

cylc/flow/scheduler.py Show resolved Hide resolved

oliver-sanders reviewed Mar 4, 2022

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Mar 4, 2022

View reviewed changes

tests/integration/test_examples.py Show resolved Hide resolved

oliver-sanders reviewed Mar 4, 2022

View reviewed changes

tests/integration/test_scheduler.py Show resolved Hide resolved

tests/integration/test_scheduler.py Show resolved Hide resolved

tests/integration/test_scheduler.py Outdated Show resolved Hide resolved

Address code review

ea69c2e

MetRonnie requested a review from oliver-sanders March 15, 2022 13:56

wxtim self-requested a review March 15, 2022 16:57

oliver-sanders modified the milestones: cylc-8.0rc2, cylc-8.0rc3 Mar 21, 2022

oliver-sanders reviewed Mar 23, 2022

View reviewed changes

MetRonnie added 2 commits March 31, 2022 11:02

Merge branch 'master' into parsec-error-tb

6d9e7c9

Update changelog

2e1bed4

oliver-sanders approved these changes Mar 31, 2022

View reviewed changes

wxtim approved these changes Mar 31, 2022

View reviewed changes

wxtim merged commit cd0f9c9 into cylc:master Mar 31, 2022

MetRonnie deleted the parsec-error-tb branch March 31, 2022 15:17

oliver-sanders mentioned this pull request Mar 31, 2022

scheduler: log command failures at error level #4793

Merged

6 tasks

wxtim mentioned this pull request Apr 4, 2022

Make test_pre_configure[template engine conflict] less specific cylc/cylc-rose#131

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop traceback when `ParsecError` occurs during config load on workflow run #4720

Stop traceback when `ParsecError` occurs during config load on workflow run #4720

MetRonnie commented Feb 28, 2022 •

edited

Loading

MetRonnie commented Feb 28, 2022

wxtim left a comment •

edited

Loading

MetRonnie commented Mar 2, 2022

oliver-sanders Mar 2, 2022

oliver-sanders Mar 2, 2022

MetRonnie Mar 2, 2022

MetRonnie Mar 2, 2022 •

edited

Loading

MetRonnie Mar 3, 2022

MetRonnie commented Mar 3, 2022

oliver-sanders Mar 4, 2022

MetRonnie commented Mar 4, 2022

oliver-sanders left a comment •

edited

Loading

oliver-sanders commented Mar 31, 2022

MetRonnie commented Mar 31, 2022 •

edited

Loading

oliver-sanders commented Mar 31, 2022

oliver-sanders left a comment

MetRonnie commented Mar 31, 2022

wxtim left a comment

Stop traceback when ParsecError occurs during config load on workflow run #4720

Stop traceback when ParsecError occurs during config load on workflow run #4720

Conversation

MetRonnie commented Feb 28, 2022 • edited Loading

MetRonnie commented Feb 28, 2022

wxtim left a comment • edited Loading

Choose a reason for hiding this comment

MetRonnie commented Mar 2, 2022

oliver-sanders Mar 2, 2022

Choose a reason for hiding this comment

oliver-sanders Mar 2, 2022

Choose a reason for hiding this comment

MetRonnie Mar 2, 2022

Choose a reason for hiding this comment

MetRonnie Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

MetRonnie Mar 3, 2022

Choose a reason for hiding this comment

MetRonnie commented Mar 3, 2022

oliver-sanders Mar 4, 2022

Choose a reason for hiding this comment

MetRonnie commented Mar 4, 2022

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders commented Mar 31, 2022

MetRonnie commented Mar 31, 2022 • edited Loading

oliver-sanders commented Mar 31, 2022

oliver-sanders left a comment

Choose a reason for hiding this comment

MetRonnie commented Mar 31, 2022

wxtim left a comment

Choose a reason for hiding this comment

Stop traceback when `ParsecError` occurs during config load on workflow run #4720

Stop traceback when `ParsecError` occurs during config load on workflow run #4720

MetRonnie commented Feb 28, 2022 •

edited

Loading

wxtim left a comment •

edited

Loading

MetRonnie Mar 2, 2022 •

edited

Loading

oliver-sanders left a comment •

edited

Loading

MetRonnie commented Mar 31, 2022 •

edited

Loading