Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature logging #244

Closed
wants to merge 17 commits into from
Closed

Feature logging #244

wants to merge 17 commits into from

Conversation

cpwnd
Copy link

@cpwnd cpwnd commented Apr 2, 2020

Hey I followed your discussion in #239 and went ahead to implement the standard logging library.
If you wan't to use the code, I'm happy to contribute.

What is already done

All print calls are replaced with a logging.info call. I don't know if the info level is always appropriate.
I changed your logging code in analyser.py (EDIT i mean objectivefunctions.py) to use the new main logger.
There is one print call in the database package. I don't know if its really necessary.

What has to be done

  • Add cli option and standard behavior: Before merging there should be a defined standard behavior for the logging output. And also add a flag --verbose/debug/quiet or something like that, to toggle the standard behavior.
    So we can met the requirements of the issue.
    I wasn't quite sure how to add a new cli option that.
    If you point me to the right direction I can add code for e.g. (1) --quiet => no console output/everything is printed to the file (2) default (no option) => info level and (3) --debug which then prints all .debug(...) messages as well.
    Also printing to the file can be disabled too.
  • Logging format: Also an question for me is: We can change the current format of the logging messages, whether the current format suits your needs. Is the current format ok with you:

https://github.com/cpwnd/spotpy/blob/15e219692f6d7d8a9553264a5830a92b481cde82/spotpy/spotpylogging.py#L16

Not implemented in this pr

In my opinion would be possible next steps, that can be adressed with future pull requests:

  1. The logging messages are now all at level info. They can be more nuanced, to have different outputs for different use cases. E.g. the individuals steps of an algorithm can be on the debug level. (optional) @thouska remove/change logging calls with \n they prevent a nice reading flow (just my opinion)
  2. At the moment, the standard file name for the logging output is spotpy.log. This is used for subsequent calls and every run is appended in this file. Of course this can be changed, maybe with a /log directory and a timestamp-suffixed filename.
  3. Maybe you want to be able to use a file config, e.g. to control the log level or the output file. see https://docs.python.org/3/howto/logging.html#configuring-logging

At the moment I tested only with different tutorials from the examples folder.

Hope to hear from you soon :)

@thouska
Copy link
Owner

thouska commented Apr 2, 2020

Hi @cpwnd ,
super nice, thank you for this cool contribution! I tested it and it runs smothely on my PC.

Three minor things:

  • I get every logging message twice in my console, however, the spotpy.log file has only one entry for each message
  • The path for the logfile should be possible to set (I assume this can/will be done when working on the cli option).
  • In my opinion, the time stamp is not necessary.

I will work on you PR and implement different levels of the logging. Indeed some of the stuff from the algorithms can be seen as --debug

@philippkraft what do you think about the cli option, logging format and having a logging file?

@cpwnd
Copy link
Author

cpwnd commented May 28, 2021

Hey @thouska, you can now set logfile, logdir and verbosity as "quiet" via cli parameters. I tested it via your suggested way:

from spotpy.cli import main
from spotpy.examples.spot_setup_rosenbrock import spot_setup

setup = spot_setup()
main(setup)

And then start it via command line e.g. $ python script.py --logfile test.log --quiet

The path for the logfile should be possible to set (I assume this can/will be done when working on the cli option).

I think we implemented now all remaining points.

EDIT:
See the help message for an explanation on the cli options

  -q, --quiet                     Suppress any logging messages to stdout
                                  during computation

  --logfile FILE                  Logging messages will be written to this
                                  file. If already exists, contents are
                                  appended. A default filename is created with
                                  a current timestamp

  --logdir DIRECTORY              Logging files will be written to this
                                  directory. Default is "."

@cpwnd
Copy link
Author

cpwnd commented May 28, 2021

Is this line important? Thats the last print statement remaining.

Maybe it can be replaced by the logger. If yes, then the database/__init__ also needs a logger object.

@coveralls
Copy link

coveralls commented Jun 1, 2021

Pull Request Test Coverage Report for Build 921

  • 216 of 254 (85.04%) changed or added relevant lines in 25 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 84.779%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotpy/algorithms/mle.py 3 4 75.0%
spotpy/algorithms/sa.py 1 2 50.0%
spotpy/database/hdf5.py 0 1 0.0%
spotpy/objectivefunctions.py 19 20 95.0%
spotpy/algorithms/_algorithm.py 26 28 92.86%
spotpy/algorithms/demcz.py 9 11 81.82%
spotpy/algorithms/fast.py 5 7 71.43%
spotpy/algorithms/rope.py 6 8 75.0%
spotpy/analyser.py 31 33 93.94%
spotpy/algorithms/abc.py 10 13 76.92%
Files with Coverage Reduction New Missed Lines %
spotpy/algorithms/abc.py 1 84.87%
Totals Coverage Status
Change from base Build 910: 0.2%
Covered Lines: 4155
Relevant Lines: 4901

💛 - Coveralls

cpwnd added 2 commits June 6, 2021 13:00
* Remove comments
* Replaces last print and log messages with backspace char
@cpwnd
Copy link
Author

cpwnd commented Jun 9, 2021

With the last two commits, at least for me, this is finished. Can be merged into master now.

@thouska
Copy link
Owner

thouska commented Jun 15, 2021

Hi @cpwnd,

thx for pushing this new feature. I tested it on my device and it works almost perfect. However, there is one strange error:
Everytime it is started for the frst time in an interactive console, any of the example errors with:

Traceback (most recent call last):
File "...\spotpy\examples\tutorial_parallel_computing_hymod.py", line 44, in
sampler=spotpy.algorithms.mc(spot_setup, dbname='Parallel_hymod', dbformat='csv',
File "...\spotpy\algorithms\mc.py", line 53, in init
super(mc, self).init(*args, **kwargs)
File "...\spotpy\algorithms_algorithm.py", line 221, in init
self.logger = spotpylogging.instantiate_logger(self.class.name, quiet, logfile, logdir)
File "...\spotpy\spotpylogging.py", line 49, in instantiate_logger
handler_file = logging.FileHandler(path_to_logfile)
File "...\anaconda3\lib\logging_init_.py", line 1143, in init
StreamHandler.init(self, self.open())
File "...\anaconda3\lib\logging_init
.py", line 1172, in _open
return open(self.baseFilename, self.mode, encoding=self.encoding)

OSError: [Errno 22] Invalid argument: '...\spotpy\examples\2021-06-15-spotpy.log'`

If I restated the code from there, it works without any error. Do you have any idea what could cause that behavior?

@cpwnd
Copy link
Author

cpwnd commented Jun 16, 2021 via email

@thouska
Copy link
Owner

thouska commented Jun 21, 2021

I start the scripts in Anaconda's spyder editor.
I will try, how it works outside of spyer and keep you posted.

@cpwnd
Copy link
Author

cpwnd commented Jul 12, 2021

@thouska please check the branch again for your Windwos Anaconda setup. I replaced the logging paths now with pathlib.PurePaths, which are OS agnostic and should work everywhere.

@cpwnd cpwnd mentioned this pull request Sep 18, 2022
@cpwnd
Copy link
Author

cpwnd commented Sep 18, 2022

Closing this in favor of #293

@cpwnd cpwnd closed this Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants