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

slava77 · 2023-11-14T23:44:37Z

coupled with updates in SegmentLinking/cmssw#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 · 2023-11-17T02:02:04Z

now rebased

VourMa

Not sure how ready this was but I took a look and have just a small question here. More questions on the CMSSW side...

VourMa · 2023-11-17T15:09:38Z

SDL/LST.cc

 void SDL::LST::eventSetup() {
    static std::once_flag mapsLoaded;
-    std::call_once(mapsLoaded, &SDL::LST::loadMaps, this);
+    std::call_once(mapsLoaded, &SDL::LST::loadMaps);
+
    TString path = get_absolute_path_after_check_file_exists(
-        TString::Format("%s/data/centroid_CMSSW_12_2_0_pre2.txt",TrackLooperDir_.Data()).Data());
+        TString::Format("%s/data/centroid_CMSSW_12_2_0_pre2.txt",trackLooperDir().Data()).Data());
    static std::once_flag modulesInited;
    std::call_once(modulesInited, SDL::initModules, path);
 }


Should this be deleted, now that the maps are loaded in the ESProducer? Or has it stayed until we know that the ESRpoducer method works and will be deleted in the future?

slava77 · 2023-12-29T16:07:57Z

@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 · 2023-12-30T02:06:50Z

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 · 2024-01-03T15:38:11Z

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.

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 · 2024-01-03T16:34:58Z

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 · 2024-01-03T16:39:05Z

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 · 2024-01-03T16:42:11Z

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

slava77 · 2024-01-03T16:43:29Z

/run standalone

slava77 · 2024-01-03T16:45:10Z

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 · 2024-01-03T16:54:29Z

@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 · 2024-01-03T16:56:27Z

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 SegmentLinking/cmssw#16

the errors from the CI are pretty hard to decode

slava77 · 2024-01-03T18:07:18Z

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 · 2024-01-03T18:13:16Z

/run standalone

slava77 · 2024-01-03T23:24:31Z

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

slava77 · 2024-01-04T03:47:35Z

cmssw on 100 events look OK now
eff/fr

hits

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

slava77 · 2024-01-10T23:23:23Z

/run cmssw from-CMSSW_13_3_0_pre3_LST_X/modules-dev

slava77 · 2024-01-10T23:24:23Z

@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 · 2024-01-10T23:31:33Z

@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 · 2024-01-10T23:38:35Z

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 · 2024-01-11T00:09:12Z

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

github-actions · 2024-01-25T03:48:45Z

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.

GNiendorf · 2024-01-27T01:35:05Z

SDL/Event.cc

@@ -1,7 +1,8 @@
 #include "Event.h"

 SDL::modules* SDL::modulesInGPU = new SDL::modules();
-SDL::modulesBuffer<Acc>* SDL::modulesBuffers = new SDL::modulesBuffer<Acc>(devAcc);
+SDL::modulesBuffer<SDL::Dev>* SDL::modulesBuffers = new SDL::modulesBuffer<SDL::Dev>(devAcc);


I see you changed Acc to SDL::Dev here, but you should do this for all of our objects to be consistent I think. I don't know if this matters since the performance plots look unchanged and this uses SDL::Dev while all the other Buffers still have Acc, but good to be consistent.

Same in all of the header files that define a buffer, to remove confusion we should move the "Acc" named templated parameter to Dev so it's clear what we pass in. See the random examples below.

TrackLooper/SDL/Event.cc

Line 319 in 25e08e7

miniDoubletsBuffers = new SDL::miniDoubletsBuffer<Acc>(nTotalMDs, nLowerModules, devAcc, queue);

TrackLooper/SDL/MiniDoublet.h

Lines 96 to 99 in 25e08e7

template <typename TAcc>

struct miniDoubletsBuffer : miniDoublets {

Buf<TAcc, unsigned int> nMemoryLocations_buf;

I don't know if this matters since the performance plots look unchanged and this uses SDL::Dev while all the other Buffers still have Acc, but good to be consistent.

right, Acc or Dev does not seem to matter for the buffers at the final implementation level (after all the template parsing is done); however code exposed to CMSSW needs to be templated from a Dev.

Dev seems to be more appropriate based on Alpaka docs for a buffer https://alpaka.readthedocs.io/en/latest/_images/structure_assoc.png

VourMa

I left a couple of comments, mostly for my own understanding, rather than real actions. I understand that there are two open issues with this PR:

It has a conflict.
We should replace Acc -> SDL::Dev

I will approve and merge when 1) is resolved, while 2) will be followed up in a new PR (do we need an issue or is it going to be followed up soon?).

VourMa · 2024-01-30T23:02:15Z

SDL/Kernels.h

