PIDML: use columns' labels from JSON file instead of hardcoding them #7371

mytkom · 2024-08-20T15:49:22Z

No description provided.

alibuild · 2024-08-20T17:26:25Z

Error while checking build/O2Physics/o2 for 6ce8bb0 at 2024-08-20 19:26:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:66:24: error: 'cent' is not a member of 'o2::aod'
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:1: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:1: error: '<expression error>' in namespace 'o2::soa' does not name a type
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:19: error: 'PidTracksData' does not name a type; did you mean 'PidTracksDataMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:19: error: 'PidTracksData' was not declared in this scope; did you mean 'PidTracksDataMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:1: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:19: error: 'PidTracksData' was not declared in this scope; did you mean 'PidTracksDataMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:65:1: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:143:24: error: 'cent' is not a member of 'o2::aod'
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:1: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:1: error: '<expression error>' in namespace 'o2::soa' does not name a type
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:19: error: 'PidTracksMc' does not name a type; did you mean 'PidTracksMcMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:19: error: 'PidTracksMc' was not declared in this scope; did you mean 'PidTracksMcMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:1: error: template argument 1 is invalid
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:19: error: 'PidTracksMc' was not declared in this scope; did you mean 'PidTracksMcMl'?
/sw/SOURCES/O2Physics/7371-slc7_x86-64/0/Tools/PIDML/pidML.h:142:1: error: template argument 1 is invalid
ninja: build stopped: subcommand failed.

Full log here.

mytkom · 2024-08-21T07:35:18Z

passing table reference would not be needed - changes in progress

saganatt · 2024-08-23T07:28:14Z

@ktf @aalkin FYI.

IOBinding not used because of additional memory copying and no gain in computational time.
Apparently we cannot pass the data to OnnxRuntime without copies, at least because of Filtered<> Tables and possible standardization of ML inputs.
Marek can explain in more detail the rationales, if you want.

saganatt · 2024-08-23T07:28:37Z

Tools/PIDML/pidOnnxModel.h

+  }
+
+  template <typename T, typename... C>
+  std::optional<float> getColumnValueByLabel(o2::framework::pack<C...>, const T& rowIterator, const std::string& columnLabel)


@aalkin @ktf
Perhaps we could have this function in O2 instead of O2Physics?

Then it would be typename C::type as the return type; for ML usage, hardcoded float cast was reasonable - but if it would be in O2, then not anymore.

@aalkin @ktf Perhaps we could have this function in O2 instead of O2Physics?

In line of principle, I do not see why not. Just open a separate PR and we discuss the details there.

saganatt · 2024-08-23T07:29:36Z

Tools/PIDML/pidOnnxModel.h

+  }
+
+  template <typename C, typename T>
+  typename C::type getPersistentValue(const T& rowIterator)


This also could replace the O2 function in ASOA.h, which now gets the value at the arrow level

After this PR is merged, it would be possible to use a common getter also for dynamic columns - and the code would look like that: static_cast<C>(rowIterator).get()

saganatt · 2024-09-02T09:21:00Z

Hi @ktf @aalkin do you think we can move the aforementioned functions to O2?

ktf · 2024-09-02T14:33:52Z

Tools/PIDML/pidOnnxModel.h

  }

+  template <typename... P1, typename... P2>
+  static constexpr bool is_equal_size(o2::framework::pack<P1...>, o2::framework::pack<P2...>)


do you really need this function? Isn't it just pack_size({some pack}) == pack_size({some othe pack})

ktf · 2024-09-03T07:09:08Z

Please, open a PR there with what you want to add, and we discuss it.

jgrosseo · 2024-09-09T12:39:24Z

@ktf this was just discussed in the AIP meeting and we are worried that the performance impact of getting columns by name is significant.

See also slide 31 from https://indico.cern.ch/event/1452758/contributions/6115208/attachments/2923455/5131519/Final%20PID%20ML%20presentation.pdf

ktf · 2024-09-10T07:11:40Z

Indeed, I would have an API which allows you to get a "token" associated to a column (which can be retrieved by name) and then use that token to get the binding in a more effective way. As I said, let's open a PR to O2 and see what can be done.

jgrosseo · 2024-09-10T07:17:36Z

Sounds good. And important is that the json (which is the desired use case here) does not do the mapping from branch name in tree but from column name in table. Only the latter is forward compatible if we change the underlying column name.

ktf · 2024-09-10T07:32:46Z

Tools/PIDML/pidOnnxModel.h

    input_shape[0] = batch_size;

