[QC-1298] Extensible interface for accessing QC data sources #2589

justonedev1 · 2025-07-31T13:15:46Z

This is just first version of Data structure, that should be able to ingest everything. @knopers8 if you can take a look and say whether it is at least something to build upon.

Usage can be seen in SkeletonCheck.cxx

justonedev1 · 2025-08-07T17:51:04Z

@knopers8 version 2. I am not saying that it is 100% done, but it should fulfill everything we talked about, so you can take a look as most of the functionality is there.

knopers8 · 2025-08-08T12:44:14Z

For the record, we discussed the PR in person and agreed to extend the interface adoption to Aggregators and add support for inserting raw pointers. I will do a full review once we have these two points complete.

knopers8

Thanks a lot! Indeed I am not happy that we leak some data storage details from Check and Aggregator into Data, but it seems we cannot avoid it. Otherwise, the interface is looking very promising.

I would ask you for the following:

See what could be moved to compiled source files. This interface will be included in hundreds of files, so it should be well optimized for compilation time. Consider having compiled versions for MonitorObject and QualityObject specializations.
See what could be hidden from the main header file into an .inl for easier reading by the users.
Prepare helpers (and feel free to disagree with the proposals):
- iterateMonitorObjects()
- iterateMonitorObjects(std::string_view taskName)
- getMonitorObject<StoredType = MonitorObject>(std::string_view objectName, std::string_view taskName) // no risk of obj name collisions
- getMonitorObject<StoredType = MonitorObject>(std::string_view objectName) // gets first matching MO!!!
- `getMonitorObject(std::string_view objectName) // gets the underlying object if StoredType is anything else than MonitorObject
- getMonitorObject<StoredType>(std::string_view objectName, std::string_view taskName) // like above, but no risk of obj name collisions
- similar helpers for QOs
- to be discussed also with Barth: maybe these helpers should be a part of Data. the advantage is that users won't have to pass Data as an argument. The disadvantage is polluting the interface...

justonedev1 · 2025-08-11T15:15:23Z

okay, thanks I will add the helpers and address other necessary additions.

knopers8

Thanks, just a few more suggestions, but this looks already good.

I propose that on Tuesday QC weekly we brainstorm the three of us:

interface names
whether to move helpers to Data or keep it in DataAdapters
whether returning std::optional<std::reference_wrapper> is the easiest and safest we can do for the users.

@Barthelemy Could you perhaps have a look as well?

knopers8 · 2025-08-15T08:46:14Z

Framework/include/QualityControl/Data.inl

+template <typename T>
+static constexpr auto any_to_specific = std::views::transform([](const auto& pair) -> std::pair<std::string_view, const T*> { return { pair.first, any_cast_try_shared_raw_ptr<T>(pair.second) }; });
+
+static constexpr auto filter_nullptr_in_pair = std::views::filter([](const auto& pair) -> bool { return pair.second != nullptr; });
+
+static constexpr auto filter_nullptr = std::views::filter([](const auto* ptr) -> bool { return ptr != nullptr; });
+
+static constexpr auto pair_to_reference = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });
+
+static constexpr auto pair_to_value = std::views::transform([](const auto& pair) -> const auto* { return pair.second; });
+
+static constexpr auto pointer_to_reference = std::views::transform([](const auto* ptr) -> const auto& { return *ptr; });


consider breaking into multiple lines for easier readability, e.g.

template <typename T> static constexpr auto any_to_specific = std::views::transform( [](const auto& pair) -> std::pair<std::string_view, const T*> { return { pair.first, any_cast_try_shared_raw_ptr<T>(pair.second) }; });

knopers8 · 2025-08-15T08:47:38Z

Framework/include/QualityControl/Data.inl

+
+static constexpr auto filter_nullptr = std::views::filter([](const auto* ptr) -> bool { return ptr != nullptr; });
+
+static constexpr auto pair_to_reference = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });


Suggested change

static constexpr auto pair_to_reference = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });

static constexpr auto pair_to_value_const_ref = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });

knopers8 · 2025-08-15T09:25:44Z

Modules/Skeleton/src/SkeletonCheck.cxx

+    return result;
+  }
+
+  const TH1& histogram = histOpt.value().get();


I have to admit I am not a great fan of .value().get(), but I don't see a better option at the moment than std::reference_wrapper. This being said, here it should automatically cast to const TH1& without get() and if we go this way, the examples should use implicit casting.

I agree with the implicit operator... I was using explicit here for 2 reasons: I overlooked implicit operator in docs and I personally prefer to use explicit calls, so I am not surprised how something happened (eg. std::optional.has_value(), .value(), or comparing raw pointers to nullptr instead of just using !)...

One thing that I would like to ask is: should I use explicit type (const TH1& in this case), or const auto&? What do you think is better for the example for users?

according to my trials on godbolt, const auto& was not enough to invoke implicit cast, so I think we have to use const TH1&. Perhaps the first case it is used in SkeletonCheck and SkeletonAggregator should be annotated with a comment that we are in fact casting from a reference wrapper.

knopers8 · 2025-08-15T12:12:25Z

Framework/include/QualityControl/DataAdapters.h

+template <typename StoredType = MonitorObject>
+std::optional<std::reference_wrapper<const StoredType>> getMonitorObject(const Data& data, std::string_view objectName);
+
+inline auto iterateQualityObjects(const Data& data);


i think we need also inline auto iterateQualityObjects(const Data& data, std::string_view checkName);. One Check may return several QOs if the policy OnEachSeparately is selected.

Barthelemy

Thank you, this is very nice. Really great !

I have put a few comments and we will discuss it during this afternoon's meeting.
In general, I would appreciate if there were more comments to explain what the intent is and why it is done this or that way.

I am also pondering the benefit of having a separate file for the inline stuff.

Barthelemy · 2025-08-19T07:36:08Z

Modules/Skeleton/include/Skeleton/SkeletonCheck.h


  // Override interface
  void configure() override;
  Quality check(std::map<std::string, std::shared_ptr<MonitorObject>>* moMap) override;


why have both check methods ?
Do we want users to have both ?

Just to clarify what I mean: I would expect users to only implement the check that takes data if they write a new Check or if they want to move to the new interface.
And if they don't want to do anything, then they just keep the old implementation.

Barthelemy · 2025-08-19T07:39:39Z

Modules/Skeleton/src/SkeletonCheck.cxx

+    return result;
+  }
+
+  const TH1& histogram = histOpt.value().get();


Barthelemy · 2025-08-19T07:40:22Z

Framework/include/QualityControl/Data.h

+// or submit itself to any jurisdiction.
+
+///
+/// \file   Data.h


That's a very generic name

Barthelemy · 2025-08-19T07:41:10Z

Framework/include/QualityControl/Data.h

+concept invocable_r = std::invocable<Function, Args...> && std::same_as<std::invoke_result_t<Function, Args...>, Result>;
+
+template <typename ContainerMap>
+class DataGeneric


Perhaps you could have a comment to explain what its purpose is and how it fits in the QC .

Barthelemy · 2025-08-19T07:42:36Z

Framework/include/QualityControl/Data.h

+  }
+};
+
+using transparent_unordered_map = std::unordered_map<std::string, std::any, StringHash, std::equal_to<>>;


A bit of explanation of what we are doing here would be helpful.

Barthelemy · 2025-08-19T09:09:18Z

Framework/include/QualityControl/Data.inl

+// or submit itself to any jurisdiction.
+
+///
+/// \file   Data.inl


As for Data.h, it would be helpful to have a bit more comments about the goal and intent of these functions.

justonedev1 · 2025-08-19T12:11:08Z

@Barthelemy

I have put a few comments and we will discuss it during this afternoon's meeting. In general, I would appreciate if there were more comments to explain what the intent is and why it is done this or that way.

I will add more comments and documentation about everything when the code is finalised. Neither me nor Piotr were sure about naming, so if you don't like some naming go ahead and propose better names, this was built as a POC, so naming might be all over the place as it was being changed as I went along and I didn't make "finalising pass" to check if the names are as best as they can be

I am also pondering the benefit of having a separate file for the inline stuff.

Do you mean .inl files?

knopers8 · 2025-08-19T12:47:38Z

I am also pondering the benefit of having a separate file for the inline stuff.

Do you mean .inl files?

@Barthelemy I asked Michal to do it and the reason was to make this header as much readable as possible for the users, so they are not lost in the implementation details when checking how they can use the interface.

Barthelemy · 2025-08-19T13:08:35Z

@Barthelemy I asked Michal to do it and the reason was to make this header as much readable as possible for the users, so they are not lost in the implementation details when checking how they can use the interface.

@knopers8 ok, understood.

knopers8

I am wondering if we still need QCInputsGeneric, now that you found the most suitable container and it's unlikely we would change it or use more than one. What do you think about merging it back with QCInputs to simplify the code?

knopers8 · 2025-09-08T07:48:20Z

Framework/include/QualityControl/CheckInterface.h

  virtual ~CheckInterface() = default;

  /// \brief Returns the quality associated with these objects.
  ///


please add a note that users should prefer the other one and this one is for backwards-compatibility.

knopers8 · 2025-09-08T07:51:39Z

Framework/include/QualityControl/QCInputs.h

+  /// \returns Optional reference to const Result if found desired item of type Result.
+  /// \par Example
+  /// \code{.cpp}
+  /// if (auto opt = data.get<unsigned>("count")) {


i would propose to use MonitorObject in examples so they are more real-life.

knopers8 · 2025-09-08T08:00:29Z

Framework/include/QualityControl/QCInputs.h

+  /// \returns Range of const references to stored Result instances.
+  /// \par Example
+  /// \code{.cpp}
+  /// for (auto& val : data.iterateByResultype<unsigned>()) {


the example uses a different method name

knopers8 · 2025-09-08T09:13:56Z

Framework/include/QualityControl/QCInputs.h

+/// \brief Transparent hash functor for string and string_view.
+///
+/// Enables heterogeneous lookup in unordered maps keyed by std::string.
+struct StringHash {


Perhaps this could be moved to inl as well? struct StringHash; declaration might be only needed.

knopers8 · 2025-09-08T09:39:19Z

Framework/include/QualityControl/QCInputsAdapters.h

+inline auto iterateMonitorObjects(const QCInputs& data, std::string_view taskName);
+
+/// \brief Retrieve the first MonitorObject of type StoredType matching both name and task.
+/// \tparam StoredType Type of MonitorObject or subclass to retrieve.


"subclass" typically means "derived class", how about "stored class" or "wrapped class"?

There are other uses of "subclass" in this file".

knopers8 · 2025-09-08T10:25:48Z

Framework/include/QualityControl/QCInputsAdapters.inl

+inline auto iterateQualityObjects(const QCInputs& data, std::string_view checkName)
+{
+  const auto filterQOByName = [checkName](const auto& pair) {
+    return std::string_view(pair.second->getName()) == checkName;


Use getCheckName. getName can be different than getCheckName when OnEachSeparately update policy was used for a Check. In such case, a separate QO is produced for each MO that is an input, so you get CheckA/MO1, Check/MO2, ... See the implementation of QualityObject::getName

I was looking at it and I thought that for some reason you want to specifically use getName and not getCheckName... that is my bad, sorry

knopers8 · 2025-09-08T10:48:58Z

Framework/src/QCInputsAdapters.cxx

+namespace o2::quality_control::core
+{
+
+std::optional<std::reference_wrapper<const QualityObject>> getQualityObject(const QCInputs& data, std::string_view objectName)


The declaration mentions checkName as the argument, here it's objectName. These might not be the same as mentioned earlier. Consequently, in the implementation, use getCheckName.

knopers8 · 2025-09-08T10:52:12Z

Modules/Skeleton/src/SkeletonAggregator.cxx

-  for (const auto& item : qoMap) {
-    ILOG(Info, Devel) << "Object: " << (*item.second) << ENDM;
-  }
+  ILOG(Info, Devel) << "   received a data of size : " << data.size() << ENDM;


Suggested change

ILOG(Info, Devel) << " received a data of size : " << data.size() << ENDM;

ILOG(Info, Devel) << " received data of size : " << data.size() << ENDM;

Data is plural of datum.

justonedev1 · 2025-09-08T13:13:14Z

I am wondering if we still need QCInputsGeneric, now that you found the most suitable container and it's unlikely we would change it or use more than one. What do you think about merging it back with QCInputs to simplify the code?

Problem with that is that I would need to delete the benchmark code as well, which would mean that the reason for unordered_map usage might be lost in time and other containers that were tried would be certainly forgotten. But if you think that this is ok or not necessary to keep, I will delete it.

knopers8 · 2025-09-08T13:40:57Z

I am wondering if we still need QCInputsGeneric, now that you found the most suitable container and it's unlikely we would change it or use more than one. What do you think about merging it back with QCInputs to simplify the code?

Problem with that is that I would need to delete the benchmark code as well, which would mean that the reason for unordered_map usage might be lost in time and other containers that were tried would be certainly forgotten. But if you think that this is ok or not necessary to keep, I will delete it.

I think you can leave the benchmark for the selected container and add a comment next to using container_t = std::unordered_map<std::string, std::any, StringHash, std::equal_to<>> which mentions that it was the fastest from x, y and z. You can also leave the output from benchmarks as an attachment to the ticket.

knopers8

Thank you!

justonedev1 requested review from Barthelemy and knopers8 as code owners July 31, 2025 13:15

justonedev1 force-pushed the QC-1298 branch 5 times, most recently from 6b6734d to c37c37e Compare August 7, 2025 17:50

justonedev1 force-pushed the QC-1298 branch from 6c6d49f to a77dc7f Compare August 8, 2025 14:46

knopers8 requested changes Aug 11, 2025

View reviewed changes

justonedev1 force-pushed the QC-1298 branch 5 times, most recently from 5597c28 to a0ae926 Compare August 14, 2025 07:18

justonedev1 changed the title ~~WIP [QC-1298] Extensible interface for accessing QC data sources~~ [QC-1298] Extensible interface for accessing QC data sources Aug 14, 2025

justonedev1 force-pushed the QC-1298 branch from a0ae926 to 7b78a28 Compare August 14, 2025 08:33

justonedev1 requested a review from knopers8 August 15, 2025 11:14

knopers8 requested changes Aug 15, 2025

View reviewed changes

Barthelemy reviewed Aug 19, 2025

View reviewed changes

justonedev1 requested a review from knopers8 August 27, 2025 14:16

Michal Tichák added 4 commits August 28, 2025 11:46

[QC-1298] first version of Data

becce3a

iterators in the middle of work

2d2c61d

filtering iterators working

2b03079

version 2

1589aa7

Michal Tichák added 6 commits August 28, 2025 11:46

Data used in aggregators

047fdf2

benchmarks

e34ad39

helpers implemented

eb0740c

data definition moved to inl file

a2cea35

rename Data to QCInputs

3a90b19

docs

8074bbb

justonedev1 force-pushed the QC-1298 branch from aa67866 to 850f31c Compare August 28, 2025 09:47

iterateQualityObjects with check name

97af63e

justonedev1 force-pushed the QC-1298 branch from 850f31c to 97af63e Compare August 28, 2025 09:51

knopers8 requested changes Sep 8, 2025

View reviewed changes

reworded docs and removed generic container from QCInputs

779ca8e

justonedev1 requested a review from knopers8 September 8, 2025 15:58

knopers8 approved these changes Sep 9, 2025

View reviewed changes

knopers8 enabled auto-merge (squash) September 9, 2025 06:53

knopers8 merged commit 87762a0 into master Sep 10, 2025
6 checks passed


		static constexpr auto filter_nullptr = std::views::filter([](const auto* ptr) -> bool { return ptr != nullptr; });

		static constexpr auto pair_to_reference = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });

	static constexpr auto pair_to_reference = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });
	static constexpr auto pair_to_value_const_ref = std::views::transform([](const auto& pair) -> const auto& { return *pair.second; });

	ILOG(Info, Devel) << " received a data of size : " << data.size() << ENDM;
	ILOG(Info, Devel) << " received data of size : " << data.size() << ENDM;

[QC-1298] Extensible interface for accessing QC data sources #2589

[QC-1298] Extensible interface for accessing QC data sources #2589

Uh oh!

Conversation

justonedev1 commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justonedev1 commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knopers8 commented Aug 8, 2025

Uh oh!

knopers8 left a comment

Choose a reason for hiding this comment

Uh oh!

justonedev1 commented Aug 11, 2025

Uh oh!

knopers8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justonedev1 Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Barthelemy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justonedev1 commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knopers8 commented Aug 19, 2025

Uh oh!

Barthelemy commented Aug 19, 2025

Uh oh!

knopers8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

justonedev1 commented Jul 31, 2025 •

edited

Loading

justonedev1 commented Aug 7, 2025 •

edited

Loading

justonedev1 Aug 19, 2025 •

edited

Loading

justonedev1 commented Aug 19, 2025 •

edited

Loading