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

• 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.

[davide-f]

Nice draft :D few little comments! :)

[davide-f]

Having logger and raise is a bit annoying.
Could you try if logger.critical(.) or logger.exception(.) also trigger an exit of the execution?
I'd avoid the execution of "raise" because it still leads to dirty logging.
In the general exception tracker, the textual description of the exception str(exception) may be added as well.

[ekatef]

Have played around logger, but there seems to be no way to stop execution just by logging tools :( As far as I was able to find, both logger.critical( ) and logger.exception( ) have a purely information function.

Agree that raise doesn't look nice. But executing is anyway stopped when geopandas tries to read a non-existing file. So, probably we can just make an error message more clear. What do you think?

[davide-f]

An approach may be to use sys.exit() to close the execution forcefully.

Regarding the exception trace, I just realized that there is the option "exc_info=True" that may automatically log the trace.
Ok also as is now, it is is nicer in the log we could also take it into consideration.

What do you think?

[ekatef]

I think, implementation with exc_info=True is definitely nicer :D Revised and ddded sys.exit(1), which seems to give much more clear logging output.

[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:

pypsa-earth/scripts/build_shapes.py

Lines 577 to 580 in b205e83

	if not os.path.exists(GDP_nc):
	raise Exception(
	f"File {name_file_nc} not found, please download it from https://datadryad.org/stash/dataset/doi:10.5061/dryad.dk1j0 and copy it in {os.path.dirname(GDP_nc)}"
	)

. 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