updates to get module maps used as ES products jointly with CMSSW

[slava77]

coupled with updates in https://github.com/SegmentLinking/cmssw/pull/16

• split somewhat heavy methods related to filling module data from Module.h to ModuleMethods
• move LST::loadMaps to a free function to avoid making an instance of LST where it's not needed
• add LST::loadAndFillES method to be called by an CMSSW ES producer
• const pointers in modules SoA data

[slava77]

now rebased

[slava77]

@ariostas please check the linter error report.

error: invalid argument '-std=c++17' not allowed with 'C' looks like the reason for error: cannot use 'try' with exceptions disabled

I was expecting a proposed patch file with changes (what's usually done with cmssw setup). Instead the bad formatting comes out with comments like

File SDL/Constants.h does not conform to Custom style guidelines. (lines 14, 66)

which are not very useful/practical. I'm not sure though if the '-std=c++17' not allowed have prevented something to complete.

[slava77]

error: invalid argument '-std=c++17' not allowed with 'C' looks like the reason for error: cannot use 'try' with exceptions disabled

I got rid of it by adding --language=c++

[ariostas]

Hi @slava77, the error: invalid argument '-std=c++17' error is not preventing anything from running. It's an annoying thing that I explain more in a comment on your PR https://github.com/SegmentLinking/TrackLooper/pull/360.

Regarding the patch file, I thought that it would be the same amount of work to apply the patch file vs to just run clang-format manually, so I didn't think it was worth it. But I can implement generating a patch file, if you prefer.

[slava77]

Regarding the patch file, I thought that it would be the same amount of work to apply the patch file vs to just run clang-format manually, so I didn't think it was worth it. But I can implement generating a patch file, if you prefer.

we should at least have the commands needed to run clang-format and clang-tidy in the readme. While clang-format seems trivial (it only needs the file name), the clang-tidy is not trivial

[slava77]

Hi @slava77, the error: invalid argument '-std=c++17' error is not preventing anything from running. It's an annoying thing that I explain more in a comment on your PR #360.

do you mean that we should accept that all linter actions will fail?

I noticed also in the logs of the linter the following:

  INFO:CPP Linter:Got 403 response from POSTing comment
  ERROR:CPP Linter:response returned 403 message: {"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/issues/comments#create-an-issue-comment"}

is it instead the reason for failing the linter action (instead of the stdc++17 thing)?

[slava77]

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

/run standalone

[slava77]

I don't see any activity in the CI frame after my /run commands. @ariostas are they blocked by the failed linter or does it take longer to react?

[ariostas]

@slava77 oh sorry, that's a bug I have to fix. What happened was that I set it so that only one job runs at a time, to prevent us from accidentally running multiple iterations of the same thing. Try writing both commands on a single comment (in separate lines), and then let's check if they run.

[slava77]

The /run action actually came back to me in https://github.com/SegmentLinking/TrackLooper/actions/runs/7399973283/job/20132579034; although I still do not see it in this PR thread.

It came back with what's probably from

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev
Branch name is invalid. Ignoring...

not sure what I was supposed to pass instead to pick up https://github.com/SegmentLinking/cmssw/pull/16

the errors from the CI are pretty hard to decode

[slava77]

The PR was built and ran successfully with CMSSW. Here are some plots.

@ariostas did you fix something or did I misunderstand the previous CI reports that said the jobs were canceled or had problems?

Are the plots meant to have no reference? It would be much better to have a comparison

[slava77]

/run standalone

[slava77]

/run standalone /run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

cmssw on 100 events look OK now eff/fr

hits

I think that the tiny differences are from reproducibility on GPU.

[slava77]

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

@ariostas is there a way to rerun the linter? I tried to rerun from the last attempt interface, but it used the old commit/variant before your latest updates.

[ariostas]

@slava77 the linter workflow is triggered differently and github uses the workflow file present in the current branch. So you would have to rebase, but it's probably not worth it.

[ariostas]

I think the cmssw test will fail because I haven't updated the recipe to use lst_headers.xml. Now that I think about it I could have updated it without breaking the action for the current default cmssw branch. It's an easy fix so I'll do it tonight.

[github-actions[bot]]

There was a problem while building and running with CMSSW. The logs can be found here.

[ariostas]

Should all work now. However, one thing to keep in mind is that I set the CI so that if you specify a custom branch of CMSSW then it doesn't produce comparison plots since presumably the changes in both repos depend on each other or the CMSSW setup is different. This could be solved by having a fixed reference file somewhere, as we discussed in the meeting.

[ariostas]

/run standalone /run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

if you specify a custom branch of CMSSW then it doesn't produce comparison plots since presumably the changes in both repos depend on each other or the CMSSW setup is different

In a context of the CMSSW branch being an unmerged PR there is still a well defined "master" reference that would come from running the cmssw test out of the box

[ariostas]

In a context of the CMSSW branch being an unmerged PR there is still a well defined "master" reference that would come from running the cmssw test out of the box

Even in that context, if let's say there's a change to the xml files one might need to scram uninstall, reinstall and rebuild. I'm expecting that most likely something will break, but I'll think if there's a way to do it without resorting to wiping the cmssw setup and starting from scratch. Do you think this kind of changes might not be an issue?

There's also the (very infrequent) case of changing to a later cmssw release, which would definitely require rebuilding everything. But we can probably not worry about those for now.

[github-actions[bot]]

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

[github-actions[bot]]

There was a problem while building and running with CMSSW. The logs can be found here.

[slava77]

Even in that context, if let's say there's a change to the xml files one might need to scram uninstall, reinstall and rebuild.

OK, this PR pair lst:355,cmssw:16 is special due to the creation of an extra header package

There's also the (very infrequent) case of changing to a later cmssw release, which would definitely require rebuilding everything. But we can probably not worry about those for now.

assuming the CI is established (we are almost there), I'd expect that the "latest" reference is generally available. So, we do not need to rerun the baseline every time.

[ariostas]

I'd expect that the "latest" reference is generally available

Yeah, that's true. I'll think of a good way to do this.

For lst:355,cmssw:16, did anything else about the CMSSW setup change? The CI failed with this error and I'm not sure why, so I'll look into it.

Exception Message:
No data of type "SDL::modulesBuffer<alpaka_common::DevHost>" with label "" in record "TrackerRecoGeometryRecord"
 Please add an ESSource or ESProducer to your job which can deliver this data.
----- End Fatal Exception -------------------------------------------------

[slava77]

No data of type "SDL::modulesBuffer"

that's my mistake, I did not update the lstProducer/tracking related file. My tests were on a manually modified config. I will fix it soon.

[slava77]

/run standalone /run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[github-actions[bot]]

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

[github-actions[bot]]

There was a problem while building and running with CMSSW. The logs can be found here.

[slava77]

There was a problem while building and running with CMSSW. The logs can be found here.

this is ERROR: Could not find the file = "/data/endcap_orientation_data_CMSSW_12_2_0_pre2.txt"

it looks like the LST_BASE is not set.

The log has a message

**** Following environment variables are going to be unset.
      LST_BASE

It sounds like something is off in the CI setup.

@ariostas do you think it's on the CI side or in my code updates?

[ariostas]

It seems like there's a bug where if there's a new line character in the line that contains the branch name it ends up messing up the branch name check. I'll try to run it again.

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[github-actions[bot]]

There was a problem while building and running with CMSSW. The logs can be found here.

[ariostas]

@slava77 seems like the issue now is that it doesn't have the required files in RecoTracker/LST/data on the CMSSW side.

[slava77]

/run standalone /run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[github-actions[bot]]

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

[github-actions[bot]]

There was a problem while building and running with CMSSW. The logs can be found here.

[ariostas]

Still have to fix the bug where a new line or some weird character messes it up.

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

Still have to fix the bug where a new line or some weird character messes it up.

oh, I thought it was fixed already are you restarting it manually or is the current solution to add some random text with a newline before the /run command?

[ariostas]

For now the solution is to put the branch name at the very end of the comment without any spaces or new lines at the end. I wonder if for some reason it's inserting a \r or something like that that it doesn't like. I'll do some testing and submit a PR tomorrow.

[github-actions[bot]]

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

[slava77]

/run standalone /run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

[slava77]

We should replace Acc -> SDL::Dev

done now in eca2c59

[github-actions[bot]]

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

[github-actions[bot]]

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.