Add LST and LSTCore

[ariostas]

Here's a draft of the LSTCore package, but now using real files. Here are some notes about it.

• The data directory is temporary and should be removed before we submit the PR to the main CMSSW repo. We should get started populating RecoTracker-LSTCore.
• I added a standalone directory, following Slava's suggestion. I adapted the scripts so that you can compile and run as before. One important thing to note is that you need to run setup.sh right before compiling/running (we can make it more seamless later).
• I made the LSTCore commit be co-authored by all contributors to TrackLooper so that we all can get some credit once it goes into CMSSW.

I tested running both standalone and CMSSW, and everything seems fine.

[ariostas]

This is a replacement of #29. This is still a draft since it includes all the data files, which should be removed. There's also many files that were added just for the standalone part, and I'm not sure if that's fine.

[VourMa]

Will check it out tonight and maybe test it and report back. Thanks!

[slava77]

The data directory is temporary and should be removed before we submit the PR to the main CMSSW repo. We should get started populating RecoTracker-LSTCore.

these will likely trigger an issue of increasing the git repo by too much and will need to be removed from the git history.

Is it a matter of getting CI to know where to take the data files?
We could already populate the master in a fork of cms-data in https://github.com/SegmentLinking/RecoTracker-LSTCore and just have the CI do git clone https://github.com/SegmentLinking/RecoTracker-LSTCore.git $CMSSW_BASE/src/RecoTracker/LSTCore/data

[ariostas]

these will likely trigger an issue of increasing the git repo by too much and will need to be removed from the git history.

Yeah, I removed them from the git history

Is it a matter of getting CI to know where to take the data files?

Yeah, I'll do that. Thanks!

[VourMa]

I foresee a comment at the review about the many commits with "invalid states", e.g. when the LST was included as an external package, something that was/will never be useful for CMSSW. Based on this, shall we squash the commits? Maybe two commits would make sense, the one that Added LSTCore and one with the rest of the commits in the cmssw repo.

[VourMa]

Is this ok if we leave it like that? Maybe we should just grab the version of CMSSW we have set up?

[slava77]

it's a standalone setup, somewhat tuned to available input file formats and other dependencies (like a test file format).

Also, aren't the following lines (from cmsrel) implying that the version is not known otherwise?

[ariostas]

We should have a better setup script. I only made the minimal modifications to get it to work.

[VourMa]

I also tried the standalone version within CMSSW. It compiles fine but it doesn't run because:

ERROR: Could not find the file = "/home/users/evourlio/LSTinCMSSW/movingFullyToCMSSW/CMSSW_14_1_0_pre3/src/RecoTracker/LSTCore/standalone/..//data/OT800_IT615_pt0.8/endcap_orientation.bin"

I guess this is because data files were removed from the history. Are there changes pending for things to work? Or is there some manual intervention required, like I need to clone the data files with an extra command? I see that this is still empty, so I can just push the data folder structure there? I noticed that Andres has already created an initial branch there, that is filled with data.

[slava77]

I also tried the standalone version within CMSSW. ...

we should probably decide what "standalone" would actually mean:

• a setup that can compile and run sdl_cuda and such while also being able to run a cmsRun
• or something more "restrictive" (with lower dependency stack) using a sparse checkout where only the libsdl_* and sdl_* bins are compiled and can run.

I guess this is because data files were removed from the history. Are there changes pending for things to work?

That's a bit related to the above:

• if the standalone will operate inside CMSSW, we can default to what will be visible in $CMSSW_RELEASE_BASE/external/el8_amd64_gcc12/data/RecoTracker/LSTCore/data
• or assume some local checkout path; in this case the LST_BASE environment will need to be set

[slava77]

• we can default to what will be visible in $CMSSW_RELEASE_BASE/external/el8_amd64_gcc12/data/RecoTracker/LSTCore/data

and if that path is not available, to add a git clone/checkout from https://github.com/SegmentLinking/RecoTracker-LSTCore

[VourMa]

we should probably decide what "standalone" would actually mean:

• a setup that can compile and run sdl_cuda and such while also being able to run a cmsRun
• or something more "restrictive" (with lower dependency stack) using a sparse checkout where only the libsdl_* and sdl_* bins are compiled and can run.

In principle both can work with small changes to the current state of the code, I believe.

For the first case, all we need is to modify:

cmssw/RecoTracker/LSTCore/src/alpaka/LSTESData.dev.cc

Lines 14 to 30 in 8ced0f1

	std::string trackLooperDir() {
	const char* path_lst_base = std::getenv("LST_BASE");
	const char* path_tracklooperdir = std::getenv("TRACKLOOPERDIR");
	std::string path_str;
	if (path_lst_base != nullptr) {
	path_str = path_lst_base;
	} else if (path_tracklooperdir != nullptr) {
	path_str = path_tracklooperdir;
	path_str += "/../";
	} else {
	// FIXME: temporary solution, will need to pass a value from FileInPath or CMSSW search path
	// in the `LSTProducer` or a related ES producer
	path_str = std::getenv("CMSSW_BASE");
	path_str += "/src/RecoTracker/LSTCore";
	}
	return path_str;
	}

to point to a different directory according to your comment:

and if that path is not available, to add a git clone/checkout from https://github.com/SegmentLinking/RecoTracker-LSTCore

For the second case, I did:

git clone --filter=blob:none --no-checkout --depth 1 --sparse --branch CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles https://github.com/SegmentLinking/cmssw.git
cd cmssw/
git sparse-checkout add HeterogeneousCore/AlpakaInterface RecoTracker/LSTCore
git checkout

Given the absence of the $CMSSW_RELEASE_BASE/external/el8_amd64_gcc12/data/RecoTracker/LSTCore/data directory, I did:

cd RecoTracker/LSTCore
git clone [email protected]:SegmentLinking/RecoTracker-LSTCore.git data
cd data
git fetch origin initial
git checkout initial
cd ../../..

Then, the include directories are a bit off, because there is no proper $CMSSW_BASE. I fixed this by including -I../../.. or -I../../../.. in the Makefile and SDL/Makefile respectively. Doing so, I managed to run everything.
Of course, this was a "hack" at this stage. The proper solution to me would be to add a compilation flag to disable the LST_IS_CMSSW_PACKAGE flag, and change lines like:

cmssw/RecoTracker/LSTCore/src/alpaka/Kernels.h

Lines 7 to 10 in 8ced0f1

	#else
	#include "Constants.h"
	#include "Module.h"
	#endif

to point to the appropriate directories, as these lines are useless right now, if also the standalone is in CMSSW.

UPDATE:

As of 6025567, the fix of the Makefiles has been applied, so the standalone package can be used without manual interventions.

[slava77]

will need compliant header guards; not sure it's an ovekill to do them with #ifdef LST_IS_CMSSW_PACKAGE;
otherwise

Suggested change

	#define Constants_cuh
	#define RecoTracker_LSTCore_alpaka_Constants_cuh

[slava77]

is this auto-generated?

[VourMa]

@sgnoohc Perhaps you know/remember?

[slava77]

it's a standalone setup, somewhat tuned to available input file formats and other dependencies (like a test file format).

Also, aren't the following lines (from cmsrel) implying that the version is not known otherwise?

[slava77]

todo: pick up the update from SegmentLinking/TrackLooper#407

[VourMa]

Fixed in #44.

[ariostas]

I foresee a comment at the review about the many commits with "invalid states"

Yeah, I think it makes sense to squash commits further. Let's also resolve all the discussions here by adding more commits to this branch and once it's in good shape we'll squash everything into 1-2 commits.

[ariostas]

Here's a new branch with squashed commits. I can change the branch name and/or commit messages if you prefer.

https://github.com/SegmentLinking/cmssw/tree/LST_and_LSTCore

[VourMa]

