Refactoring of Exception handling code in common.py

[Forthoney]

Working on PR #1511, I noticed that the error handling in common.py was a bit bloated in general, probably due to legacy code not being updated, several people working on it, etc. I found a couple code patterns where it's clear that a try-except block is unnecessary. I included examples of these patterns below.

1. Catches an exception only to re-raise it.
   

2. Catches an exception and simply prints the error message.
   

3. Catches an exception, repackages it in a less specific Exception and re-raises.
   

For the first case, the try-except block will actually not do anything, so it can be removed without any change in behavior.
The second case technically prevents the error from being raised while still providing info about it. The error catching for the second case unnecessary and arguably misleading, since the receiving an error communicates to the user that something went wrong better than just a print statement. Errors may look ugly, but they should be raised, especially if the user's command did not go through.
The third case is unnecessary because the ImportError by itself is already alerting the user to the fact that they need to install the missing package. Repackaging it into a generic Exception with a different message may make the problem more difficult to resolve. A user will find much more information about how to solve a ImportError rather than a generic exception with geemap's specific error message.

As mentioned in #1511, there still remains room for bugs in common.py, especially in the error handling. Reducing unneeded try-except blocks would help narrow down the room for bugs. I would love to hear your thoughts on this refactoring proposal

[giswqs]

I agree that the error handling can be improved. The point of using raise Exception(e) is to reduce the error message from many lines to just one line. Many of those functions are being used by the Output widget in the toolbar GUI. Without it, the original error message (sometime 20-30 lines) can occupy the entire map space, which is ugly. See the difference below.

[Forthoney]

Thanks for the detailed example. It seems that Google Earth's ee API has interesting design choices. Looking at the error log in your example, the get_persistent_credentials() function looks like this

def get_persistent_credentials():
  """Read persistent credentials from ~/.config/earthengine.

  Raises EEException with helpful explanation if credentials don't exist.

  Returns:
    OAuth2Credentials built from persistently stored refresh_token
  """
  try:
    return Credentials(None, **oauth.get_credentials_arguments())
  except IOError:
    raise ee_exception.EEException(
        'Please authorize access to your Earth Engine account by '
        'running\n\nearthengine authenticate\n\n'
        'in your command line, and then retry.')

The reason why the error log is so long is because this single function is throwing two errors for one problem: there is no credentials file.
It throws both FileNotFoundError at the line return Credentials... This error being thrown makes sense since the operating system can't find the file. With that said, just this error alone is a bit confusing and doesn't provide much info on how to fix it.
I think that's where the second error EEError comes in. It does indeed have a more informative error message attached to it ("Please authorize access..."). But I don't think two errors being thrown is the best way to handle the situation since, as you mentioned, it clutters the screen horribly.

This double-error-throwing is happening despite the try-except block because the block only catches an IOError. I'm not sure in which context the IOError would be thrown, but it seems pretty clear that it should also catch the FileNotFoundError, so that only one error is thrown to the end user: the EEError with the useful message. For this specific issue, I submitted ticket on the Google Earth API repository itself.

[Forthoney]

As in what this means for geemap, I still do think that some sort of exception should be thrown, even in this example (I would even say especially in this example).

1. Clear indication of failure
   If something fails critically, meaning subsequent cells will not run at all, it should be the case that the user is alerted about this through the cell failing to run. If we just print the error message while allowing the cell to be marked as complete, it's not as clear to the user that a critical problem occurred.

2. Easier debugging
   An error being printed in the console will have a traceback of where the error came from. This makes it much more easier for users to fix the problem themselves. It also helps developers greatly, since they will have access to the full traceback in help requests or bug requests, meaning they can pinpoint where the problem is coming from. It may clutter the output box, but it helps in the long run.

With that said, for this case, I think the error is indeed overdone, not on geemap's end, but rather on Google Earth Engine's end. One way of reducing the output log while still indicating something went wrong is

try:
    ee.Initialize()
except ee.ee_exception.EEException as e:
    raise e from None

What raise e from None does is it disables automatic error chaining. You can find out more about it here. Oversimplifying greatly, this will drop superfluous error statements (i.e. errors chained to the one we caught).

The end result is

It is shorter than the original (if you view the full output) and much easier to figure out where the error is actually coming from. I don't think error tracebacks being long is really a problem, as long as it's clear where to look to resolve the issue.

[giswqs]

Thank you for looking into this. You are welcome to submit a pull request to improve the error handling in geemap. One thing to keep in mind is that if a function is invoked directly or indirectly by the map GUI, the error message should be limited to one to two lines. Otherwise, the output widget on the map interface might be able display the full error message.

[Forthoney]

Sorry to keep you hanging. I tried my hand a bit at improving error handling and I'm finding that there's larger issues that obstruct fully being able to make this change. Namely, it's very difficult to incrementally apply changes because of very large files like common.py being very large. I think this issue can be backlogged for the time being and we can return to it when the codebase is made more atomic (the addition of a core.py file #1527 is a great step!). I can submit an issue with that summarizes what needs to be done based on this discussion thread.