Improve parsec validate #2609

matthewrmshin · 2018-03-23T12:14:05Z

(I have kept this branch in my own space for a while now. I am raising this PR for initial discussion. Happy to drop if not popular.)

Cleaner SPEC data structures:

(which is the main motivation of this change.)
Values are simple lists, instead of individual validator objects, so the data structure can easily be dumped out (or even loaded in). Each list looks like [value_type, default, allowed_2, allowed_3, ...] which should be enough to represent the essentials, where:
- value_type: is the compulsory value type of the setting, and defined only using constants from parsec.validate.ParsecValidator.
- default: is the optional default value. Default is None if this is not defined.
- allowed_2, ...: are the only other allowed values of this setting, if specified.

Other changes:

Single validator with simple dispatch table for coercers methods. All coercers moved to live under parsec.validate.ParsecValidator.
Clearer separation of concerns between parsec.config and parsec.validate, and between parsec.* and cylc.cfgspec.*.
cylc.cfgspec.* modules no longer export the singleton configuration objects - so configuration files should only be loaded on demand at run time, instead of at compile time.

hjoliver · 2018-03-26T04:08:52Z

This seems generally sensible. (I'm not sure that on-demand loading of config files is much of an advantage in our context - suite configs are the only big ones, and they're always "demanded" more or less immediately, no? - but that's not really an objection). Do you have a use case in mind for dumping and loading parsed config files, or is this mainly about simplification (which I think I agree your change does quite nicely).

matthewrmshin · 2018-03-26T08:56:34Z

Quick answers:

The change is mainly about simplification, although I was looking at ways to improve the data structure of the configuration objects and the data structure of their specifications.

On singleton. We have already eliminated the suite.rc singleton in #2373, and we have loading on demand of global.rc in #2601. This change will apply to the remaining, mainly the GUI configuration files, so this is really for completeness rather than a necessity.

On loading/dumping. I am really talking about the specification data structure, instead of the configuration here. It may be good to be able to load the specification on demand - to reduce the memory footprint of loading the cylc.cfgspec.* modules.

oliver-sanders · 2018-04-26T15:48:28Z

lib/parsec/validate.py

+            raise IllegalValueError('boolean', keys, value)
+
+    @classmethod
+    def _coerce_cycle_point(cls, value, keys):


Are we able to use the cylc.cycling.iso8601.SuiteSpecifics.iso8601_parsers[0] instance of TimePointParser rather than creating a new one here or is that not available at this stage?

It is not yet available at this stage, and will introduce a circular dependency between parsec and cylc libraries. Having said, I'll take a look to see whether it is possible to do it the other way round or not.

(Having looked at it, it may be more trouble than it is worth.)

hjoliver · 2018-07-04T04:57:53Z

(We should get this one in once you've resolved the current conflicts)

matthewrmshin · 2018-07-04T05:43:49Z

Quite difficult conflicts, but I think I've got something working. There will be some renames and relocations of classes. (I'm on a course today and tomorrow. I'll reassess this as soon as I'm back on Friday.)

matthewrmshin · 2018-07-09T19:24:38Z

This is good enough again. (Conflict resolved. Tests pass.) I have done some relocation and renames to ensure that the parsec package can still be independent of the cylc package. In particular:

cylc.wallclock is now just wallclock. I have also removed the cylc.flags.utc flag in favour of an encapsulated global setting in wallclock.
The context classes in cylc.mp_pool are moved to parsec.validate. They are renamed SubProcContext and SubFuncContext. (Not the best natural home for these classes, so please feel free to suggest a better location.)

matthewrmshin · 2018-10-03T10:51:21Z

Conflicts resolved. Should be ready to review again.

lib/parsec/config.py

hjoliver

This is going to seem like an anticlimax after the long wait, but ... I'm happy with this change, and haven't found any further issues with it 💥

matthewrmshin · 2018-10-23T08:47:03Z

@oliver-sanders please sanity check.

Single validator with simple dispatch table for coercers. Cleaner specs that reflect actual usages for type, default, options.

Relocate dependency on wallclock module. Rename subprocess context classes.

SubProcContext and SubFuncContext now in their own module. * cylc.subprocctx * cylc.mp_pool renamed cylc.subprocpool New CylcConfigValidator subclass of ParsecValidator in its own module. The wallclock module is moved back to cylc.wallclock namespace.

matthewrmshin · 2018-10-24T07:50:34Z

Resolved conflict by rebasing.

matthewrmshin · 2018-10-24T17:49:45Z

@oliver-sanders please prioritise review of this one.

oliver-sanders

I'm broadly OK with this change, I can't find any way to break it and am running out of ways to test it.

I think it's time to put it in and see what happens!

oliver-sanders · 2018-10-26T13:11:02Z

lib/cylc/cfgvalidate.py

+
+    def __init__(self):
+        ParsecValidator.__init__(self)
+        self.coercers.update({


review postscript: if we made self.coercers a class attribute then validate could be a class method saving us having to create validator objects.

matthewrmshin added this to the soon milestone Mar 23, 2018

matthewrmshin self-assigned this Mar 26, 2018

matthewrmshin force-pushed the improve-parsec-validate branch from 26f43a3 to 4655716 Compare March 26, 2018 09:01

matthewrmshin force-pushed the improve-parsec-validate branch 4 times, most recently from 7ae1885 to 621b874 Compare April 23, 2018 14:52

matthewrmshin requested review from hjoliver and oliver-sanders April 24, 2018 11:43

oliver-sanders reviewed Apr 26, 2018

View reviewed changes

matthewrmshin force-pushed the improve-parsec-validate branch 2 times, most recently from 4fdb388 to 82417a9 Compare May 2, 2018 10:14

matthewrmshin force-pushed the improve-parsec-validate branch 2 times, most recently from 8b01f94 to 7ecc46a Compare May 16, 2018 13:26

matthewrmshin force-pushed the improve-parsec-validate branch from 7ecc46a to 78d9148 Compare May 22, 2018 11:48

matthewrmshin modified the milestones: soon, next release May 25, 2018

matthewrmshin mentioned this pull request Jun 28, 2018

External trigger "plugin" functions. #2423

Merged

hjoliver mentioned this pull request Jul 4, 2018

Incorporate suite host selection logic #2693

Merged

7 tasks

matthewrmshin force-pushed the improve-parsec-validate branch from 78d9148 to e10a0b3 Compare July 9, 2018 15:46

matthewrmshin force-pushed the improve-parsec-validate branch from e10a0b3 to c66774d Compare July 10, 2018 11:51

matthewrmshin force-pushed the improve-parsec-validate branch 2 times, most recently from 543b3be to 854cd2f Compare July 20, 2018 08:29

matthewrmshin force-pushed the improve-parsec-validate branch from 854cd2f to 936602e Compare August 3, 2018 15:49

matthewrmshin force-pushed the improve-parsec-validate branch from 936602e to c718a85 Compare August 15, 2018 14:53

matthewrmshin force-pushed the improve-parsec-validate branch from 6d06a98 to 061b13b Compare September 18, 2018 09:25

matthewrmshin force-pushed the improve-parsec-validate branch 2 times, most recently from 3d37445 to c2191cb Compare September 25, 2018 15:29

matthewrmshin force-pushed the improve-parsec-validate branch from c2191cb to 7bd6b04 Compare October 3, 2018 10:49

cylc deleted a comment Oct 3, 2018

matthewrmshin mentioned this pull request Oct 3, 2018

Improve logging #2781

Merged

cylc deleted a comment Oct 8, 2018

matthewrmshin mentioned this pull request Oct 14, 2018

Fix global config get for port scan. #2794

Merged

matthewrmshin force-pushed the improve-parsec-validate branch from ac73439 to 35d51f1 Compare October 15, 2018 15:18

cylc deleted a comment Oct 15, 2018

hjoliver reviewed Oct 21, 2018

View reviewed changes

lib/parsec/config.py Show resolved Hide resolved

hjoliver approved these changes Oct 23, 2018

View reviewed changes

matthewrmshin added 6 commits October 24, 2018 08:49

Improve parsec validate

8024d38

Single validator with simple dispatch table for coercers. Cleaner specs that reflect actual usages for type, default, options.

Misc relocation and fixes

10061ee

Relocate dependency on wallclock module. Rename subprocess context classes.

Update copyright

5bba4ac

Unit tests for ParsecValidator.coerce_* methods

cd2985a

Fix unused import

4ca487b

matthewrmshin force-pushed the improve-parsec-validate branch from 35d51f1 to 4ca487b Compare October 24, 2018 07:49

cylc deleted a comment Oct 24, 2018

cylc gui graph view sub-graph setting fix

5deab7e

cylc deleted a comment Oct 24, 2018

oliver-sanders approved these changes Oct 29, 2018

View reviewed changes

oliver-sanders merged commit 8012caf into cylc:master Oct 29, 2018

matthewrmshin deleted the improve-parsec-validate branch October 29, 2018 18:47

matthewrmshin mentioned this pull request Nov 25, 2018

Move parsec to cylc package #2880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve parsec validate #2609

Improve parsec validate #2609

matthewrmshin commented Mar 23, 2018

hjoliver commented Mar 26, 2018 •

edited

Loading

matthewrmshin commented Mar 26, 2018

oliver-sanders Apr 26, 2018

matthewrmshin Apr 26, 2018

matthewrmshin Apr 27, 2018

hjoliver commented Jul 4, 2018

matthewrmshin commented Jul 4, 2018

matthewrmshin commented Jul 9, 2018

matthewrmshin commented Oct 3, 2018

hjoliver left a comment

matthewrmshin commented Oct 23, 2018

matthewrmshin commented Oct 24, 2018

matthewrmshin commented Oct 24, 2018

oliver-sanders left a comment

oliver-sanders Oct 26, 2018

Improve parsec validate #2609

Improve parsec validate #2609

Conversation

matthewrmshin commented Mar 23, 2018

hjoliver commented Mar 26, 2018 • edited Loading

matthewrmshin commented Mar 26, 2018

oliver-sanders Apr 26, 2018

Choose a reason for hiding this comment

matthewrmshin Apr 26, 2018

Choose a reason for hiding this comment

matthewrmshin Apr 27, 2018

Choose a reason for hiding this comment

hjoliver commented Jul 4, 2018

matthewrmshin commented Jul 4, 2018

matthewrmshin commented Jul 9, 2018

matthewrmshin commented Oct 3, 2018

hjoliver left a comment

Choose a reason for hiding this comment

matthewrmshin commented Oct 23, 2018

matthewrmshin commented Oct 24, 2018

matthewrmshin commented Oct 24, 2018

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders Oct 26, 2018

Choose a reason for hiding this comment

hjoliver commented Mar 26, 2018 •

edited

Loading