Skip to content

Draft: setup prototype service structure and implement working thread-… #2

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

Closed
wants to merge 20 commits into from

Conversation

sly2j
Copy link
Contributor

@sly2j sly2j commented Sep 2, 2022

Briefly, what does this PR introduce?

Introduce a mechanism for minimal services for the stand-alone algorithms, with hooks to be configured by a calling framework

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • [ X] New feature (initial prototype)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

@sly2j sly2j changed the title WIP - setup prototype service structure and implement working thread-… Draft: setup prototype service structure and implement working thread-… Sep 2, 2022
@sly2j sly2j marked this pull request as draft September 2, 2022 20:49
@faustus123
Copy link

Just a quick comment since it looks like a lot of work went into the logger. I apologize since we are clearly missing some communication here. The logger has been discussed at the developer meeting, but not really documented in a way that could be easily followed outside of that.

We have started moving toward spdlog in EICrecon. There is some documentation on its implementation as a service https://github.com/eic/EICrecon#logging .

This actually came up at the key4hep meeting this week where they said they were needing to implement a standard logger and asked what we were doing at EIC. I told them we were looking at spdlog and a few people piped up and said this was not the first time it was suggested. They seemed fairly positive at looking at it as a solution.

@sly2j
Copy link
Contributor Author

sly2j commented Sep 2, 2022

I think spdlog is a wonderful choice for the logger for the framework. I have some experience working with it in the past and can highly recommend it.

The purpose of this functionality is a bit different, as algorithms is supposed to be just a library of, well, algorithms, without taking on the responsibility and services of a full-fledged framework. It is an important design criterion that we are fully framework-agnostic. For this we need to have an API that the algorithms can use to interface with the required services, with the necessary hooks for the framework to link them to the actual framework services. This API is meant to just be a thin wrapper around the backend services provided by the framework.

I did write a minimal logger (just fmt::print) so I can do stand-alone testing, but in the end the framework's interface with algorithms is supposed to register a (presumably thread-safe) logger action with algorithms. This logger action will be provided with logLevel (an enum), caller (a string), and msg (also a string). This should be enough to create a meaningful message with the framework's logger. Here is some relevant code:

This is the signature of the log action:

using LogAction = std::function<void(LogLevel, std::string_view, std::string_view)>;

and here is how to register a different log action (should be done at the framework boundary):

algorithms::LogSvc::instance().action(YourLogFunctionHere);

I tried to keep this extremely simple. Do you think there is more info needed to link this to the logger service in JANA2?

@faustus123
Copy link

This sounds reasonable. I was just concerned about duplication of effort and it sounds like there is none. Thanks for the detailed explanation.

@faustus123
Copy link

So I was looking at this and ran into an issue. The spdlog package removed support for stream loggers years ago and put forth some pretty reasonable arguments for doing so. People asked about wrapping spdlog to add back in stream support and the response was (parphrased) "you can, but it is probably not a good idea for the same reasons it was removed". Thus, to support spdlog logging, you need printf-style calls that consist of a format string and a variable number of additional arguments. There seems to be only two ways to handle variable arguments: variadic templates and the old C-style va_list. The latter is probably a non-starter.

So to support this type of logger we would need the interface class to have methods using variadic templates. The methods can't be virtual since you can't have a virtual templated method (at least in C++17 AFIK). This kind of blocks you from using a standard inheritance model of implementing an interface class.

The only other option is to use a templated interface model where the template parameter implements the underlying logger system. The problem here is that the only way to include a template parameter in the logger class that a user can determine is to also make the algorithm class a template. Below is an example where I define a simple interface class and then both a templated and non-templated version of the algorithm class.

// Interface class
template< typename Tlogger = spdlog::logger>
class ILogger{
public:

    template<typename... Args>
    void debug(std::string_view format_string,Args... args){
        logger->debug( format_string, std::forward<Args>(args)... );
    }

    Tlogger *logger;
};

// Option 1: algorithm class is non-templated which means logger is always type spdlog::logger
class MyAlgo:public ILogger<>{

    void execute(void){
        debug("Testing {}", 3.14);
    }
};

// Option 2: algorithm class is templated so end user can specify logger type
template<typename Tlogger>
class MyAlgo:public ILogger<TLogger>{

    void execute(void){
        debug("Testing {}", 3.14);
    }
};

Option 2 could work, but it forces making all algorithms templated simply for the sake of the logger. Somehow that bothers me. Maybe there is something I'm missing.

At the Developer's meeting today we discussed this and concluded there was no obviously good solution. So, we will move forward for now by having the generic algorithms hold a shared pointer to the spdlog logger that they should use . This can be set by the algorithm caller. If you (or anyone) can come up with a clever solution for abstracting out a printf style logger, then we can retrofit at that point.

@sly2j
Copy link
Contributor Author

sly2j commented Sep 13, 2022

For future reference, David and I had a meeting to chat about the path forward. After the meeting we don't see any showstoppers. The plan is to:

  1. Finish algorithms
  2. Implement the interface in Juggler and adapt a single Juggler algorithm
  3. Use the Juggler interface layer as a guide to implement the JANA2 interface layer.

@sly2j sly2j mentioned this pull request Sep 14, 2022
14 tasks
@sly2j
Copy link
Contributor Author

sly2j commented Sep 14, 2022

Superseded by #3

@sly2j sly2j closed this Sep 14, 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.

2 participants