Add an error message when GADM server is down

[ekatef]

Changes proposed in this Pull Request

Add an error message for a case when GADM server is down.

Checklist

• [x] I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
• [ ] I tested my contribution locally and it seems to work fine.
• [ ] Code and workflow changes are sufficiently documented.
• [ ] Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
• [ ] Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
• [ ] Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
• [ ] Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
• [ ] A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

[ekatef]

A suggestion on how to make more clear that GADM server is down, when running the workflow. @davide-f may you please have a look? :)

Error messages are duplicated to keep error message both in console and in logs.

It could probably make sense to stop execution with something like sys.exit(1) instead of raising exceptions. Although, it would be not quite consistent with the approaches used in different parts of the code.

[ekatef]

Nice draft :D few little comments! :)

Thanks a lot!! :)) Added a next iteration :)

[ekatef]

Nice draft :D few little comments! :)

@davide thanks a lot for reviewing! :)

As a comment, I fear a currently implemented approach is not perfectly consistent with the rest of the code. Usually, we are rising exceptions to handle errors, for example: https://github.com/pypsa-meets-earth/pypsa-earth/blob/b205e83c3b631448a778e29257eed551790b9d5c/scripts/build_shapes.py#L577-L580 . Which also means that error messages are not captured into log files, while console output looks a sometimes not perfectly clear.

Now we are testing logger.error(.) + sys.exit(.) which seems to look and work good, but is not perfectly consistent with the other code. Happy to revise the current approach or make adjustments across the code. What is your feeling about that? :)

[davide-f]

Agree, nice catch! :) Also less lines of code :)

[ekatef]

@davide-f thanks a lot for looking into logs for uncaught exceptions :) Have tried an implementation you referred, and I think the results is very nice!

Snakemake error listing remains and is verbose as usual, while a cleaner output is added to a log file. An example of log output provoked by calling an undefined variable:

ERROR:_helpers:An error happened during workflow execution.
Traceback (most recent call last):
  File "/Users/ekaterina/Documents/_github_/pypsa-earth/.snakemake/scripts/tmpz3zieg8k.build_shapes.py", line 942, in <module>
    print(x + y)
NameError: name 'x' is not defined

I think, that may be exactly what we do need. What is your feeling about that?

[davide-f]

@davide-f thanks a lot for looking into logs for uncaught exceptions :) Have tried an implementation you referred, and I think the results is very nice!

Snakemake error listing remains and is verbose as usual, while a cleaner output is added to a log file. An example of log output provoked by calling an undefined variable:

ERROR:_helpers:An error happened during workflow execution.
Traceback (most recent call last):
  File "/Users/ekaterina/Documents/_github_/pypsa-earth/.snakemake/scripts/tmpz3zieg8k.build_shapes.py", line 942, in <module>
    print(x + y)
NameError: name 'x' is not defined

I think, that may be exactly what we do need. What is your feeling about that?

So, for this PR, I personally believe that simply using the raise instead of logging.error was enough, while the better handling of the exception could be done in a separate PR.

I like the implementation, but I'd keep them separate to ease the revision too as the revision of the logger/exception handling should be done to all other scripts as well. Would you mind moving these interesting changes to a different PR so we could discuss on that?

To better handle the errors, I strongly like this draft. I'd envision in the helpers a function, like create_logger or alike that accepts a string input that is the logger name and encapsulates this code:

logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)

handler = logging.StreamHandler(stream=sys.stdout)
logger.addHandler(handler)
sys.excepthook = handle_exception

However, since implementing this requires changing all scripts, I'd keep for a separate PR, what do you think?

[ekatef]

So, for this PR, I personally believe that simply using the raise instead of logging.error was enough, while the better handling of the exception could be done in a separate PR.

I like the implementation, but I'd keep them separate to ease the revision too as the revision of the logger/exception handling should be done to all other scripts as well. Would you mind moving these interesting changes to a different PR so we could discuss on that?

To better handle the errors, I strongly like this draft. I'd envision in the helpers a function, like create_logger or alike that accepts a string input that is the logger name and encapsulates this code:

logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)

handler = logging.StreamHandler(stream=sys.stdout)
logger.addHandler(handler)
sys.excepthook = handle_exception

However, since implementing this requires changing all scripts, I'd keep for a separate PR, what do you think?

Agree that it's better to keep it simple. Revised :)

Have moved a part with the exceptions hook to #898