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.

Sign up for GitHub

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

Jump to bottom

DPL Analysis: Table definition rewrite #13664

Draft

aalkin wants to merge 5 commits into AliceO2Group:dev from aalkin:table-definition-rewrite

+1,631 −1,195

Member

aalkin commented Nov 7, 2024

Draft until AliceO2Group/O2Physics#8322 is merged.

Contributor

github-actions bot commented Nov 7, 2024

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1

aalkin requested a review from ktf

November 7, 2024 12:16

alibuild mentioned this pull request

Please consider the following formatting changes to #13664 aalkin/AliceO2#62

Merged

aalkin force-pushed the table-definition-rewrite branch from ceb880d to 6701988 Compare

November 8, 2024 09:11

ktf reviewed

View reviewed changes

DataFormats/simulation/include/SimulationDataFormat/ConstMCTruthContainer.h Outdated

@@ @@ -116,6 +116,7 @@ class ConstMCTruthContainer : public std::vector<char> @@
               #ifndef GPUCA_STANDALONE
               namespace o2::framework
               {
+              // FIXME: this is potentially dangerous

Member

ktf Nov 8, 2024

This should be explained.

Member Author

aalkin Nov 8, 2024

Indeed, I will elaborate the comment.

ktf reviewed

View reviewed changes

Detectors/AOD/include/AODProducerWorkflow/AODProducerHelpers.h

@@ @@ -55,7 +53,7 @@ auto createTableCursor(framework::ProcessingContext& pc) @@
                 framework::Produces<T> c;
                 c.resetCursor(pc.outputs()
                                 .make<framework::TableBuilder>(framework::OutputForTable<T>::ref()));
-                c.setLabel(o2::aod::MetadataTrait<T>::metadata::tableLabel());

Member

ktf Nov 8, 2024

Can't we rewrite aod::MetadataTrait<T>::metadata::tableLabel() to simply return o2::aod::Hash<T::ref.label_hash>::str? This way we would not need to change a few dozens of lines all at once.

Member Author

aalkin Nov 8, 2024

Unfortunately no, since the metadata trait is now defined only on the description/version hash and has no information about the label.

Member

ktf Nov 8, 2024 •

edited

Loading

I do not understand. T is the same class, no? Can't you simply have:

o2::aod::MetadataTrait<T>::metadata::tableLabel() {
  return o2::aod::Hash<T::ref.label_hash>::str;
}

?

ktf reviewed

View reviewed changes

Framework/Core/include/Framework/ASoAHelpers.h Outdated

                 }
               }
-              template <typename... T2s>

Member

ktf Nov 8, 2024

This concept can probably be beneficial independently and it should be trivial to implement. This would allow us to remove a few lines from this diff.

Member Author

aalkin Nov 8, 2024

I'm not sure I understand your point. You mean leaving a compatibility concept alias is_soa_filtered_v and not adding the restrictions in the templates for now, so the file can be left unchanged?

Member

ktf Nov 8, 2024

No, I mean the opposite. If we have a concept which makes sense already now, I would introduce it and use it, so that we get it out of the way.

Member Author

aalkin Nov 12, 2024

Implemented in #13679

ktf reviewed

View reviewed changes

Framework/Core/include/Framework/AnalysisDataModel.h Show resolved Hide resolved

ktf reviewed

View reviewed changes

Framework/Core/include/Framework/AnalysisHelpers.h


		#include <string>
		namespace o2::soa

Member

ktf Nov 8, 2024

Given TableRef is a new class, we can simply add it and introduce these helpers, no?

Member Author

aalkin Nov 8, 2024

It is independent and can be added separately. Should I put it into a separate header then?

Member

ktf Nov 8, 2024

As you prefer. I think we can introduce any internal new classes / structs / concepts as long as we do not use them and there is no side effects or if they are merely a cleanup (e.g. soa::is_table as you have elsewhere).

ktf reviewed

View reviewed changes