Here's a new branch with squashed commits. I can change the branch name and/or commit messages if you prefer.

https://github.com/SegmentLinking/cmssw/tree/LST_and_LSTCore

Seems good to me. Should we force-push those commits in this branch, so that we can keep track of the discussions that we have already started here?

[ariostas]

Should we force-push those commits in this branch, so that we can keep track of the discussions that we have already started here?

Yeah, sounds good. I wasn't sure about that so I made a separate one to be safe, but let's just stick with this branch for discussions.

[ariostas]

There are lots of changes after the force-push, but that's just because I re-synced with upstream master.

[slava77]

There are lots of changes after the force-push, but that's just because I re-synced with upstream master.

the actual diffs in the https://github.com/SegmentLinking/cmssw/pull/30/files look OK.

[ariostas]

Should we close this PR now? It seems like it's no longer needed for discussions.

[VourMa]

Should we close this PR now? It seems like it's no longer needed for discussions.

I think that the point of this PR is to mirror the ones in CMSSW master, so that when the central one is merged, this is also merged automatically. At least that's what I had understood based on the mkFit model of development.

[slava77]

I think that the point of this PR is to mirror the ones in CMSSW master, so that when the central one is merged, this is also merged automatically. At least that's what I had understood based on the mkFit model of development.

yes, that's the idea.
In addition, it's a convenient place to have aside discussion about changes there.

Also, being able to run our CI here would also be helpful; although the comparisons would have to be with an earlier version of itself; so, perhaps more complicated. Would it be possible to at least run the linter part?

[VourMa]

Should I sync the SegmentLinking/cmssw master to the cms-sw/cmssw master? I guess that would allow us to see the conflicts also here?

[VourMa]

Do we know what this 0.015 is? It seems to be the double of the pixel size? However, it doesn't correspond to the numbers that @YonsiG had sent:

For 2S module, the stripe is 5cm * 90 μm.
For PS module pixel, 1.5mm * 100 μm.
For PS module stripe, 2.4cm * 100 μm.

[YonsiG]

I think the number is related to the 1.5mm of pixel module size. And later, it says 0.03 is pixel size times 2 in the comments of L1367. However, unfortunately I'm not sure how this variable is scaled to 0.015..

[slava77]

this part is from my old code;
this is referring to the IT pixel size here. The PS "P" side is in zGeom below

[VourMa]

I read in the CMSSW coding rules that namespaces should use lowercase.
@slava77 Is this acceptable or should I start changing this preemptively?

[slava77]

I would not change now

[VourMa]

Staying true to the inline comment, this variable is not used anywhere. Should I remove it from the code? Any clue by anyone if this will potentially fulfill a purpose and should be kept in? Maybe @slava77 has an idea why/how this was used in the past?

[slava77]

The purpose is in the name: variables without shifting
// The main feature of this shifting is that the strip hits are shifted to be "aligned" in the line of sight from interaction point to the the pixel hit.

It seems OK to remove

[VourMa]

Comments from Matti:

Should the code hereafter be in a separate file?

Maybe @slava77, do you remember why PixelTriplets and PixelQuintuplets were merged? I think it must have been because of the suggestions we got from a hackathon? Shall we just remove these lines and rename the folder to be more descriptive of what it contains?

[slava77]

SegmentLinking/TrackLooper#141

[VourMa]

Are we ok with a rename and removal of lines that makes this look like a separate file?

[slava77]

it would be good to recall why/if the register gains are happening.
Naively, one or two header files should give the same result

[VourMa]

It seems like calling functions from the same file reduced registers wrt. the case where functions are called from separate files. For pT5s, it is common to call functions that are for pT3s, like

cmssw/RecoTracker/LSTCore/src/alpaka/PixelTriplet.h

Line 2491 in 1d9b396

