Logger

[wclodius2]

The main files to focus on are src/stdlib_logger.f90, src/tests/logger/test_stdlib_logger.f90, and doc/specs/stdlib_logger.md. I have also updated the CMakeLists.txt, and Makefile.manual so they should compile with this branch, but my original testing was with a download of the current main branch and not this older one. Note I had some problems with the other source code when compiled with a couple years old version of ifort, but no problems with gfortran.

[milancurcic]

Great, thanks William, I will review it.

[jvdp1]

Thank you @wclodius2 for this PR. I will review it this weekend

[wclodius2]

Two reviewers will be great.

…

On Sep 3, 2020, at 1:04 PM, Jeremie Vandenplas ***@***.***> wrote: Thank you @wclodius2 <https://github.com/wclodius2> for this PR. I will review it this weekend — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APTQDOTEVLEKXSRSLZKKIELSD7SFRANCNFSM4QSNH5WA>.

[jvdp1]

Thank you @wclodius2 for this PR. I reviewed the specs, and I like this proposition for a logger. I added several comments. Some of them could be applied at other parts of the specs.
I will review the rest later.

[jvdp1]

Here are some additional comments

[jvdp1]

It was indeed to be more explicit, but also to follow the same logic as for the declaration of variables inside the derived type (private default), and of the module (private default too). Le dim. 6 sept. 2020 à 22:39, Milan Curcic <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In src/stdlib_logger.f90 <#228 (comment)>: > +! procedure, pass(self) :: assert + procedure, pass(self) :: add_log_file + procedure, pass(self) :: add_log_unit + procedure, pass(self) :: configuration + procedure, pass(self) :: configure + procedure, pass(self) :: log_error + procedure, pass(self) :: log_information + procedure, pass(self) :: log_io_error + procedure, pass(self) :: log_message + procedure, pass(self) :: log_text_error + procedure, pass(self) :: log_units_assigned + procedure, pass(self) :: log_warning + procedure, pass(self) :: remove_log_unit I don't think this change is needed. Type-bound methods are public by default unless explicitly set to private, right? Unless that with this change you aim to be more explicit. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5RO7H2Y4U25BP64TBWS3LSEPXQDANCNFSM4QSNH5WA> .

[milancurcic]

It was indeed to be more explicit, but also to follow the same logic as for the declaration of variables inside the derived type (private default), and of the module (private default too).

You're right, we should be consistent. Your suggested change is fine with me.

[milancurcic]

I think I have committed all outstanding suggestions by @jvdp1 (Thank you!).

Considering the size of this PR and small number of reviewers, let's leave it open for another week or so before merging.

[jvdp1]

Considering the size of this PR and small number of reviewers, let's leave it open for another week or so before merging.

I agree. I think it would be good to discuss it during the next monthly call.

[wclodius2]

I have needed a breather, and you two have also put in a lot of useful work. One additional change I have thought of doing is to add an optional suffix argument so that stat, instate, errmsg, and iomsg can be output as part of one single write statement. However while they would be part of one write statement they would be output as separate lines using the new_line function. But I can put that off if you want. FWIW I intend to attend the monthly call this Friday.

[jvdp1]

I have needed a breather, and you two have also put in a lot of useful work. One additional change I have thought of doing is to add an optional suffix argument so that stat, instate, errmsg, and iomsg can be output as part of one single write statement. However while they would be part of one write statement they would be output as separate lines using the new_line function.

This could solve indeed some potential issues with parallel programming. This proposition would be more robust IMO.
Anyway, I am happy with the actual code, and this option could be added in a next PR.

[milancurcic]

If there are no objections, I'll merge this tomorrow into master.

If anybody would like more time to review, just write here.

[certik]

Here is my review:

• Overall I am fine with merging as is.

• This is what I would consider a high level API. If there is demand, we can provide a low level API that does not use a derived type, thus passing all configuration as arguments. In this case such low level API might not be as useful.

• Does this design allow to extend it in the future to write all logs into a string? That would be helpful for things like interactive usage in a Jupyter notebook, to send all logs over it it from the kernel.

• I don't have an opinion on the more general issue of: unlike some of the other functionality in stdlib, such as statistics, linear algebra, special functions etc. where there is a lot of prior art (in scipy, matlab, etc.) regarding their API, this logger API is in some sense designed in this PR. As such, it is not clear this is the best design, and it doesn't have usage yet. Down the road, for such new APIs, it might make sense to implement them as fpm packages first. And only later "standardize" in stdlib. However, at the same time, this logger API is experimental. So I think there is no harm including it also, with the understanding that the API might change in the future as we gain experience using it.

[wclodius2]

Answering each of the points; * Good * I have trouble seeing the usefulness of a low level API, for a logger. FWIW the loggers in FLIBS, Futility, SLATEC, and Python are all higher level than what you describe. * I have trouble interpreting “write all logs into a string”. What functionality do you see over redirecting standard output? - Do you mean all logs into a single string? With Fortran’s concatenation semantics that can be O(N**2), though with growable strings doubling when necessary, I guess that could be O(N ln N). - Do you mean each log command into its own string? Easily do able, but I don’t see the use. - Do you mean each log to a list of strings, to be concatenated on demand? * FWIW Python,,does have a logger, https://docs.python.org/3/library/logging.html <https://docs.python.org/3/library/logging.html>. It is impractical to do its “child” hierarchy in Fortran. we could add the option of turning off output depending on the error level: error > warning > info.

…

On Sep 28, 2020, at 10:04 AM, Ondřej Čertík ***@***.***> wrote: @certik approved this pull request. Here is my review: Overall I am fine with merging as is. This is what I would consider a high level API. If there is demand, we can provide a low level API that does not use a derived type, thus passing all configuration as arguments. In this case such low level API might not be as useful. Does this design allow to extend it in the future to write all logs into a string? That would be helpful for things like interactive usage in a Jupyter notebook, to send all logs over it it from the kernel. I don't have an opinion on the more general issue of: unlike some of the other functionality in stdlib, such as statistics, linear algebra, special functions etc. where there is a lot of prior art (in scipy, matlab, etc.) regarding their API, this logger API is in some sense designed in this PR. As such, it is not clear this is the best design, and it doesn't have usage yet. Down the road, for such new APIs, it might make sense to implement them as fpm packages first. And only later "standardize" in stdlib. However, at the same time, this logger API is experimental. So I think there is no harm including it also, with the understanding that the API might change in the future as we gain experience using it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#228 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APTQDOVABFA7ICC5ZRO7NU3SICX2NANCNFSM4QSNH5WA>.

[certik]

I agree a lower level API might not be useful.

Regarding saving to a string, Python has a concept of Handlers: https://docs.python.org/3/howto/logging.html#useful-handlers, which allow to specify where the log messages are being written. You might decide to send them over a network, or do something else with them, and use standard output separately. So you cannot just redirect a standard output. The best interface would be to provide a user call back every time a message is written. That way users can overwrite this and do whatever they want.

[wclodius2]

What could be done for "handlers" is define for the logger_type an internal array of procedure pointers with an interface say

interface
    subroutine logger_handle( string )
        character(*), intent(in) :: string
    end subroutine logger_handler
end interface

or define an array of class handler with a method

interface
    subroutine logger_handle( handle, string )
        class(handler), intent(in) :: handle
        character(*), intent(in) :: string
    end subroutine logger_handler
end interface

[milancurcic]

Merging, thank you all and especially @wclodius2!