Framework/Core/include/Framework/AnalysisHelpers.h

    
                {

                  static_assert(sizeof...(PC) == sizeof...(T), "Argument number mismatch");

                  static_assert(sizeof...(Ts) == framework::pack_size(typename persistent_table_t::persistent_columns_t{}), "Argument number mismatch");

Member

ktf Nov 8, 2024

I would make this a require statement and drop the static_assert

Framework/Core/include/Framework/AnalysisHelpers.h Outdated

                     return arg.globalIndex();
                   } else {
-                    static_assert(!framework::has_type<T>(framework::pack<PC...>{}), "Argument type mismatch");
+                    static_assert(!framework::has_type<A>(typename persistent_table_t::persistent_columns_t{}), "Argument type mismatch");

Member

ktf Nov 8, 2024

I would use a requires arg.globalIndex() and drop the static_assert.

Member Author

aalkin Nov 12, 2024

Implemented in #13679

Framework/Core/include/Framework/AnalysisHelpers.h Outdated

-              template <template <o2::framework::OriginEnc, typename...> class T, o2::framework::OriginEnc ORIGIN, typename... C>
-              struct Produces<T<ORIGIN, C...>> : WritingCursor<typename soa::PackToTable<ORIGIN, typename T<ORIGIN, C...>::table_t::persistent_columns_t>::table> {
+              template <producable T>

Member

ktf Nov 8, 2024

Producible (with capital P), maybe?

Member Author

aalkin Nov 12, 2024

I used std's approach to naming concepts, so is_producible would be the better choice.

Framework/Core/include/Framework/AnalysisHelpers.h Outdated

-                using expression_pack_t = typename aod::MetadataTrait<extension_t>::metadata::expression_pack_t;
+              concept spawnable = soa::is_table<T> && soa::has_metadata<T>;
+              template <spawnable T>

Member

ktf Nov 8, 2024

Capital s?

Member Author

aalkin Nov 12, 2024

is_spawnable

Framework/Core/include/Framework/AnalysisHelpers.h

+              template <spawnable T>
+              struct Spawns : TableTransform<typename aod::MetadataTrait<o2::aod::Hash<T::ref.desc_hash>>::metadata, T::ref> {
+                using metadata = TableTransform<typename aod::MetadataTrait<o2::aod::Hash<T::ref.desc_hash>>::metadata, T::ref>::metadata;
+                using extension_t = typename metadata::extension_table_t;

Member

ktf Nov 8, 2024

In retrospect these inline types are a bad idea, because the compiler is forced to instanciate them. Don't we have a better way of doing it?

Member Author

aalkin Nov 12, 2024

I needed a common base class template for both Builds and Spawns. It is possible to define a function and put here a decltype of it, rather than the type name directly.

Framework/Core/include/Framework/AnalysisHelpers.h Outdated

               }
               template <typename C>
               struct ColumnTrait {
                 static_assert(framework::is_base_of_template_v<o2::soa::Column, C>, "Not a column type!");
                 using column_t = C;
-                static constexpr auto listSize()
+                static consteval auto listSize()

Member

ktf Nov 8, 2024

This is probably consteval already now, no?

Framework/Core/include/Framework/AnalysisHelpers.h

-                  for (counter = 0; counter < tables[0]->num_rows(); ++counter) {
-                    auto idx = -1;
-                    if constexpr (std::is_same_v<T1, Key>) {
+                  for (int64_t counter = 0; counter < tables[0]->num_rows(); ++counter) {

Member

ktf Nov 8, 2024

how does it work now?

Member Author

aalkin Nov 12, 2024

counter is uselessly defined few lines above, but is otherwise not used outside the loop.

Framework/Core/include/Framework/AnalysisHelpers.h Outdated

+              }
+              template <soa::is_table T>
+                requires(!soa::is_filtered_table<T>)

Member

ktf Nov 8, 2024

Do you really need the requires? It's simply the negation of the above, no?

Member Author

aalkin Nov 12, 2024

is_table includes a is_filtered_table and compiler considers the function call ambiguous without this. I have defined is_not_filtered_table in #13679

Framework/Core/include/Framework/AnalysisManagers.h

@@ @@ -417,47 +416,10 @@ struct OutputManager<Builds<T>> { @@
                 }
               };
-              template <typename T>

Member

ktf Nov 8, 2024

This can already go. No need to bind it to the table rewrite.

Member Author

aalkin Nov 12, 2024

Implemented in #13679

Member

ktf commented Nov 8, 2024

I have done a few comments. As discussed privately yesterday, we should try to minimise the differences and introduce "safe" / "unrelated" changes as soon as possible, to minimise the diff. In particular I think:

We can introduce all the new classes / structs / concepts as long as they do not get used by the current code.
We can introduce safe standalone concept if they are already applicable now, and simply use them immediately (e.g. soa::is_table).
We can extend the macro to have an extra field (even if unused).

aalkin force-pushed the table-definition-rewrite branch from 6701988 to c19e9a7 Compare

November 11, 2024 10:04

aalkin mentioned this pull request

DPL Analysis: Preliminary changes for Table rewrite #13679

Merged

aalkin added 4 commits

November 19, 2024 12:00


          Table rework

a521cf4


          minor rebase fixes

4c786be


          improve cursor conditions


          fixup! improve cursor conditions

ddca440

aalkin force-pushed the table-definition-rewrite branch from c19e9a7 to ddca440 Compare

November 19, 2024 11:49


          fix wrong autoreplace

e13a86b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet