deprecate log.error and log.exception in favor of log.fatal (WIP)

[boegel]

@stdweird, @JensTimmerman: as discussed yesterday

This PR is not complete yet, the replacement needs to be done in other modules too, but I'd like to get feedback on this first before I make the diff even more unreadable...
A similar cleanup will have to be done in the easyblocks.

This makes #1171, #1174, #1179 and #1182 futile.

cc @riccardomurri

[hpcugentbot]

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1368/
Test FAILed.

[riccardomurri]

isn't self missing from this method's signature?

[boegel]

Not sure yet whether this is actually worth the trouble...

This will not catch all occurrences of the use of the error() log method anyway (see for example https://github.com/hpcugent/easybuild-easyblocks/blob/master/easybuild/easyblocks/generic/intelbase.py#L51).

I'm strongly considering just dropping the custom definition of error (and exception too, while we're at it) all together, and announcing this breaking change on the mailing list (before it gets merged).

This will only affect easyblocks that have not been contributed back, in the sense that the log.error still being used in them will no longer raise an EasyBuildError, and hence will (hopefully) crash and burn somewhere else rather than with a clean error message...

I think we can get away with this by including it in EasyBuild v2.0 (since people are likely to revisit their in-house easyblocks anyway because of http://easybuild.readthedocs.org/en/latest/Deprecated-functionality.htm .

Thoughts?

[boegel]

@riccardomurri: no, self is part of args; I can add self explicitly though

[riccardomurri]

I agree: if you are going to make breaking changes, then v2.0.0 is a better place for them than a minor release.

[JensTimmerman]

no, a call to self.error(msg, args) inside an easyblock will result in a call EasyBlock.error(self, msg, args) because it's a class method.

[boegel]

@JensTimmerman: error will still be there, yes, but not with the intended behaviour anymore since it's no longer redefined in EasyBuildLog

anyway, this PR will change significantly after some discussion with @stdweird, hold on...

[boegel]

Just to clarify to others that were not involved in this discussion: this change is being made because EasyBuild modifies the semantics of the standard error and exception methods, by making them raise an exception rather than only logging.

This has serious consequences for 3rd party libraries like GC3Pie, since if they use log.error this now suddenly results in a hard crash.

The proper (yet painful) solution is to define our own logging method that both logs and raises, which is the log.fatal being proposed here.

[wpoely86]

Looks fine by me but I would change the name. log.fatal seems like something from the standard libraries. As we define our own error handling function, the name should reflect that IMHO. So I would go for something like log.eb_error or log.eb_fatal. Then is 100% clear that this is a custom error method.

[boegel]

Another option is to introduce a fail function (not a logging method, a stand-alone function), that logs and raises an error. Then @JensTimmerman will be happy too that we separate logging and raising exceptions.

The only 'problem' then is that we need to pass the logger instance to this fail method, e.g.:

fail("FAIL!", log=self.log)

[stdweird]

look at the above code again. exception never raised anything

[stdweird]

nvm

[boegel]

will be reworked once hpcugent/vsc-base#156 is merged in

[JensTimmerman]

vsc base was updated

[riccardomurri]

Why not have your fatal exceptions log the message? E.g., something
like::

    import inspect, logging

    class FatalError(EasyBuildError):
          def __init__(msg, logger=None):
              super(EasyBuildError,self).__init__(msg)
              if logger is None:
                 logger = use_callers_logger()
              # search can fail, so check for `None` again
              if logger is not None:
                 logger.fatal(msg)

    def use_callers_logger():
          logger_cls = logging.getLoggerClass()
          cur = inspect.currentframe()
          if cur is None:
             # cannot inspect stack
             return None
          for frameinfo in inspect.getouterframes(cur):
             frame = frameinfo[0]
             bindings = inspect.getargvalues(frame).locals
             for name, value in bindings.items():
                 if isinstance(value, logger_cls):
                     return value

This would allow you to do::

    raise FatalError("Boom!")

and use a logger defined in the caller's environment, or to specify
the logger explicitly::

    raise FatalError("Bang!", logger=log_)

[JensTimmerman]

👍 I like riccardomurri's suggestion a lot
easybuild exception could do this

you could add an else case to use the rootlogger (or another default, maybe an exceptionlogger) if no logger was found.

[boegel]

I like @riccardomurri's idea too... We can maybe even go about this without the fancy inspect stuff, and just use the root logger to log the error? Which logger is logging the error isn't terribly important imho, the stacktrace being correct is a lot more important (and it should be correct using this approach, since the raise will be done in correct location.

I'll look into revamping this PR with an approach like that, thanks for the excellent suggestion @riccardomurri!

[stdweird]

@boegel please keep the inspect stuff. there's no real reason to throw it away. correct logger is the way to go (or we switch to root logger only in whole of EB).
this is generic enough to go into vsc-base btw. the only thing missing is the raiseException case (to reraise the exception that was last thrown; or at least log it).

[JensTimmerman]

I agree with stdweird, the inspect stuff might slow things down, but in easybuild you always quit after an EasyBuildException has been thrown, so having it do an inspect right at the end of your execution is not a big deal.
Do note that inspect will fail if only .pyc and .po files are used, without .py files.

[boegel]

implementation for LoggedException, from which EasyBuildError can/should derive, available in hpcugent/vsc-base#160

[boegel]

different approach following @riccardomurri's suggestion in #1218, closing this as a dead end