Allow global constants to be controlled by a context manager

[davidhassell]

Fixes #154
Fixes #85

Hi @sadielbartholomew

Note that I removed the unsettable constants from cf.configuration because they upset using the output to reset the global config. I.e.

>>> old = cf.configuration()
>>> cf.confiuration(old)

did not work. We should review this another day, at the same time as thinking about code re-use between cfdm and cf-python in this area.

I also made cf-python copies of ATOL and RTOL to address NCAS-CMS/cfdm#103 (comment)

Thanks.

[davidhassell]

@sadielbartholomew I have just seen that the .rst have been corrupted (I suspect by a newer version of sphinx that installed itself against my wishes) - I shall fix them.

[davidhassell]

This PR now also requires NCAS-CMS/cfdm/pull/104

[sadielbartholomew]

Note that I removed the unsettable constants from cf.configuration because they upset using the output to reset the global config

That's okay, in fact for the best I would say, as values that can't be set aren't strictly 'configuration'. That and they were a bit of a pain to handle as a special case, logic-wise!

I believe all PR dependencies are now merged, so I'll start reviewing.

[sadielbartholomew]

I think this PR will close #85 too, by the way.

[davidhassell]

I'll merge #148 into this PR, so that cf.bounds_combination_mode can get the "class" and "with" treatment (or is that a bad idea?)

[sadielbartholomew]

so that cf.bounds_combination_mode can get the "class" and "with" treatment (or is that a bad idea?)

No, I think that is a good idea. If a user does something silly with the newly-acquired flexibility then that is their own fault...

[sadielbartholomew]

I guess this is ready for review now? If not, please let me know when it is ready.

[davidhassell]

All good - please do review, thanks.

[sadielbartholomew]

The functionality otherwise looks good (so far, not quite finished testing that aspect), but I've spotted that the context manager settings persist if it exits with an exception, for example note how the rtol and chunksize are permanently changed when it errors (cleanly, as standard) with a bad log level:

>>> import cf
>>> orig = cf.configuration()
>>> print(orig['log_level'], orig['rtol'], orig['chunksize'])
WARNING 2.220446049250313e-16 82873466.88000001
>>> with cf.configuration(log_level=4, rtol=5e-10, chunksize=5e10):
...     print(cf.configuration)
... 
ValueError: 4 is not a valid ValidLogLevels

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sadie/cf-python/cf/functions.py", line 364, in configuration
    return _configuration(
  File "/home/sadie/cf-python/cf/functions.py", line 439, in _configuration
    reset_mapping[setting_alias](new_value)  # ...run corresponding func
  File "/home/sadie/cfdm/cfdm/functions.py", line 926, in __new__
    cls._CONSTANTS[cls._name] = cls._parse(cls, arg)
  File "/home/sadie/cfdm/cfdm/functions.py", line 1242, in _parse
    elif cls._is_valid_log_level_int(arg):
  File "/home/sadie/cfdm/cfdm/functions.py", line 227, in _is_valid_log_level_int
    ValidLogLevels(int_log_level)
  File "/home/sadie/anaconda3/envs/cf-env/lib/python3.8/enum.py", line 309, in __call__
    return cls.__new__(cls, value)
  File "/home/sadie/anaconda3/envs/cf-env/lib/python3.8/enum.py", line 600, in __new__
    raise exc
  File "/home/sadie/anaconda3/envs/cf-env/lib/python3.8/enum.py", line 584, in __new__
    result = cls._missing_(value)
  File "/home/sadie/anaconda3/envs/cf-env/lib/python3.8/enum.py", line 613, in _missing_
    raise ValueError("%r is not a valid %s" % (value, cls.__name__))
ValueError: 4 is not a valid ValidLogLevels
>>> new = cf.configuration()
>>> print(new['log_level'], new['rtol'], new['chunksize'])
WARNING 5e-10 50000000000.0

Possibly this is the case for the logic in cfdm (I'll check in a moment) but unfortunately if it is, I missed it there.

So it seems like we need a tweak to the PR (and possibly the cfdm merged code) to handle resetting of any temporary changes to values by the context manager upon tear down with an exception, generally. A possible way to do that would be with a try/except/finally block. I actually used one for a similar case of resetting in any closure scenario with the manage_log_level_via_verbos{ity, e_attr} decorators, e.g. here in cf-python for the latter:

cf-python/cf/decorators.py

Lines 142 to 162 in 2f25237

	# After method completes, re-set any changes to log level or enabling
	try:
	return method_using_verbose_attr(self, *args, **kwargs)
	except Exception:
	raise
	finally: # so that crucial 'teardown' code runs even if method errors
	# Decrement indicates one decorated function has finished execution
	calls[0] -= 1
	# Due to the incrementing and decrementing of 'calls', it will
	# only be zero when the outermost decorated method has finished,
	# so the following condition prevents resetting occurring once
	# inner functions complete (which would mean any subsequent code
	# in the outer function would undesirably regain the global level):
	if calls[0] == 0:
	if verbose_attr == 0:
	_disable_logging(at_level='NOTSET') # lift deactivation
	elif (verbose_attr is not None and
	_is_valid_log_level_int(verbose_attr)):
	_reset_log_emergence_level(log_level())
	if log_level() == 'DISABLE' and verbose_attr != 0:
	_disable_logging() # disable again after re-enabling

[davidhassell]

Good spot, and thanks for the tips. I have put in a fix for cfdm (NCAS-CMS/cfdm#107), which I shall follow up here, too.

[sadielbartholomew]

Thanks @davidhassell, continuing with a (re-)review inclusive of the new fix commit now and I'll finish it tonight.

[sadielbartholomew]

Feedback regarding the issue with persistence on tear down has been addressed.

The context manager works and is well-tested. The documentation has been updated appropriately. Great stuff. Happy to merge.

[sadielbartholomew]

I was wondering if there might be cases missed where exceptions other than ValueError are raised, especially here in cf-python with many more constants where some error could be encountered in trying to set them, but it does seem that at the moment it is only possible to run into a ValueError (sometimes raised in that form after being caught as e.g. an AttributeError or a TypeError) so that is all fine.

I'm mainly noting this since, in future, if we add exception handling for the constants they should ultimately raise only ValueError, else they should be added to this except catch.