Batch 3 for updates to LST integration in cms-sw

VourMa commented 3 months ago

ef48f7427d3d08b8c92f6554883a5b5f4b72f220

Resolves https://github.com/cms-sw/cmssw/pull/45117#issuecomment-2173437010.

149c628fc13ab77dbcde0cbde9e7880c38c560ee

Should resolve the conflict regarding the workflows, as shown in the screenshot, where I attempt to merge in the cmssw master a branch that is a copy of CMSSW_14_1_0_pre3_LST_X_LSTCore_realfiles with this commit applied:

Screenshot from 2024-06-28 22-25-21

236ad8c9f19a9376f42f540779a4e6a4bb1b24ce and a97714a574a35371b982d6d80ff4deff8593c9bd and c563eb04a71057bd169d0997e406e3d8b57417e9 and a3a8054945636973191c9c4cc76e2ed36e271e1e and 7cfe3a5bccce359c4f7a9cb073cf4533883ca481

Resolve all of Matt's comments, applying the suggestions wherever they applied. These concern:

Introduction of constants for the number object hits and layers
Explanation of several magic numbers in the code (they are more cases which I didn't manage to figure out, and these are discussed in #30)
Simplification of some loops.
Elimination of sqrts throughout the code by using squared quantities when this is possible.

When(Before) we merge, we should squash these two commits. I didn't do it at this stage, so as not to create conflicts with the local branch of people who may be already working on this.

4d7e54a369f7040cb559e5f51aa09fe812382355

Remove LST_IS_CMSSW_PACKAGE dc13ac526b32b4c3ad1b391fb7b97511ce7802ea
configurable dup cleaning needed to more flexibly setup the offline and HLT workflows f5554858b3538e056909dc98fa7c6930e9a0d704
typo in T3 code 14af98162c821fc69026e1b11acdb6f9859a42c6

ae67290cb97aed9e53efc9e340c7bd030b2f7748

Resolves comments on CMSSW PR regarding the LST workflows.

e0acfa56fd37bb85b32ae1973720be32806af0d0 and a3f65dacbb686313574fb36be5f4d421e3333284 and eb5de05acde834fecafba919b609f6d416476c25 and f46b6cb13c9c14df422fc16e7ed3cf7e0755a6bc and f47212f13ca208b20aa0fcd5f30ec5b411a647e1 and 86503a29d741a5654caf588fedd7240addc7dff6

Αddress some comments regarding the ESProducer that were made in the CMSSW PR. Specifically, these comments:
Modify the code so that all the data is set up on the host and then the framework takes care of copying the ESData to the devices.

To be squashed together.

YonsiG commented 2 months ago

I checked the parts with magic numbers and T5 rz chi2 functions. These parts are good:) Thank you Manos

slava77 commented 2 months ago

/run all

github-actions[bot] commented 2 months ago

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     44.2    323.5    122.6     47.7     96.1    503.2    134.0    159.0    100.1      1.7    1532.1     984.7+/- 264.4     417.8   explicit_cache[s=4] (target branch)
   avg     44.5    321.5    118.2     47.2     96.2    509.0    131.3    148.7     99.0      2.4    1518.0     964.5+/- 258.3     415.2   explicit_cache[s=4] (this PR)

github-actions[bot] commented 2 months ago

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

OOTB All Tracks

Efficiency and fake rate vs pT, eta, and phi

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

VourMa commented 2 months ago

Thanks for the comments, @slava77! Everything makes sense and I will follow up on them.

However, I see differences in the comparisons. Could you clarify what is the branch we compare to? Because I couldn't find any non-technical change. Did anyone else manage to?

slava77 commented 2 months ago

However, I see differences in the comparisons. Could you clarify what is the branch we compare to? Because I couldn't find any non-technical change. Did anyone else manage to?

k2Rinv1GeVf-related changes are 0.3-0.6% fractional change in math. Even the sqrt-related changes are not bitwise identical

VourMa commented 2 months ago

k2Rinv1GeVf-related changes are 0.3-0.6% fractional change in math. Even the sqrt-related changes are not bitwise identical

Indeed, we can expect changes from those, thank you for checking. I was mostly worried about missing something while converting things. We should be ok with the level of the changes then?

slava77 commented 2 months ago

I added the following in the PR description after #58 was merged:

4d7e54a369f7040cb559e5f51aa09fe812382355

Remove LST_IS_CMSSW_PACKAGE dc13ac526b32b4c3ad1b391fb7b97511ce7802ea
configurable dup cleaning needed to more flexibly setup the offline and HLT workflows f5554858b3538e056909dc98fa7c6930e9a0d704
typo in T3 code 14af98162c821fc69026e1b11acdb6f9859a42c6

VourMa commented 2 months ago

I think I dealt with all of the follow up comments on my commits, so people may want to recheck the status.

Next up for me is to try and deal with the comments on the workflows.

slava77 commented 2 months ago

/run all

github-actions[bot] commented 2 months ago

There was a problem while building and running in standalone mode. The logs can be found here.

github-actions[bot] commented 2 months ago

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

VourMa commented 2 months ago

/run all

github-actions[bot] commented 2 months ago

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     43.8    322.7    123.3     50.3     97.7    502.6    133.2    157.5    100.3      1.7    1533.0     986.6+/- 262.7     419.7   explicit_cache[s=4] (target branch)
   avg     43.9    323.0    118.7     48.8     96.2    498.3    125.8    147.8    100.2      2.9    1505.6     963.4+/- 254.2     416.2   explicit_cache[s=4] (this PR)

VourMa commented 2 months ago

Sorry for the mess with the commits. I updated the branch to solve all of your comments, applied code format/checks, and dealt with all of the LST-workflow-related comments we received in the CMSSW PR.

github-actions[bot] commented 2 months ago

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 commented 2 months ago

/run checks

VourMa commented 2 months ago

I ran the code format/checks (that had failed the previous time, due to my mistake) and they ran fine, and updated the PR description based on the description of Andres's PR. If there are no follow up comments, I think a squash of commits according to the description is needed (let me know if I should do it) and we are good to go.

slava77 commented 2 months ago

/run all

github-actions[bot] commented 2 months ago

There was a problem while building and running in standalone mode. The logs can be found here.

slava77 commented 2 months ago

There was a problem while building and running in standalone mode. The logs can be found here.

docker: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.

aha there are some limits

slava77 commented 2 months ago

Anonymous and Free Docker Hub users are limited to 100 and 200 container image pull requests per six hours.

@ariostas are we possibly hitting this?

ariostas commented 2 months ago

are we possibly hitting this?

I'll look into it. I got the same error at some point, but when I restarted the job it worked. I'll just try running it again to see if it works.

/run standalone

github-actions[bot] commented 2 months ago

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     43.4    322.8    121.9     48.6     96.5    505.5    133.7    162.9     99.9      2.0    1537.4     988.5+/- 262.8     417.1   explicit_cache[s=4] (target branch)
   avg     43.4    324.5    117.8     48.9     94.4    498.0    124.6    151.1    101.3      2.9    1506.7     965.3+/- 250.2     411.1   explicit_cache[s=4] (this PR)

github-actions[bot] commented 2 months ago

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.

SegmentLinking / cmssw