Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) #127

ariostas · 2024-11-20T15:28:32Z

This is a rebase of PRs #45 #36 and #55. I grouped them since they are standalone-only and relatively simple.

For #45, I simply adapted to the new SoA stuff.

For #36, I made the changes in that PR, but with additional changes that I'll comment in the code.

For #55, I copied the current readme and updated things. For this one, we could alternatively do it at the very end in case there are more changes.

Co-authored-by: Gavin Niendorf <[email protected]>

ariostas · 2024-11-20T15:29:25Z

RecoTracker/LSTCore/src/LSTESData.cc

+    std::stringstream search_path;
+    search_path << std::getenv("CMSSW_SEARCH_PATH");


I changed the initialization here since the IB was complaining about a nullptr.

ariostas · 2024-11-20T15:32:53Z

RecoTracker/LSTCore/standalone/setup.sh

-elif [[ $ARCH == "aarch64" || $ARCH == "arm64" ]]; then
-  export SCRAM_ARCH=el9_aarch64_gcc12
+if [ -z ${CMSSW_SEARCH_PATH+x} ]; then
+  if [ -z ${CMSSW_VERSION+x} ]; then


I changed it to check if a version was set in case we want to override it in the CI. Also, now it uses the command Slava suggested instead of a hard-coded path for the release in case we want to use an IB version.

Actually, now I'm thinking that this might lead to confusion. It should check for something more explicit like FORCED_CMSSW_VERSION

slava77 · 2024-11-20T15:45:04Z

RecoTracker/LSTCore/standalone/README.md

+Currently, the data files for LST need to be copied manually (under `$CMSSW_BASE/external/$SCRAM_ARCH/data/RecoTracker/LSTCore/data/`). This is done by running:
+
+```bash
+mkdir -p $CMSSW_BASE/external/$SCRAM_ARCH/data/RecoTracker/LSTCore/
+cd $CMSSW_BASE/external/$SCRAM_ARCH/data/RecoTracker/LSTCore/
+git clone [email protected]:cms-data/RecoTracker-LSTCore.git data
+cd -
+```


since cmsrel CMSSW_14_2_0_pre4 is done above, this part is needed only if the data files need to be updated. Please add a comment about it

slava77 · 2024-11-20T15:54:46Z

RecoTracker/LSTCore/standalone/README.md

+# These are the lines that you need to manually change for a CVMFS-less setup.
+# In this example we use cvmfs paths since that is where the dependencies are in lnx7188/cgpu1, but they can point to local directories.
+export BOOST_ROOT=/cvmfs/cms.cern.ch/el8_amd64_gcc12/external/boost/1.80.0-60a217837b5db1cff00c7d88ec42f53a
+export ALPAKA_ROOT=/cvmfs/cms.cern.ch/el8_amd64_gcc12/external/alpaka/1.1.0-7d0324257db47fde2d27987e7ff98fb4


I'm not sure this (full quoted part) is particularly maintainable.
Does it still work?
Considering such a heavy dependence on cvmfs, why not collapse this down to source setup.sh?

slava77 · 2024-11-20T15:58:26Z

RecoTracker/LSTCore/standalone/README.md

-git cms-checkdeps -a -A
+git fetch SegLink ${LST_BRANCH}
+git checkout ${LST_BRANCH}
+git cms-addpkg Configuration RecoTracker/ConversionSeedGenerators RecoTracker/FinalTrackSelectors RecoTracker/IterativeTracking RecoTracker/LST RecoTracker/LSTCore


with CMSSW_14_2_0_pre4 can we fall back to a more standard git cms-checkdeps -a -A after adding RecoTracker/LST RecoTracker/LSTCore ?

Technically, only new changes are needed; smth like git cms-addpkg $(git diff CMSSW_14_2_0_pre4...${LST_BRANCH} --name-only | cut -d/ -f-2 | uniq) or, more precisely git merge-base CMSSW_14_2_0_pre4 ${LST_BRANCH} should be used as a ref to the diff above

slava77 · 2024-11-20T16:05:30Z

RecoTracker/LSTCore/standalone/README.md


-Then all that is left to do is set some environment variables. We give an example of how to do this in lnx7188/cgpu-1.
+    /data2/segmentlinking/CMSSW_14_1_0_pre0/step2_21034.1_100Events.root


is this used in the CI?
I recall that we've updated to something more fresh than 21034

Good catch! It's 29834.1

Co-authored-by: Manos Vourliotis <[email protected]>

VourMa · 2024-11-20T17:09:41Z

RecoTracker/LSTCore/standalone/README.md

