Add logging infrastructure to all algorithms #824
Conversation
|
Apologies for the high code volume, the vast majority is boilerplate. |
stephenswat
changed the title
Would it be an acceptable solution for you if the logger had a default parameter so the external behaviour of the constructors would remain identical to the current behaviour? |
|
That would be acceptable |
stephenswat
force-pushed
the
feat/logging
branch
from
e939705 to
ea034a0
Compare
|
There we go, all the algorithms now have default loggers so the external behaviour is unchanged, and the code now uses the ACTS core only conditionally, i.e. if it is already available; so the code continues to work without ACTS core. |
There was a problem hiding this comment.
This is pretty messy now.
I suggest we just add ActsCore as a dependency here and start making our code more unified and less complex, rather than increasing complexity.
I think littering our code with preprocessor instructions and duplicating the infrastructure without a good reason is a pretty bad idea and we should refrain from doing this to avoid tech debt.
There was a problem hiding this comment.
Without having gone through the entire thing completely...
I agree with @beomki-yeo that I don't want the current traccc::core algorithm to depend on ActsCore. At the same time, clearly future ActsSYCL, ActsCUDA, etc. libraries will publicly depend on ActsCore. So it's a good idea to use a carbon copy of Acts's logger code.
But I would just always use the carbon copy. As long as we have to have a copy in our code, what do we win by sometimes not using it?
I would remove the conditional compilation flag, and just use the "imported" logger as-is.
Now... did you think at all about logging (error reporting...) in the GPU code? Since to me that is the actually interesting part here. For the host-side logging it was clear that we would follow what Acts has been doing since forever. (Which itself is a variation on what Gaudi does. Not sure if you've seen Heretic... Its main premise applies here as well. 😆)
core/include/traccc/ambiguity_resolution/greedy_ambiguity_resolution_algorithm.hpp
Outdated
Show resolved
Hide resolved
The problem is that the ACTS and Athena code will be passing in the actual ACTS logger, not our carbon copy. And you and I know that those types are functionally the same, but the compiler obviously doesn't - so it will reject the code with a compilation error. |
AFAIK nobody has come up with a better solution than what I proposed in #640, so as far as I am concerned that is going to be the way to go unless someone comes up with something really fancy! |
Not quite what I meant. 🤔 Though it's good that you reminded me that we'll need some generic functionality for "monitoring" as well. I was rather thinking of something along the lines of the vecmem logging code. https://github.com/acts-project/vecmem/blob/main/core/include/vecmem/utils/debug.hpp But I don't think that quite scales to these algorithms. That's sort of good enough for debugging low-level functionality in special builds of the code, but in this project I would rather go with a combination of
We don't have to tackle those here. But we should start more seriously thinking about them. 🤔 |
Wait, why? Once the code is actually merged into acts-project/acts, then of course we will just have a single class. The current (or maybe slightly improved) But as long as Athena and Acts use this project as an external, why couldn't they construct its own loggers? As long as this carbon copy can forward its messages to an outside logger like |
|
@krasznaa why the duplication and tech debt now though? Just adopting the core logger type at this poin we'd be converging things already, rather than increasing divergence. Unless your argument is NIH, this has basically no downsides at this point. We will be adding code now with the only goal of removing it for... what exactly? We'll be adding wrapper code to ACTS, we'll be adding wrapper code to Athena, we'll be adding wrapper code for wrapper code. Or, we could just use the one thing that does exactly what we need and we have at hand already and is already integrated. I'm sorry to make such a fuzz about this, but there's no good reason not to use a single logging system at this point. We should stop thinking about this as a separate project at some point, and I think that point is now. |
stephenswat
force-pushed
the
feat/logging
branch
from
ea034a0 to
662adf1
Compare
|
Okay, I've addressed all the topics of conversation. Clearly we have reached some impasse when it comes to the reliance on the ACTS Core library, so I think the solution of selecting either the ACTS logger or our own carbon copy of it is the best solution until the integration continues. I'd like to move towards merging this now. |
This commit adds ACTS-style loggers to all existing traccc algorithms, and requires that all future algorithms have the same. It also carefully threads logger objects through the computation chain. Currently, little is added in the way of useful logging outputs, as that can happen in a future commit: this is just infrastructure.
stephenswat
force-pushed
the
feat/logging
branch
from
662adf1 to
b8c3578
Compare
|
| std::unique_ptr<const Logger> m_logger; | ||
|
|
||
| const Logger& logger() const override { return *m_logger; } |
There was a problem hiding this comment.
Let's talk in the afternoon in person. Since I still very much dislike the amount of boilerplate code added in this PR. 😦 Since this is something that we'll keep as a design definitely for a while, I do want to come to something that we can all live with...
There was a problem hiding this comment.
This is the ACTS design, if we want to use ACTS_XXX macros (and I strongly think we do), this is what it'll be. So, in a sense, the design is already decided, therefore not much to discuss.
There was a problem hiding this comment.
There has to be a way of factorizing this with a mix-in class. I have a very hard time accepting that every single client has to have their own member functions defined separately. We stopped doing that in Athena a decade ago...
There was a problem hiding this comment.
That I don't have an opinion on, beyond trying to avoid multiple-inheritance if possible.
This commit adds ACTS-style loggers to all existing traccc algorithms, and requires that all future algorithms have the same. It also carefully threads logger objects through the computation chain. Currently, little is added in the way of useful logging outputs, as that can happen in a future commit: this is just infrastructure.