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

feat: Decouple DD4hep detector construction from ACTS #1241

Merged
merged 22 commits into from
May 10, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented May 3, 2022

This is done via a new glue library ActsDD4hep that lives here.

image

commit 3f10f37
Author: Andreas Salzburger <[email protected]>
Date:   Tue Apr 12 15:17:57 2022 +0200

    add possibility of initial covariance inflation

commit bd98135
Author: Paul Gessinger <[email protected]>
Date:   Tue Apr 12 11:06:53 2022 +0200

    format fix

commit 1d76476
Author: Paul Gessinger <[email protected]>
Date:   Tue Apr 12 11:06:45 2022 +0200

    add argument parser

    add argument for output dir

commit 729f2c7
Author: Paul Gessinger <[email protected]>
Date:   Tue Apr 12 11:06:22 2022 +0200

    switch to Path for the input files

commit dabd75f
Author: Andreas Salzburger <[email protected]>
Date:   Tue Apr 12 09:07:56 2022 +0200

    change eta region, add parameter inflation

commit d28ba46
Merge: 0d8d349 394cfdb
Author: Andreas Salzburger <[email protected]>
Date:   Mon Apr 11 13:16:44 2022 +0200

    Merge branch 'main' into feat-odd-python-example

commit 0d8d349
Author: Andreas Salzburger <[email protected]>
Date:   Mon Apr 11 13:12:35 2022 +0200

    adding full_chain example for the ODD
@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label May 3, 2022
@paulgessinger paulgessinger added this to the next milestone May 3, 2022
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #1241 (40151ea) into main (8948fa3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1241   +/-   ##
=======================================
  Coverage   47.89%   47.89%           
=======================================
  Files         375      375           
  Lines       19588    19588           
  Branches     9214     9214           
=======================================
  Hits         9382     9382           
  Misses       3822     3822           
  Partials     6384     6384           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@paulgessinger paulgessinger changed the title Decouple DD4hep detector construction from ACTS feat: Decouple DD4hep detector construction from ACTS May 4, 2022
@paulgessinger paulgessinger marked this pull request as ready for review May 5, 2022 07:34
@paulgessinger
Copy link
Member Author

Still have to sort out the downstream build scenario. Currently the ActsDD4hep glue lib cmake config does not get installed in this scenario.

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label May 6, 2022
@paulgessinger
Copy link
Member Author

@asalzburger can you review this?

@asalzburger asalzburger self-requested a review May 10, 2022 14:32
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

This is a straight forward move of the ActsExtension.
Approved.

@asalzburger
Copy link
Contributor

I suppose this one is updated with the ODD PR - and can go in?

@paulgessinger
Copy link
Member Author

Yes ODD should be all up to date.

@kodiakhq kodiakhq bot merged commit 47f9817 into acts-project:main May 10, 2022
@paulgessinger paulgessinger deleted the dd4hep_decouple branch May 10, 2022 14:41
@timadye
Copy link
Contributor

timadye commented May 11, 2022

Hi @paulgessinger,

Very nice to see this all sorted out. However, with this update, locally-run pytest started failing for my setup:

E               RuntimeError: Unable to find OpenDataDetector factory library. You might need to point LD_LIBRARY_PATH at it
Examples/Scripts/Python/common.py:46: RuntimeError

I think there are two issues here:

  1. I was building with -DACTS_BUILD_EXAMPLES_DD4HEP=on, but not -DACTS_BUILD_ODD=on. Of course that won't support the ODD any more, but maybe the tests like test_odd in test_detectors.py should be disabled based on ACTS_BUILD_ODD, rather than ACTS_BUILD_EXAMPLES_DD4HEP. (Though actually those tests (like dd4hepEnabled) aren't directly on the build options.)
  2. I didn't have build/thirdparty/OpenDataDetector/factory in my $LD_LIBRARY_PATH. It looks like getOpenDataDetector is supposed to set that automatically, but that doesn't seem to work. Can this be fixed, or do we need to add this as an extra instruction or setup.sh.

I worked round both these issues, and have the ODD tests run in pytest, so it's no big deal for me. I hope it's still useful for me to comment on this, and it's OK to comment here in the already-merged PR. Of course if you think it's worth it, I could submit an issue.

@paulgessinger
Copy link
Member Author

paulgessinger commented May 11, 2022

Unfortunately, you'll have to manually adjust your LD_LIBRARY_PATH variable to where the ODD library and components file is located.

I was unable to handle this automatically at the python level. We could indeed try to be smart with our python setup.sh, or add an additional one. Note that you have the option of building ODD separately now, and ODD produces its own setup.sh that should set this up correctly. I don't think it's present in the integrated build though.

@timadye
Copy link
Contributor

timadye commented May 11, 2022

Yes, maybe the python setup.sh is the best place. I was thinking about this_acts.sh, but I guess people don't use that - and anyway for me it just adds /usr/local/lib64, not $BUILD_DIR/lib64.

I have my own setup.sh script, so added build/thirdparty/OpenDataDetector/factory there.

@timadye
Copy link
Contributor

timadye commented May 11, 2022

Remaining Python questions:

  • If the getOpenDataDetector LD_LIBRARY_PATH setting doesn't work any more, maybe better to replace it with a check and error.
  • Can we change the relevant dd4hepEnabled tests to oddEnabled tests?

@paulgessinger paulgessinger modified the milestones: next, v19.1.0 May 25, 2022
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.

3 participants