+
+```bash
+CMSSW_VERSION=CMSSW_14_2_0_pre4 # Change with latest/preferred CMSSW version
+LST_BRANCH=CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel # Change to the appropriate branch


I haven't fully checked the code yet, but should we decide on our "new", "master" branch and have that listed here?

Yeah, that would be good. Maybe it can just be master_LST, we submit PRs to CMSSW from there, and every time they get merged we sync it with the upstream master.

I think mkFit has an established way of doing that that works nicely. @slava77 could probably give us the details.

mkFit is using master in trackreco repository directly for the internal PRs. It's resynced whenever necessary. Also, in mkFit we never merge PRs directly: the same topic branch is submitted to the upstream and once it's reviewed and merged, it will appear as merged in trackreco as well (except that not all developers do it and we end up with PRs that look unmerged but actually are).
there is no CI in mkFit and using master is more straightforward.
So, I'm not sure that in our case the is a good reference in mkFit practices.

With CI things get complicated and we need to discuss/decide what's a better mode.

Smth like CMSSW_14_2_0_pre4_LST_X that's explicitly coupled to a pre-release is more clear for developers and the CI. The downside is this requires explicit updates to a new branch and pre-release.

Using master for a target can be OK, but the CI would need to be updated to figure out the latest working release and do a rebase of the topic branch for a test, which is problematic.

Another question is do we want to merge in SegmentLinking/cmssw/master-or-whatever before the full review in the upstream?

I was under the assumption that we would submit to CMSSW in batches so that things can move quicker. Either way, I can figure out a way to get the CI to pick the right release if needed.

I was under the assumption that we would submit to CMSSW in batches so that things can move quicker.

It's practical for our todos from #75
After that part settles it may be more practical to avoid batching.

GNiendorf · 2024-11-20T17:45:01Z

I haven't checked to see if this runs properly, but it looks fine otherwise (for my portion of the PR).

VourMa

Thanks for the rebase, @ariostas! The PR looks good, I have a couple of minor comments. I guess the most significant one would be to settle on the main branch, but this could even go in before that, if we decide on that.

VourMa · 2024-11-21T15:16:39Z

RecoTracker/LSTCore/standalone/README.md

+git remote add SegLink [email protected]:SegmentLinking/cmssw.git
+git fetch SegLink ${LST_BRANCH}
+git checkout ${LST_BRANCH}


Now that the LST PR is in CMSSW, these steps can in principle be skipped. Maybe add a comment that this is only needed to get some bleeding-edge development from some branch?

Thank you, Manos! I addressed all these comments.

VourMa · 2024-11-21T15:17:39Z

RecoTracker/LSTCore/standalone/README.md

+For this setup, dependencies are still provided from CMSSW through CVMFS but no CMSSW area is setup. This is done by running the following commands.
+
+``` bash
+LST_BRANCH=CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles # Change to the development branch


Maybe update this to at least be consistent with the one mention in the section just above?

VourMa · 2024-11-21T15:22:36Z

RecoTracker/LSTCore/standalone/README.md

