Improve config deprecated upgrade log, etc #2854

matthewrmshin · 2018-11-12T15:17:52Z

Use DEBUG level for site files - users may not be able to fix them
easily. Use WARNING level for all other files to better alert users.

See also Improve logging #2781 and Fix tests broken by cylc/cylc#2781 and cylc/cylc#2849 metomi/rose#2257.

Remove more print or sys.std*.write statements by converting them to use logging.

Remove some unnecessary diagnostics.
Remove some debug flag check in favour logging level check.
Exit with error message instead of print error then sys.exit(1).

matthewrmshin · 2018-11-12T20:54:36Z

(Not done yet.)

oliver-sanders · 2018-11-13T09:24:35Z

See #1136.

matthewrmshin · 2018-11-13T14:40:54Z

lib/cylc/rundb.py

@@ -1023,15 +1016,11 @@ def upgrade_pickle_to_json(self):
                    else:
                        args.append(cell)
            except (EOFError, TypeError, LookupError, ValueError):
-                n_skips += 1  # skip bad rows
+                pass


These were diagnostics for my own use when I wrote these upgrade methods.

They are mostly upgrade methods for restarting Cylc 6 suites in Cylc 7. We should not have much usage for them any more. Happy to revert and leave them alone but aim to remove them for Cylc 8.

I think it's OK to remove them, as they were for testing/debugging only.

When we go full Python3, this - and other try/catch with similar pattern - can go away too with

with ignored(EOFError, TypeError, LookupError, ValueError): for col, cell in zip(cols, row): ...

Cool syntax.

Cool indeed, I dislike except: pass type statements (although I guess that's probably what this is doing underneath). I've been stuck with Python 2.6 for too long, need to get up to date.

matthewrmshin · 2018-11-13T14:41:21Z

lib/cylc/daemonize.py

-        sys.stderr.write(
-            "fork #1 failed: %d (%s)\n" % (exc.errno, exc.strerror))
-        sys.exit(1)
+    except OSError as exc:


For Python 3 readiness.

👏 👏 👏 thanks!!!

matthewrmshin · 2018-11-13T14:41:35Z

lib/cylc/daemonize.py


    # reset umask, octal
-    os.umask(022)
+    os.umask(0o22)


For Python 3 readiness.

matthewrmshin · 2018-11-13T14:48:00Z

lib/cylc/scheduler.py

-                sys.exit(1)
-
+                LOG.exception(exc2)
+            raise exc


Here we have an unknown Exception. I think it is best if we re-raise it regardless of debug mode or log level.

Indeed Cylc is a little too fond of swallowing exceptions, particularly in cylc.config.

matthewrmshin · 2018-11-13T14:51:02Z

lib/cylc/suite_srv_files_mgr.py

            # Remove symlink to the original suite.
            os.unlink(target)

        # Create symlink to the suite, if it doesn't already exist.
        if source != orig_source:
            os.symlink(source, target)

-        print 'REGISTERED %s -> %s' % (reg, source)
-        return reg
+        return reg, source


This increases the purity of the method:

Let it return all the useful variables to the caller.

Let the caller decide if it needs to print any diagnostics.

I would have wanted to remove the remaining logging logic if there is an easy way to do so.

My first impression was a bit of confusion. Why should we return source, being it is an argument for the function in the first place (which can be a code smell, or a feature or per design [e.g. builder pattern]).

Then I realized reg is also a parameter. I think the code would be simpler to read if it did not return anything. Simply received parameters, registered suites and threw exceptions if any error occurred.

The caller of the function would be responsible for choosing the right value for reg (i.e. if you don't have one, use os.path.basename(os.getcwd())), and for source.

This way, IMO, it would be easier to i) read and understand what the function does, ii) write tests, iii) maintain the code.

The iii may not be clear right now, but if we decide to adopt other strategies for choosing the possible default value for reg or source (e.g. say we have AWS and decide to use an environment variable for the default values?) we wouldn't have to touch this function, but move the logic to the caller.

So I agree with the change, for the current design of the function 😬 but later will try to come up with alternatives (unless I'm missing something that prevents other designs).

matthewrmshin · 2018-11-13T14:53:03Z

lib/cylc/task_events_mgr.py

@@ -524,8 +524,7 @@ def _event_email_callback(self, proc_ctx, schd_ctx):
                else:
                    self.event_timers[id_key].unset_waiting()
            except KeyError as exc:
-                if cylc.flags.debug:
-                    LOG.exception(exc)
+                LOG.exception(exc)


KeyError used to happen quite often because we were relying on the task proxy being in the pool. In the current logic, this is no longer the case - as the event handler contexts are now kept in a dict independent of their original task proxies.

So if an exception is caught, we want to log it, regardless.

Thanks for the info!

matthewrmshin · 2018-11-13T14:53:58Z

Done, I think.

kinow

This pull request had - besides the great work on better logging - some nice improvements for Python3 in exception handling, new comments in the code, some extra comments in GitHub's UI explaining the rationale behind the change in the pull request and some history of the code 👏

+1, LGTM, and big thanks!

lib/cylc/task_events_mgr.py

bin/cylc-register

oliver-sanders · 2018-11-14T10:49:51Z

lib/cylc/rundb.py

@@ -1023,15 +1016,11 @@ def upgrade_pickle_to_json(self):
                    else:
                        args.append(cell)
            except (EOFError, TypeError, LookupError, ValueError):
-                n_skips += 1  # skip bad rows
+                pass


Cool indeed, I dislike except: pass type statements (although I guess that's probably what this is doing underneath). I've been stuck with Python 2.6 for too long, need to get up to date.

oliver-sanders · 2018-11-14T10:51:06Z

lib/cylc/scheduler.py

-                sys.exit(1)
-
+                LOG.exception(exc2)
+            raise exc


Indeed Cylc is a little too fond of swallowing exceptions, particularly in cylc.config.

Use DEBUG level for site files - users may not be able to fix them easily. Use WARNING level for all other files to better alert users. Remove more print and sys.std*.write * Where possible, use logging instead of print or sys.std*.write. * Remove unnecessary diagnostics and some debug flag check in favour of simple logging. * Exit with error message instead of print error then exit 1. Use 0o syntax for octal literal.

matthewrmshin added this to the next release milestone Nov 12, 2018

matthewrmshin self-assigned this Nov 12, 2018

matthewrmshin mentioned this pull request Nov 12, 2018

Fix tests broken by cylc/cylc#2781 and cylc/cylc#2849 metomi/rose#2257

Merged

matthewrmshin force-pushed the improve-conf-deprecated-upgrade-logging branch from 396d01c to 4d629bc Compare November 12, 2018 16:34

matthewrmshin force-pushed the improve-conf-deprecated-upgrade-logging branch 2 times, most recently from 8b01653 to 80cfabf Compare November 13, 2018 13:10

matthewrmshin commented Nov 13, 2018

View reviewed changes

lib/cylc/daemonize.py

# reset umask, octal

os.umask(022)

os.umask(0o22)

Copy link

Contributor Author

matthewrmshin Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Python 3 readiness.

matthewrmshin force-pushed the improve-conf-deprecated-upgrade-logging branch from 80cfabf to 88352a1 Compare November 13, 2018 14:45

matthewrmshin commented Nov 13, 2018

View reviewed changes

matthewrmshin changed the title ~~Improve config deprecated upgrade log~~ Improve config deprecated upgrade log, etc Nov 13, 2018

matthewrmshin requested review from oliver-sanders and kinow November 13, 2018 20:05

kinow approved these changes Nov 13, 2018

View reviewed changes

lib/cylc/task_events_mgr.py Outdated Show resolved Hide resolved

cylc deleted a comment Nov 13, 2018

oliver-sanders reviewed Nov 14, 2018

View reviewed changes

matthewrmshin force-pushed the improve-conf-deprecated-upgrade-logging branch from a0460ed to 64b4d9e Compare November 14, 2018 15:22

oliver-sanders approved these changes Nov 14, 2018

View reviewed changes

oliver-sanders merged commit 4f8dc47 into cylc:master Nov 14, 2018

matthewrmshin deleted the improve-conf-deprecated-upgrade-logging branch November 14, 2018 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve config deprecated upgrade log, etc #2854

Improve config deprecated upgrade log, etc #2854

matthewrmshin commented Nov 12, 2018 •

edited

Loading

matthewrmshin commented Nov 12, 2018

oliver-sanders commented Nov 13, 2018

matthewrmshin Nov 13, 2018

kinow Nov 13, 2018

matthewrmshin Nov 13, 2018

oliver-sanders Nov 14, 2018

matthewrmshin Nov 13, 2018

kinow Nov 13, 2018

matthewrmshin Nov 13, 2018

matthewrmshin Nov 13, 2018

kinow Nov 13, 2018

oliver-sanders Nov 14, 2018

matthewrmshin Nov 13, 2018

kinow Nov 13, 2018

matthewrmshin Nov 13, 2018

matthewrmshin Nov 13, 2018

kinow Nov 13, 2018

matthewrmshin commented Nov 13, 2018

kinow left a comment

oliver-sanders Nov 14, 2018

oliver-sanders Nov 14, 2018

Improve config deprecated upgrade log, etc #2854

Improve config deprecated upgrade log, etc #2854

Conversation

matthewrmshin commented Nov 12, 2018 • edited Loading

matthewrmshin commented Nov 12, 2018

oliver-sanders commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrmshin commented Nov 13, 2018

kinow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrmshin commented Nov 12, 2018 •

edited

Loading