@@ -18,12 +18,12 @@ namespace SDL {

  ALPAKA_FN_ACC ALPAKA_FN_INLINE void rmPixelTripletFromMemory(struct SDL::pixelTriplets& pixelTripletsInGPU,
                                                               unsigned int pixelTripletIndex) {
-    pixelTripletsInGPU.isDup[pixelTripletIndex] = 1;
+    pixelTripletsInGPU.isDup[pixelTripletIndex] = true;


Out of curiosity, why this change? Did it cause an issue? Is this defined as a bool variable?

this was from a clang check; a part missed in #356

VourMa · 2024-01-30T23:06:24Z

SDL/LST.cc

-    exit(2);
+  void loadMaps() {
+    // Module orientation information (DrDz or phi angles)
+    auto const& tldir = trackLooperDir();


What's the point of defining this, if trackLooperDir() is used almost everywhere in the following? Was it meant to be a short version but then it was forgotten?

removed now

…ce duplication)

… namespace)

…r/TAcc ; add modulesBuffersES as pointer to const (ES) module data not owned by SDL

…paka design

slava77 · 2024-02-01T00:22:54Z

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

slava77 · 2024-02-01T00:26:40Z

We should replace Acc -> SDL::Dev

done now in eca2c59

github-actions · 2024-02-01T00:41:01Z

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 · 2024-02-01T02:05:22Z

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.

VourMa

This is out of @slava77's way

slava77 mentioned this pull request Nov 14, 2023

split heavier methods from Module.h to ModuleMethods.h #344

Closed

slava77 closed this Nov 16, 2023

slava77 force-pushed the mmap-with-es branch from 729b6d9 to d72d5da Compare November 16, 2023 23:48

slava77 reopened this Nov 17, 2023

VourMa reviewed Nov 17, 2023

View reviewed changes

slava77 force-pushed the mmap-with-es branch from 10fe919 to 1fd4b59 Compare December 29, 2023 15:43

slava77 marked this pull request as ready for review January 3, 2024 23:09

slava77 mentioned this pull request Jan 3, 2024

SDL::modulesBuffer as an ES product SegmentLinking/cmssw#16

Merged

SegmentLinking deleted a comment from github-actions bot Jan 11, 2024

GNiendorf reviewed Jan 27, 2024

View reviewed changes

VourMa reviewed Jan 30, 2024

View reviewed changes

slava77devel added 18 commits January 31, 2024 15:00

define values derived from Acc only when it's expected to be defined

810f844

split heavier methods from Module.h to ModuleMethods.h

0efcf09

make module parseIs methods static

f75a8af

make loadMaps static (consider moving it out of the LST class to redu…

7962508

…ce duplication)

make modules data pointers const

73667f9

drop emacs temp files

4c574d1

check both CMSSW_BASE and CMSSW_RELEASE_BASE if available

01760bf

initialize derived quantities

ce85856

move constants away from global namespace to SDL (unless already in a…

08743db

… namespace)

sdlLayers before connectedPixels consistently in modules and buffers

bd004dd

clang-format

d91352b

clang-tidy

2340fed

describe lst_headers variant of the LST external

412596f

use device/TDev template argument trait for Buf instead of accelerato…

7b58aa0

…r/TAcc ; add modulesBuffersES as pointer to const (ES) module data not owned by SDL

clang-format and clang-tidy

ac98d78

clang-fix

817c1c5

refactor LST ES-related calls to be usable by CMSSW directly

0aa37d2

cleanup unused var

5487bf0

slava77 force-pushed the mmap-with-es branch from 0f6afe7 to 5487bf0 Compare January 31, 2024 23:15

slava77devel added 2 commits January 31, 2024 16:21

clang compile fix

87ad3de

move data buffer templates from Acc to Dev to be more in line with al…

eca2c59

…paka design

VourMa approved these changes Feb 2, 2024

View reviewed changes

VourMa merged commit d79218c into SegmentLinking:master Feb 2, 2024
1 check passed

	template <typename TAcc>
	struct miniDoubletsBuffer : miniDoublets {
	Buf<TAcc, unsigned int> nMemoryLocations_buf;

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

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

Conversation

slava77 commented Nov 14, 2023 • edited Loading

slava77 commented Nov 17, 2023

VourMa left a comment

Choose a reason for hiding this comment

VourMa Nov 17, 2023

Choose a reason for hiding this comment

slava77 commented Dec 29, 2023

slava77 commented Dec 30, 2023

ariostas commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

ariostas commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 3, 2024

slava77 commented Jan 4, 2024

slava77 commented Jan 10, 2024

slava77 commented Jan 10, 2024

ariostas commented Jan 10, 2024

ariostas commented Jan 10, 2024 • edited Loading

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 25, 2024

GNiendorf Jan 27, 2024

Choose a reason for hiding this comment

slava77 Jan 27, 2024

Choose a reason for hiding this comment

VourMa left a comment

Choose a reason for hiding this comment

VourMa Jan 30, 2024

Choose a reason for hiding this comment

slava77 Jan 31, 2024

Choose a reason for hiding this comment

VourMa Jan 30, 2024

Choose a reason for hiding this comment

slava77 Jan 31, 2024

Choose a reason for hiding this comment

slava77 commented Feb 1, 2024

slava77 commented Feb 1, 2024

github-actions bot commented Feb 1, 2024

github-actions bot commented Feb 1, 2024

VourMa left a comment

Choose a reason for hiding this comment

slava77 commented Nov 14, 2023 •

edited

Loading

ariostas commented Jan 10, 2024 •

edited

Loading