-sdl_make_tracklooper -mc
-cd ..
-```
+## Setting up the area


Do we even need this section? Isn't that already covered above ("Setting up LST" section)?

...I know, probably a question to myself back in the day, but maybe we can improve now...

Co-authored-by: Manos Vourliotis <[email protected]>

ariostas · 2024-11-22T17:42:11Z

/run all

github-actions · 2024-11-22T17:57:53Z

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.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     41.0    387.1    191.5    129.0    136.5    503.7    119.5    248.9    100.9      3.3    1861.4    1316.7+/- 365.1     491.6   explicit[s=4] (target branch)
   avg     41.2    385.4    192.4    129.1    136.9    499.3    119.3    244.3    102.2      3.1    1853.2    1312.8+/- 367.0     492.4   explicit[s=4] (this PR)

github-actions · 2024-11-22T19:10:13Z

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 · 2024-11-25T10:46:12Z

Shall I merge this, now that the tests have passed successfully?

slava77 · 2024-11-25T14:00:45Z

Shall I merge this, now that the tests have passed successfully?

the target branch is CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles_batch1_devel_rebased
we talked about getting something better named. I'm not sure when is the right time to do it.

ariostas · 2024-11-25T14:21:17Z

we talked about getting something better named

What if we do CMSSW_14_2_X_LST_batch1_devel?

ariostas · 2024-11-25T14:25:00Z

What if we do CMSSW_14_2_X_LST_batch1_devel?

I just saw that Gavin merged another PR, so let's stick with the current name for now. We could just rename it before submitting the CMSSW PR if necessary.

slava77 · 2024-11-25T14:36:26Z

What if we do CMSSW_14_2_X_LST_batch1_devel?

I just saw that Gavin merged another PR, so let's stick with the current name for now. We could just rename it before submitting the CMSSW PR if necessary.

CMSSW_14_2_X_LST_batch1_devel sounds reasonable. batch1 would apparently have a new meaning for the first batch in 14_2_X with actual feature developments.
Making another PR to this branch as a copy of what was merged to the very-long-not-so-meaningful name is still possible

VourMa · 2024-11-25T14:40:00Z

What if we do CMSSW_14_2_X_LST_batch1_devel?

Maybe just drop batch1?

slava77 · 2024-11-25T14:49:21Z

What if we do CMSSW_14_2_X_LST_batch1_devel?

Maybe just drop batch1?

in my comment I had in mind that this branch will become a topic branch for a PR to CMSSW.

There's a benefit of also having a branch for general LST integration; there I think both batch1 and devel can be dropped.

VourMa · 2024-11-25T14:59:17Z

in my comment I had in mind that this branch will become a topic branch for a PR to CMSSW.
There's a benefit of also having a branch for general LST integration; there I think both batch1 and devel can be dropped.

Sorry, I must have missed your comment because I hadn't reloaded the page previously. I think that this makes sense in general but I would like to understand some details with a specific example:

Should that "general LST integration" branch be the target for my "HLT config" PR?
If so, how can the the "rebased developments" PR go in separate PRs in CMSSW but also be reviewed in parallel with the "HLT config" PR?
If not, which branch should be my target?

slava77 · 2024-11-25T15:29:37Z

Should that "general LST integration" branch be the target for my "HLT config" PR?
If so, how can the the "rebased developments" PR go in separate PRs in CMSSW but also be reviewed in parallel with the "HLT config" PR?
If not, which branch should be my target?

in #94
the only change in LST is in RecoTracker/LSTCore/interface/Constants.h, while the rest is elsewhere. It may be better to not combine LST-only changes with quite extensive updates outside. So, for a CMSSW review target, I think it's better to submit #94 separately

ariostas · 2024-11-25T19:27:19Z

The last batch of rebases have a conflict with this PR. Is this ready to be merged? If not, I'll wait until this is merged.

Add Occupancy Branches

6c226e8

Co-authored-by: Gavin Niendorf <[email protected]>

ariostas commented Nov 20, 2024

View reviewed changes

ariostas requested review from VourMa and GNiendorf November 20, 2024 15:33

slava77 reviewed Nov 20, 2024

View reviewed changes

Changes to make standalone run also from within a CMSSW area

7fc7606

Co-authored-by: Manos Vourliotis <[email protected]>

VourMa reviewed Nov 20, 2024

View reviewed changes

ariostas force-pushed the rebase_pr45_pr36_pr55 branch from 03cd6c7 to 6ef991f Compare November 20, 2024 17:14

VourMa reviewed Nov 21, 2024

View reviewed changes

Improvements to the README

395e8ca

Co-authored-by: Manos Vourliotis <[email protected]>

ariostas force-pushed the rebase_pr45_pr36_pr55 branch from 6ef991f to 395e8ca Compare November 22, 2024 16:25

VourMa approved these changes Nov 22, 2024

View reviewed changes

ariostas changed the title ~~Rebase PR45, PR36, and PR55~~ Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) #127

Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) #127

ariostas commented Nov 20, 2024

ariostas Nov 20, 2024

ariostas Nov 20, 2024

ariostas Nov 20, 2024

ariostas Nov 20, 2024

slava77 Nov 20, 2024

slava77 Nov 20, 2024

slava77 Nov 20, 2024

slava77 Nov 20, 2024

ariostas Nov 20, 2024

VourMa Nov 20, 2024 •

edited

Loading

ariostas Nov 20, 2024

VourMa Nov 20, 2024

slava77 Nov 20, 2024

ariostas Nov 20, 2024

slava77 Nov 20, 2024

GNiendorf commented Nov 20, 2024

VourMa left a comment

VourMa Nov 21, 2024

ariostas Nov 22, 2024

VourMa Nov 21, 2024

VourMa Nov 21, 2024

ariostas commented Nov 22, 2024

github-actions bot commented Nov 22, 2024

github-actions bot commented Nov 22, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

ariostas commented Nov 25, 2024

ariostas commented Nov 25, 2024

slava77 commented Nov 25, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

ariostas commented Nov 25, 2024

		std::stringstream search_path;
		search_path << std::getenv("CMSSW_SEARCH_PATH");


		Then all that is left to do is set some environment variables. We give an example of how to do this in lnx7188/cgpu-1.
		/data2/segmentlinking/CMSSW_14_1_0_pre0/step2_21034.1_100Events.root

Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) #127

Are you sure you want to change the base?

Add occupancy branches + standalone improvements (rebase PR45, PR36, and PR55) #127

Conversation

ariostas commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VourMa Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GNiendorf commented Nov 20, 2024

VourMa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariostas commented Nov 22, 2024

github-actions bot commented Nov 22, 2024

github-actions bot commented Nov 22, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

ariostas commented Nov 25, 2024

ariostas commented Nov 25, 2024

slava77 commented Nov 25, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

VourMa commented Nov 25, 2024

slava77 commented Nov 25, 2024

ariostas commented Nov 25, 2024

VourMa Nov 20, 2024 •

edited

Loading