Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MkFit] Phase2 geometry & String configurable standard functions #39866

Merged
merged 24 commits into from Nov 21, 2022

Conversation

osschar
Copy link
Contributor

@osschar osschar commented Oct 26, 2022

PR description:

This PR introduces:

1. Support for 2029 geometry

including its extraction, special partition-seed method (+ other / minimal setup needed to run on initialStep for phase2).

2. Introduce standard functions into IterationConfig

They are accessible via a string name that can be set from JSON (or eventually via CMSSW config)

class IterationConfig {
...
    // Standard functions
    clean_seeds_func m_seed_cleaner;
    partition_seeds_func m_seed_partitioner;
    filter_candidates_func m_pre_bkfit_filter, m_post_bkfit_filter;
    filter_duplicates_func m_duplicate_cleaner;

    // Names for Standard functions that get saved to / loaded from JSON.
    std::string m_seed_cleaner_name;
    std::string m_seed_partitioner_name;
    std::string m_pre_bkfit_filter_name, m_post_bkfit_filter_name;
    std::string m_duplicate_cleaner_name;
...
};
  • When those are not set for a specific iteration, they get skipped (without era specific if-iteration-equals constructs that we have now in top-level steering function).
  • Introduce standard function catalogs that get filled through static initialization in implementation files and then get used/demangled when iteration-config is being finalized at runtime.
  • When a standard function name is empty, the std::function remains unset and does not get called. When a name is non-empty and the function is not found in the catalog an exception is thrown.

All changes for phase1 are technical, i.e., no changes in physics performance are expected.

Parallel PR has also been made to cms-data/RecoTracker-MkFit to include required changes in MkFit JSON configuration files: cms-data/RecoTracker-MkFit#10

See also the dedicated presentation in Tracker DPG / Tracking POG meeting on Nov. 2nd.

PR validation:

Comparison of all 10 tracking iterations run with mkFit:
http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/Phase2/MTV_TTbarPU_2022_mkFitDevel-matevz/

After doing the requested 2017->phase1, 2029->phase2 substitutions (no additional change):
http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/Phase2/MTV_TTbarPU_2022_mkFitDevel-matevz2/

osschar and others added 13 commits October 6, 2022 09:04
…s from MkFitCore/standalone/Makefile; remove obsolete tkNtuple/Makefile (build included in top-level MkFitCMS/standalone/Makefile).
* mkFit.cc: Make use-dead-modules really optional. They are hardcoded
for CMS-2107, some unknown setup.

* MkBuilder.cc: Make some high-volume debug output optional.

* val_scripts/validation-cmssw-benchmarks-multiiter.sh:
- Add new sample TTbar_Phase2.
- Add variable for numiters
- Make --use-dead-modules configurable
- Add var for exstra mkFit args to pass in --geom CMS-2029
- Detect Fedora and use lower n_threads / maxvu. Use AVX2 instead
  of AVX_512. This should be configurable in some other way.

* plotting/Common.hh: Do not try to plot STD in MIMI case - exported
via an env variable for lack of a better idea.
We should support STD in MIMI with upcoming configurability changes.

* validation-desc.txt: Add a prelimiary note describing how I ran
the Phase2 test. Standalone validation has not been tried since the
move into CMSSW so this should be worked on further.

* xeon_scripts/init-env.sh:
Detect Fedora and use el8 binaries in this case.
This should be the default once we drop Centos-7.
…ates to clean_duplicates.

* Rename find_duplicates functions to clean_duplicates prefix.

* Remove obsolete iteration-id dependant steering code.
* Update README.md with changes / std function name config dexcriptions.

* Move seed-cleaning and duplicate-cleaning parameters out of IterationParams
  into IterationConfig. Prefix them with sc_ and dc_.

* When getting std function by name, throw an exception if name string is
  non-empty and the function does not exist in the registry.

* Add m_layer member to IterationLayerConfig.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39866/32770

  • This PR adds an extra 184KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @osschar (Matevž Tadel) for master.