-    std::vector<float> inputTensorValues = createInputsSingle(track);
+    std::vector<float> inputTensorValues = getValues(track);


I just saw that you are still converting our columns to rows. I though you said you managed to do it with bindings. Did I understand it wrong?

I.e., I understood you found a way to setup an OrtValue directly from a signle IoBinding which is attached to the values of the column, without an intermediate representation.

As Maja said previously in the discussion in this PR, we do not use IOBinding. We wanted to use it, but we still needed to make a copy of a value to standardize it. Anton suggested using gandiva to copy and standardize the whole column and then use IOBinding, so I implemented the first part of it: standardization using gandiva, but there was an issue. It copies the whole column, but even our example task used filtered iterator and just needed to iterate over only 1/10 of the column, because of the filtering.

So copying it and IOBinding would mean copying x (10 for my example) times more data than we need and then we can't use IOBinding, because it needs contiguous memory.

Another solution can be copying only filtered rows of column to the new column and then making a second copy to gandiva standardized column (we need continuous memory to use IOBinding), but for us, it still seems not ideal. (of course, we can skip gandiva stage and copy it once with standard arithmetic operations, but I am not sure which one would be faster)

We could also say that we ban Filtered<> from our PIDML API and the user would need to prepare the table contiguous, but this would be hard to use our API then

On top of that it wouldn't support dynamic columns unless we evaluate lambda and save it to a newly created column
So we decided not to use IOBinding.

mytkom · 2024-09-10T09:19:11Z

About the performance of this solution: I think that @jgrosseo is correct, and implementing that would be a performance issue, but I will make a draft PR in O2 because I have already written it. I will add a benchmark there and tell you the numbers.

If it comes to our OnnxModel, we should still be able to get some vector of getters in our constructor (string comparison would be only there), and then we would call this handler during iteration. Performance should be similar to hard-coded implementation, but it wouldn't be possible for other projects to reuse our code.

github-actions · 2024-11-08T01:38:31Z

This PR has not been updated in the last 30 days. Is it still needed? Unless further action is taken, it will be closed in 5 days.

use dynamic columns from JSON file instead of hardcoding them

c771513

mytkom requested review from alibuild and saganatt as code owners August 20, 2024 15:49

formatting

6ce8bb0

alibuild mentioned this pull request Aug 20, 2024

Please consider the following formatting changes to #7371 mytkom/O2Physics#12

Closed

add missing includes

82ebb92

mytkom added 9 commits August 21, 2024 11:23

use getIterator - no more passing of table

17e007e

comment out tracks table in KaonPidTask

6524f6d

refactor solution to be more generic

cde7a0a

move out common condition

4973874

remove unused methods from ONNXModel and simplify is_equal_size function

77ff0a7

tidy up pidEffAndPurProducer

b8c119d

remove unused include

50c6bd4

remove more unused includes

a3408c0

add include to ONNXModel

0ee085b

saganatt reviewed Aug 23, 2024

View reviewed changes

mytkom changed the title ~~PIDML: use dynamic columns from JSON file instead of hardcoding them~~ PIDML: use columns' labels from JSON file instead of hardcoding them Aug 23, 2024

ktf reviewed Sep 2, 2024

View reviewed changes

ktf reviewed Sep 10, 2024

View reviewed changes

jgrosseo marked this pull request as draft September 10, 2024 08:23

mytkom mentioned this pull request Sep 10, 2024

Get column value by column label in Framework Core ASoA AliceO2Group/AliceO2#13498

Merged

use getGetterByLabel (not merged to O2Framework yet) as proof of concept

312a1c5

alibuild mentioned this pull request Oct 8, 2024

Please consider the following formatting changes to #7371 mytkom/O2Physics#13

Closed

github-actions bot added the stale label Nov 8, 2024

github-actions bot closed this Nov 13, 2024

PIDML: use columns' labels from JSON file instead of hardcoding them #7371

PIDML: use columns' labels from JSON file instead of hardcoding them #7371

Uh oh!

Conversation

mytkom commented Aug 20, 2024

Uh oh!

alibuild commented Aug 20, 2024

Uh oh!

mytkom commented Aug 21, 2024

Uh oh!

saganatt commented Aug 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mytkom Aug 23, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

saganatt commented Sep 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktf commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgrosseo commented Sep 9, 2024

Uh oh!

ktf commented Sep 10, 2024

Uh oh!

jgrosseo commented Sep 10, 2024

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!

mytkom commented Sep 10, 2024

Uh oh!

github-actions bot commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mytkom Aug 23, 2024 •

edited

Loading

ktf commented Sep 3, 2024 •

edited

Loading