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.

Sign up for GitHub

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

Jump to bottom

[cxxmodules] Use the global module index only when no rootmap candidate is found #9592

Merged
merged 1 commit into from
Jan 26, 2022
Merged

[cxxmodules] Use the global module index only when no rootmap candidate is found #9592

merged 1 commit into from
Jan 26, 2022

Conversation

vgvassilev
Copy link
Member

In the hybrid mode when we use ROOT with modules and third party software without modules we have two name resolution systems: the global module index (GMI) and the rootmaps. In case an identifier is defined in both (eg TMVA::Event and X::Event), the GMI will take priority and load the TMVA module without later allowing the system to look for other candidates.

This patch uses the GMI only when no rootmap file has told ROOT that it provides names for the given namespace.

This fixes #9583

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

  • [2022-01-16T08:19:33.952Z] 53/2226 Test Rename print_wrapper to get_wrapper_code. #642: tutorial-hist-fillrandom ..........................................................................***Failed Error regular expression found in output. Regex=[: error:] 0.42 sec
  • [2022-01-16T08:20:53.334Z] 886/2226 Test Add support for LZ4 in classic build #593: tutorial-graphs-gerrors ...........................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.11 sec
  • [2022-01-16T08:20:53.334Z] 889/2226 Test Change Snapshot option from file cache to TTree auto-flush parameter #595: tutorial-graphs-gmultierrors ......................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.08 sec
  • [2022-01-16T08:20:53.603Z] 890/2226 Test Refactor of test/stressMathCore.cxx to use Google Test #594: tutorial-graphs-gerrors2 ..........................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.19 sec
  • [2022-01-16T08:20:53.603Z] 891/2226 Test gROOTMutex -> RWLock #596: tutorial-graphs-graph .............................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.18 sec

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

Failing tests:

And 12 more

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/soversion.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@vgvassilev
[cxxmodules] Use the global module index only when no rootmap candida…
e94089f 
…te is found

In the hybrid mode when we use ROOT with modules and third party software
without modules we have two name resolution systems: the global module index (GMI)
and the rootmaps. In case an identifier is defined in both (eg TMVA::Event and
X::Event), the GMI will take priority and load the TMVA module without later
allowing the system to look for other candidates.

This patch uses the GMI only when no rootmap file has told ROOT that it
provides names for the given namespace.

This fixes root-project#9583
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

And 4 more

@vgvassilev vgvassilev mentioned this pull request Jan 17, 2022
Closed
1 task
@pcanal
Copy link
Member

pcanal commented Jan 17, 2022

@vgvassilev I see In case an identifier is defined in both (eg TMVA::Event and X::Event), the GMI will take priority and load the TMVA module without later allowing the system to look for other candidates.. Could you explicit in the commit log why an unrelated fully qualified name (TMVA::Event) takes precedence over another fully qualified name (X::Event) and why this is usually not an issue (i.e. Is there any other cases (other than module vs rootmap) where this could lead to problems?)

@vgvassilev
Copy link
Member Author

@vgvassilev I see In case an identifier is defined in both (eg TMVA::Event and X::Event), the GMI will take priority and load the TMVA module without later allowing the system to look for other candidates.. Could you explicit in the commit log why an unrelated fully qualified name (TMVA::Event) takes precedence over another fully qualified name (X::Event)

The GMI contains Identifier->list_of_modules mapping. This means that a lookup for Event will give all modules in a fully modularized system. However, until today, we have not considered (nor had the tests for) the hybrid approach where parts of the names are resolved from a rootmap files.

@vgvassilev
Copy link
Member Author

@smuzaffar, could you test this PR for CMSSW as it would likely end up in v6-26 and is in the core of ROOT name resolution mechanism.

cc: @davidlange6

@smuzaffar
Copy link
Contributor

@vgvassilev , cmssw tests for latest root master + this change are now running via cms-sw/cmsdist#7558

@smuzaffar
Copy link
Contributor

@vgvassilev , this lookgs good in cmssw root master branch based IBs.

@vgvassilev
Copy link
Member Author

@smuzaffar, thanks a lot for the prompt feedback!

@smuzaffar
Copy link
Contributor

@vgvassilev , I also tested this for our CXXMODULE IBs and most of relvals failed with errors like

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d3a1b7/21842/runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step1_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log

----- Begin Fatal Exception 21-Jan-2022 04:35:59 CET-----------------------
An exception of category 'DictionaryNotFound' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=BeamHaloProducer label='generator'
   [2] Calling ProductRegistryHelper::addToRegistry, checking dictionaries for produced types
Exception Message:
No data dictionary found for the following classes:

  map<string,ULong64_t>

Most likely each dictionary was never generated, but it may
be that it was generated in the wrong package. Please add
(or move) the specification '<class name="whatever"/>' to
the appropriate classes_def.xml file along with any other
information needed there. For example, if this class has any
transient members, you need to specify them in classes_def.xml.
Also include the class header in classes.h

A type listed above might or might not be the same as a
type declared by a producer module with the function 'produces'.
Instead it might be the type of a data member, base class,
wrapped type, or other object needed by a produced type. Below
is some additional information which lists the types declared
to be produced by a producer module that are associated with
the types whose dictionaries were not found:

  edm::HepMCProduct

----- End Fatal Exception -------------------------------------------------

this is a different error than what we get in out CXXMODULE IBs https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/raw/slc7_amd64_gcc10/CMSSW_12_3_CXXMODULE_X_2022-01-19-2300/pyRelValMatrixLogs/run/135.1_TTbar_13+TTbarFS_13+HARVESTUP15FS+MINIAODMCUP15FS/step3_TTbar_13+TTbarFS_13+HARVESTUP15FS+MINIAODMCUP15FS.log ?

@vgvassilev
Copy link
Member Author

@smuzaffar, indeed, now the failure looks very basic.

@davidlange6, I remember we've seen such a failure but did we manage ever to solve it?

Axel-Naumann
Axel-Naumann approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vgvassilev vgvassilev merged commit 84cc694 into root-project:master Jan 26, 2022
@davidlange6
Copy link
Contributor

davidlange6 commented Jan 26, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Axel-Naumann Axel-Naumann Axel-Naumann approved these changes

@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner

Assignees

@vgvassilev vgvassilev

Labels
in:C++ modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Interpreter autoload lookup failure when runtime_cxxmodules=ON
7 participants
@vgvassilev @phsft-bot @pcanal @smuzaffar @davidlange6 @Axel-Naumann @jalopezg-git