It involves the following packages:

  • RecoTracker/MkFit (reconstruction)
  • RecoTracker/MkFitCMS (reconstruction)
  • RecoTracker/MkFitCore (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@VourMa, @makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @missirol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Oct 26, 2022

type tracking

@slava77
Copy link
Contributor

slava77 commented Oct 27, 2022

test parameters:

@slava77
Copy link
Contributor

slava77 commented Oct 27, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39866/33075

  • This PR adds an extra 24KB to repository

  • Found files with invalid states:

    • RecoTracker/MkFitCMS/standalone/Geoms/CMS-2029.cc:
    • RecoTracker/MkFitCMS/src/MkSeedPartitioners-2017.cc:
    • RecoTracker/MkFitCMS/src/MkSeedPartitioners-2029.cc:

@slava77
Copy link
Contributor

slava77 commented Nov 17, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

Pull request #39866 was updated. @mandrenguyen, @clacaputo can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Nov 17, 2022

following the discussion in the ORP of Nov 15 I added clarification comments in the code in 8ab5c78

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d391b8/29068/summary.html
COMMIT: 8ab5c78
CMSSW: CMSSW_12_6_X_2022-11-16-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39866/29068/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d391b8/29068/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d391b8/29068/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3417074
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3417046
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 4.224 KiB( 47 files compared)
  • DQMHistoSizes: changed ( 11634.0,... ): 0.530 KiB Physics/NanoAODDQM
  • DQMHistoSizes: changed ( 13234.0,... ): 0.348 KiB Physics/NanoAODDQM
  • Checked 206 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@slava77
Copy link
Contributor

slava77 commented Nov 18, 2022

@cms-sw/reconstruction-l2 @mandrenguyen @rappoccio
please clarify if this is going to pre5.
It would be nice to have an update in comments in the code approved faster than a couple of days

@mandrenguyen
Copy link
Contributor

+1
resign, only comments added, which look fine

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@slava77
Copy link
Contributor

slava77 commented Nov 19, 2022

+1 resign, only comments added, which look fine

Thank you @mandrenguyen

@rappoccio @perrotta
is there something else left to address your previous concerns/comments or is it just waiting for Monday (working day)?

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8a956e6 into cms-sw:master Nov 21, 2022
@Martin-Grunewald
Copy link
Contributor

Seems this PR causes problems in IB tests:

----- Begin Fatal Exception 22-Nov-2022 07:38:30 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 355558 lumi: 224 event: 215655207 stream: 0
   [1] Running path 'dqmoffline_step'
   [2] Prefetching for module DQMMessageLogger/'DQMMessageLogger'
   [3] Prefetching for module LogErrorHarvester/'logErrorHarvester'
   [4] Prefetching for module HcalNoiseInfoProducer/'hcalnoise'
   [5] Prefetching for module FastjetJetProducer/'ak4PFJets'
   [6] Prefetching for module PFLinker/'particleFlow'
   [7] Prefetching for module PFProducer/'particleFlowTmp'
   [8] Prefetching for module PFBlockProducer/'particleFlowBlock'
   [9] Prefetching for module PFElecTkProducer/'pfTrackElec'
   [10] Prefetching for module GsfTrackProducer/'electronGsfTracks'
   [11] Prefetching for module CkfTrackCandidateMaker/'electronCkfTrackCandidat\
es'
   [12] Prefetching for module MeasurementTrackerEventProducer/'MeasurementTrac\
kerEvent'
   [13] Prefetching for module JetCoreClusterSplitter/'siPixelClusters'
   [14] Prefetching for module PrimaryVertexProducer/'firstStepPrimaryVerticesP\
reSplitting'
   [15] Prefetching for module TrackProducer/'initialStepTracksPreSplitting'
   [16] Prefetching for module MkFitOutputConverter/'initialStepTrackCandidates\
PreSplitting'
   [17] Prefetching for module MkFitProducer/'initialStepTrackCandidatesMkFitPr\
eSplitting'
   [18] Calling method for EventSetup module MkFitIterationConfigESProducer/'in\
itialStepTrackCandidatesMkFitConfigPreSplitting'
Exception Message:
A std::exception was thrown.
[json.exception.out_of_range.403] key 'm_seed_cleaner_name' not found
----- End Fatal Exception -------------------------------------------------

@Martin-Grunewald
Copy link
Contributor

Perhaps this needs to be added?

Parallel PR has also been made to cms-data/RecoTracker-MkFit to include required changes in MkFit JSON configuration files: https://github.com/cms-data/RecoTracker-MkFit/pull/10

cmsbuild added a commit to cms-data/RecoTracker-MkFit that referenced this pull request Nov 22, 2022
Update JSON files in line with changes done in cms-sw/cmssw#39866
@perrotta
Copy link
Contributor

Perhaps this needs to be added?

Parallel PR has also been made to cms-data/RecoTracker-MkFit to include required changes in MkFit JSON configuration files: https://github.com/cms-data/RecoTracker-MkFit/pull/10

Yes, it was forgotten. It is merged now. Unfortunately not in time for the 1100 IB
@smuzaffar could you please delay them to, e.g., 1200, so that the needed data file can be picked by mkfit? Otherwise we'll still have those massif failures, making the 12_6_X IBs quite useless

@smuzaffar
Copy link
Contributor

@perrotta , I have restarted 11h00 build [a] and it should now include the cms-sw/cmsdist#8191

[a] https://cmssdt.cern.ch/jenkins/job/build-any-ib/142101/console

11:27:55 HEAD is now at 0b4ad91 Merge pull request #8191 from cms-sw/update-RecoTracker-MkFit-to-V00-10-00

@perrotta
Copy link
Contributor

@perrotta , I have restarted 11h00 build [a] and it should now include the cms-sw/cmsdist#8191

Thank you @smuzaffar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet