[v7r2] Helper functions to make working with errors easier

[chrisburr]

This is an idea that I've been using for my own code with works which DIRAC which I think would be generally useful. As my explanation became a little long I've put it in a codimd document: https://codimd.web.cern.ch/8eEaG3lnRG2SiXCUS6WhFA#

If people approve of the idea I'll add tests and documentation.

BEGINRELEASENOTES

*Core
NEW: Add unwrap and convertReturnValue to simplify writing code with exceptions

ENDRELEASENOTES

[chaen]

As already discussed, I like the idea :-)

[atsareg]

The idea here is smart certainly. But I do not like it. Packing the code in fewer lines does not mean it is more readable, especially for people who are not deeply involved in the development and should quickly guess the logic of what they read. The example of the ComponentInstaller is very good. The longer version is readable as just a text. The shorter version requires extra non-trivial knowledge about the decorator and the unwrap function semantics. Yes, the second one is shorter but this is the only added value. Is it really a value ?

[andresailer]

I would find it much preferable if I could write fewer if result['OK']s. (this scales exponentially the deeper functions are nested, and every time you need to check and return even if you can only take care of the exception at the origin of the call stack.

I think the unwrap should have a less generic name, something that maybe makes it clear it raises exception on error so that one can see the program flow can be interrupted (returnValueOrRaise, similar to returnSingleResult)? On the other hand, unlike the Spanish inquisition, one should always expect exceptions to happen.

Maybe also convertReturnValue should be convertToReturnValue or convertExceptionToReturnValue.

[atsareg]

This discussion happens once in a while :) . May be I am just in habit, but I like the necessity when writing code to do the check if result['OK'] at all the levels. If you think it should be handled upper in the stack, just return it. But you do that consciously and do not rely on some general mechanism. The length of the code IS NOT a problem itself if takes LESS time to understand what goes on. That's why many smart python features allowing to reduce in one line otherwise multi-line code is smart but is actually worse, less clear in the end especially if the code is to be read by your colleagues (or by yourself later on). The constructs like in this PR are creating new language constructs which only have value if everybody speaks this language otherwise we get a Babylonian problem.

[andresailer]

just return it.

except (pun intended?) you can't just return it, you first need to check it.

[chaen]

I do think the length of the code is a problem. You get lost in the if not res['OK']: return res

Had we used exceptions from the beginning, there would be no need for this new functions. What Chris proposes here allows for a backward compatible and smooth transition, although the names could be improved.

Now, when it comes to code complexity, I find it much more difficult to understand what DIRAC does under the hood than this :-D

[atsareg]

Do you want to say that we are transiting from functions returning status to functions throwing exceptions changing completely the paradigm ? I am afraid this will be another project then.

[chaen]

I am afraid this will be another project then.

What do you mean ?

[chrisburr]

In response to @andresailer:

I think the unwrap should have a less generic name,

I'm divided here. Having used this style for a while I find the length of the "unwrap" function to be annoying and forces the code on to multiple lines, especially in combination with DIRAC's tenancy to have fairly long/repetitive class and function names, e.g.

fileCounts = returnValueOrRaise(TransformationClient().getTransformationFilesCount(transID, "Status"))
fileCounts = unwrap(TransformationClient().getTransformationFilesCount(transID, "Status"))

In my code I've been using dw() ("dirac wrapper") and I actually quite like a single character function name to hide it better but might be too concise. We should definitely avoid people working around the length using a mess of different custom names like from xxx import returnValueOrRaise as u. As I've spent too long thinking about the name so I'll let you/others form a consensus on what to call it.

For the decorator I think I prefer @convertToReturnValue without "Exception" as it's also converting valid values to be S_OK.

[chrisburr]

And in response to @atsareg:

The constructs like in this PR are creating new language constructs which only have value if everybody speaks this language otherwise we get a Babylonian problem.

DIRAC has new language constructs by trying to pretend Python doesn't use exceptions for this and I've never seen anything like ret["OK"]/ret["Value"]/ret["Message"] in another Python project. There is definitely value in the exception-free style of error handling (see go/rust/C/...) but that is not Python and never will be.