if (not runPixelTripletDefaultAlgo(acc,

hence the files were joined - but in crude way, just copying PixelQuintuplets.h into PixelTriplets.h...
I don't know the lower level details for this behavior...

[slava77]

back then we had .cu files and PixelQuintuplets were in a separate .cu. So, a merge was clearly practical. The headers may even have stayed separate.

Now we compile everything (relevant for this split) in a combined Event.dev.cc.
So, I think that re-split of the .h should be OK

[VourMa]

Ok, let's put that in the queue of updates.

[VourMa]

I am working on removing the ALPAKA_STATIC_ACC_MEM_GLOBAL keyword from our Constants.h file. When I remove it from in front of sinAlphaMax, I get:

src/RecoTracker/LSTCore/src/alpaka/MiniDoublet.h(358): error: identifier "lst::sinAlphaMax" is undefined in device code

However, if I do:

Suggested change

	const float miniSlope = alpaka::math::asin(acc, alpaka::math::min(acc, rt * k2Rinv1GeVf / ptCut, sinAlphaMax));
	const float miniSlope = alpaka::math::asin(acc, alpaka::math::min(acc, rt * k2Rinv1GeVf / ptCut, 1.f * sinAlphaMax));

it compiles just fine. Any idea what is going on? If not, I can propagate the comment to the CMSSW PR.

The same happens with other variables.

[slava77]

The function expects a reference (T const&) in the arguments.
In the example above with 1.f* the compiler generates a temporary first, which works.
But when you use it directly, the constexpr (without static specifiers) has no address.

[VourMa]

I see, it makes sense. How should we handle it then? Should I just keep the ALPAKA_STATIC_ACC_MEM_GLOBAL for the constants that we use "alone" or create temporaries for those and get rid of the keyword to have a consistent treatment for the constants?

[slava77]

This seems to be a guideline topic; better ask Matti.

[VourMa]

Comment from Andrea:

I would suggest to pre-compute the final size and reserve() the vectors to avoid the multiple reallocations.

Do you see any easy way to precompute this? I understand that these vectors should be of size phase2OTHits.size() * it->size(), Σ_i (it->size())_i, where the sum is over all of the phase2OTHits elements, and that this is very much dependent on the occupancy of the tracker per event, which can vary by a lot. Printing out the numbers for 10 events:

phase2OTHits.size()
max(it->size())

2093
18

1625
25

2772
17

2077
24

1390
14

1867
28

2967
21

3667
22

3001
46

4158
18

[slava77]

this should be just a simple ph2_hits.reserve(phase2OTHits.size());

[VourMa]

Thinking about this some more, I was wrong in the calculation of the size of the vectors. I corrected the calculation in the original comment above, and from that it follows that ph2_hits.size() >= phase2OTHits.size(). Running again for 10 events:

ph2_hits.size()

3243

2429

3811

2756

2043

2872

3839

5379

4424

5922

But are you saying that having the estimate ph2_hits.reserve(phase2OTHits.size()); is good enough, so I should just implement that?

[slava77]

I should've checked the interface; it should be ph2_hits.reserve(phase2OTHits.dataSize());

[slava77]

https://github.com/cms-sw/cmssw/pull/45117/files#r1681618028 and related comment suggest some assurance is needed that this is effectively a 1D (and more importantly, a single block) kernel.
Should the kernel be rewritten to Acc1D?

[slava77]

following https://github.com/cms-sw/cmssw/pull/45117/files#r1682937131 and cms-sw#45117 (comment)
should this have a comment about non-existent FP16 support and also condition this ifdef on #if defined(FP16_Base)

[slava77]

I've resynced our master fork to show another conflict in Configuration/PyReleaseValidation/python/relval_2026.py

[slava77]

it looks like options are not consistent with the help anymore

[slava77]

@VourMa
this is now the same as LSTObjType.
Is there a good reason to duplicate?

RecoTracker/LSTCore/interface/Common.h seems more authoritative than this location

[VourMa]

I initially thought that the LSTObjType would include more types (not only those included in TCs) but it turns out that it doesn't. However, LSTOutput.h is a CMSSW-only module, and I am not sure we want it to have interface with LSTCore-only header files -the only way they interface till now is through the LST class If it's ok to "export" an additional dependency from the LSTCore package, then we could remove this indeed.

[slava77]

I'm mainly concerned about expanding standalone being dependent on more of CMSSW packages.

I'm not sure about cases where dependency of CMSSW-specific LST or even remotely LST package on LSTCore/interface can be a problem.
Do you have some hypothetical examples?

[VourMa]

No, I don't have any specific example. I can give a try to switching to the enum in Common.h while resolving conflicts.

[VourMa]

@slava77 Not sure whether you intended to add anything more in your #115 PR, but, if you do, could you also remove this comment? I forgot to do it while updating this file.

[slava77]

I started picking up other stuff at first, but then made a long-range change and reverted back to just a technically required change.
So, I didn't really intend to, but I can, if you prefer.

We still need an update in python files elsewhere to resolve the conflicts; could be added there.

[VourMa]

Indeed, I noticed the conflicts only afterwards, and it can be added there. Let me give that a try now.

[slava77]

I was looking at the git history trying to see the feasibility of rebase: the full diff now is 036340d...CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles; files in non-{LST,LSTCore} packages are modified in the following commits, in their time order from the oldest (including a list of packages)

• 7bc53be (May 31) Configuration, RecoTracker/{ConversionSeedGenerators,FinalTrackSelectors,IterativeTracking,LST}
• 7c6bcfa Configuration
• 85682e8 RecoTracker/{IterativeTracking,LST}
• ef48f74 Configuration
• 149c628 Configuration
• ae67290 Configuration
• a22faeb RecoTracker/FinalTrackSelectors
• 5b1e21c Configuration
• 476d3aa batch8 Configuration, RecoTracker/{IterativeTracking,LST,LSTCore} ... this will sweep everything
• 8928adf (Nov 7) Configuration

IIUC, a clear squash in Configuration would need a squash starting from the first commit grouped all the way down to the most recent; at least the part through batch8 would need to go like that due to relval_2026.py renaming that happened only by batch8. Consequently, it would require squashing of everything in between. A more useful path is to split the above commits if they touch multiple packages and then go back and squash accordingly.

This still seems like too much effort to preserve updates related to the review of the code and not so much to most of the historical features that entered as a single commit in the beginning.

[slava77]

A more useful path is to split the above commits if they touch multiple packages and then go back and squash accordingly.

uhm, but then this will just make non-functional commit states: the code will not run in the earlier commits.

[ariostas]

I've been trying a few options. The rebase with --rebase-merges is the opposite of what we want. But doing the normal rebase we'll have to resolve conflicts again. Given that so many things changed in between, I think it's better to just squash everything down to a single commit.

[slava77]

I've been trying a few options. The rebase with --rebase-merges is the opposite of what we want. But doing the normal rebase we'll have to resolve conflicts again. Given that so many things changed in between, I think it's better to just squash everything down to a single commit.

I agree with one commit

[VourMa]

I've been trying a few options. The rebase with --rebase-merges is the opposite of what we want. But doing the normal rebase we'll have to resolve conflicts again. Given that so many things changed in between, I think it's better to just squash everything down to a single commit.

I agree with one commit

+1

[ariostas]

What's your opinion on force-pushing vs new PR? I'm fine with either.

[slava77]

What's your opinion on force-pushing vs new PR? I'm fine with either.

with a force push you can still see the diffs.
I was thinking of making a copy branch for ourselves (can be mentioned in the PR as well) for the state right before the force-push

[ariostas]

I was thinking of making a copy branch for ourselves (can be mentioned in the PR as well) for the state right before the force-push

Sounds good. I had made _realfiles_backup just in case, but maybe we can use a better name like _45117_final or something

[slava77]

like _45117_final

sounds good to me

[ariostas]

I force-pushed with a single commit co-authored by everyone, and the diff is empty, so I think we're all set