SegmentLinking / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1 stars 1 forks source link

Changes to make standalone run also from within a CMSSW area #36

Closed VourMa closed 2 weeks ago

VourMa commented 3 weeks ago

When we are within a CMSSW area, we would like the data files to be picked up from the CMSSW_SEARCH_PATH. This was not possible with the current setup.sh, as it was resetting the search path. This behavior is now modified, partially according to https://github.com/SegmentLinking/cmssw/pull/30#discussion_r1621064078, so it may be a controversial change and feedback is welcome. That also means that we want to change the order with which we check the potential directories in which the data files may reside, and this is also taken care of in this PR.

Once we agree on the changes, I can upload a README with instructions on how to test the standalone both within and without a CMSSW area.

VourMa commented 2 weeks ago

@ariostas Please let me know what you think of this one. I would like to decide on whether this looks fine or not before writing the instructions for how to test PRs in CMSSW locally.

ariostas commented 2 weeks ago

@VourMa The changes look good to me. It's up to you whether we should remove LST_BASE now or just do it later.

ariostas commented 2 weeks ago

It shouldn't break the CI, but just to make sure.

/run all

github-actions[bot] commented 2 weeks ago

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

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

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.1    325.1    123.1     47.6     95.5    503.5    132.4    158.1    100.1      1.8    1531.4     983.9+/- 256.0     415.1   explicit_cache[s=4] (target branch)
   avg     44.0    323.3    121.7     48.0     95.9    505.7    133.2    156.8     99.4      1.9    1529.7     980.0+/- 257.3     416.5   explicit_cache[s=4] (this PR)
slava77 commented 2 weeks ago

I'm not sure this should be just in the devel branch; the updates are supposedly in the context of the cms-sw PR to get things run in standalone.

However, since the CI runs already, I guess this change can be applied later (no need to add this to the batch2 branch as well). Is my understanding correct?

VourMa commented 2 weeks ago

It's up to you whether we should remove LST_BASE now

I can remove it now to be done with it. I will add one more commit to this PR a bit later today.

I'm not sure this should be just in the devel branch; the updates are supposedly in the context of the cms-sw PR to get things run in standalone.

This can definitely go in the batch 2 as well, and maybe it finalizes the "Modifications to the standalone scripts" to-do that we have set in the cms-sw PR description, although I am not sure of the best way to do it. Let me know what you prefer and I will do it.

github-actions[bot] commented 2 weeks 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 weeks ago

@slava77 @ariostas If there are no more comments, please approve the PR but do not merge it yet - I would like to push the README with the instructions.

VourMa commented 2 weeks ago

I think this is ready to be merged now. People can test the README instructions to check whether they work for them.

VourMa commented 2 weeks ago

General reply on the comments for the README: both are valid, and especially for the first one, I don't know how it worked before - this was just (blindly) keeping the relevant README section from what already existed. It is clear that it needs to be updated. Having said that, I am not willing to come up with all the details for the CVMFS-less configuration at this stage. My proposal is to comment out that part of the README and come back to it if needed in the future.

slava77 commented 2 weeks ago

/run all

github-actions[bot] commented 2 weeks ago

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

github-actions[bot] commented 2 weeks ago

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

Efficiency vs pT comparison Efficiency vs eta comparison
Fake rate vs pT comparison Fake rate vs eta comparison
Duplicate rate vs pT comparison Duplicate rate vs eta comparison

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.9    323.9    122.7     48.2     95.9    500.3    134.0    156.8    105.8      1.8    1533.2     989.0+/- 259.8     418.0   explicit_cache[s=4] (target branch)
   avg     43.7    325.7    123.4     50.2     97.4    499.9    133.9    158.3    106.7      1.6    1541.0     997.4+/- 263.0     420.8   explicit_cache[s=4] (this PR)
slava77 commented 2 weeks ago

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

@ariostas is there a problem in the CI ? It looks like it failed at checking out the baseline/reference part

ariostas commented 2 weeks ago

is there a problem in the CI ?

Sorry, there were two steps that should have been done in the opposite order. I'm re-running it now.

github-actions[bot] commented 2 weeks 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.