This unwrap function actually makes the code much more Pythonic and you don't even need to know what it does to understand the code. It also makes DIRAC behave much more like it claims as it's impossible for a function using @convertToReturnValue to raise an Exception (e.g. this error in today's hackathon).

The length of the code IS NOT a problem itself if takes LESS time to understand what goes on.

I agree, but if the code I'm interested in doesn't fit in my screen height it's a problem as it's much harder to maintain the mental model of what it's doing. Of course you have over a decade of looking at DIRAC's way of doing things so anything else is going to look "wrong" to you. Most of the time DIRAC's way results in you being distracted by error handling which doesn't even work properly because you also need to also have the try..except to handle the exceptions thrown by the standard library/other Python packages. Or, more commonly, the exceptions aren't handled properly and just get lost somewhere making debugging painful.

Do you want to say that we are transiting from functions returning status to functions throwing exceptions changing completely the paradigm ?

Despite everything above about why ret["OK"]/ret["Value"]/ret["Message"] is a design mistake I stand by what I wrote in the codimd document:

I don’t propose to retrofit this into existing code in DIRAC and this is not a complex proposal to completely rethink how DIRAC works. This is just a couple of helper functions to make new developments and refactoring slightly more concise, readable and Pythonic.

[atsareg]

First, the goal of the DIRAC project is not to be Pythonic but rather deliver the functionality it is intended for. However, coding in DIRAC should be of course as efficient as possible for most of the developers.
The choice of "no-exception" style was done after long discussions when the DIRAC3 project refactoring was done back to 2006 if I remember correctly. Also other conventions like Camel-style naming. This was a consistent set of rules adopted and used by the project team. Of course, other styles are possible also, but mixing different styles is certainly a bad idea and would be a design mistake.
Now a new generation of DIRAC developers entered the game with their own experience and deem the project development style as crap. This is kind of normal. Coding practices of course changed a lot in 15 years. So, programming styles are not cast in stone and can be (or even should be) reviewed. But rather than adding some "small" helper functions (that will change everything), there should be better an explicit discussion on the basic design principles of the project taking into account that this is a production project and we can not afford any disruptions.

[chrisburr]

First, the goal of the DIRAC project is not to be Pythonic but rather deliver the functionality it is intended for.

DIRAC isn't a project written and used by a single person so the reason to be Pythonic is to make it easier for contributors. Many people that contribute are inexperienced students that benefit much more if they see best practices instead of DIRAC's own weird way of doing things. People literally get rejected during job interview processes for this kind of "bad style". (It's arbitrary and unfair but there is nothing we can do about it.)

explicit discussion on the basic design principles of the project taking into account that this is a production project and we can not afford any disruptions.

Absolutely! I've done this in the context of a PR as the change is so small that I think the idea is better communicated with code. I'm completely open to discussing any technical issues with the approach or better ideas that others may have.

[fstagni]

This is kind of a "coffee discussion" that routinely comes up.

I have mixed feelings (maybe because I am neither part of the first group of DIRAC developers, nor of the last one). On one side, I like exceptions, and their handling. On the other, I would not so much like to code in a "mixed world". With the years I have just accepted that this is how DIRAC is coded and I adapt to it. I agree that:

programming styles are not cast in stone and can be (or even should be) reviewed. But rather than adding some "small" helper functions (that will change everything), there should be better an explicit discussion on the basic design principles of the project

[chrisburr]

"small" helper functions (that will change everything)

I should probably say that I'm somewhat down-playing the potential impact of this. I see this as having the potential to significantly improve the design of DIRAC without causing any migration headaches. For example when reviewing #5149 I couldn't help think that this could have been much easier to understand with my proposal:

def writeToTokenFile(tokenContents, fileName=False):
  """ Write a token string to file
      :param str tokenContents: token as string
      :param str fileName: filename to dump to
      :return: S_OK(str)/S_ERROR()
  """
  if not fileName:
    try:
      fd, tokenLocation = tempfile.mkstemp()
      os.close(fd)
    except IOError:
      return S_ERROR(DErrno.ECTMPF)
    fileName = tokenLocation
  try:
    with open(fileName, 'wb') as fd:
      fd.write(tokenContents)
  except Exception as e:
    return S_ERROR(DErrno.EWF, " %s: %s" % (fileName, repr(e).replace(',)', ')')))
  try:
    os.chmod(fileName, stat.S_IRUSR | stat.S_IWUSR)
  except Exception as e:
    return S_ERROR(DErrno.ESPF, "%s: %s" % (fileName, repr(e).replace(',)', ')')))
  return S_OK(fileName)

@convertToReturnValue
def writeToTokenFile(tokenContents, fileName=False):
  """ Write a token string to file
      :param str tokenContents: token as string
      :param str fileName: filename to dump to
      :return: S_OK(str)/S_ERROR()
  """
  if not fileName:
    fd, tokenLocation = tempfile.mkstemp()
    os.close(fd)
    fileName = tokenLocation
  with open(fileName, 'wb') as fd:
    fd.write(tokenContents)
  os.chmod(fileName, stat.S_IRUSR | stat.S_IWUSR)
  return fileName

In fact, it's now obvious that fileName = tokenLocation is completely pointless and one of the names should be used throughout.

[atsareg]

I think what we should avoid is using our own exceptions in the DIRAC code. Otherwise there will be a mixture of two paradigms which is certainly worse than one or another. But of course we have to deal with exceptions in standard and third party modules. If this PR is to deal with these cases only and only when exceptions or failures in the called functions are not treated in the code of the caller, then these tools are quite appropriate.
As for the function naming, unwrap does not seem to be clear while reading the code. If something concise to be used, may be, it can be "escape". However, Andrés' suggestion I like better.

[fstagni]

Any more ideas on this PR? The comments have been drying up lately.

[chaen]

OK then I propose we just change the name of the method, and we merge it as is. The tool is good as it is, and then we can argue about its use on specific PRs