eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 28 forks source link

ACTS versions running ahead of those supported in EICrecon #623

Closed wdconinc closed 8 months ago

wdconinc commented 1 year ago

Is your feature request related to a problem? Please describe. The tracking working group has requested pinning the ACTS version to 21.1.0. At the same time, ACTS is now already at version 24.0.0, with typically significant work required when upgrading major versions. Past experience indicates that letting this work pile up is a headache (think of it as upgrading from geant4 10.4 to 11.1 and having to deal with all the interface changes).

Describe the solution you'd like The work of upgrading to newer versions of ACTS is best approached piecemeal, with a running branch (based off main) with upgrades required for 22, a branch (based off 22) with upgrades required for 23, etc etc. Periodically we would trigger a rebase cascade.

The underlying issue is with the lack of tracking benchmarks that we can use to determine when an upgrade is ok to perform, and when it isn't ok to upgrade.

Describe alternatives you've considered We can do the changes all at once, but this is going to be a headache for an expert instead of an opportunity to learn about ACTS itself (which version upgrades are good for).

wdconinc commented 1 year ago

This is becoming particularly relevant as ACTS 21.1.0 fails to compile with other upgraded components in the environment. Those bugs have been fixed, but there are maintenance costs associated with pinning older versions which are not always apparent to the tracking working group.

wdconinc commented 1 year ago

Comments from @ShujieL @veprbl invited.

wdconinc commented 1 year ago

ACTS 25 is now out too :-\

wdconinc commented 1 year ago

Adding to the reason to upgrade to 26 or later:

wdconinc commented 1 year ago

The current version of Acts (21.1.1) has crashes under certain conditions (e.g. https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/jobs/1724160) which prevent us from compiling for the actual CPU architecture that is used on OSG nodes. This means that we run for an older CPU architecture which does not have certain vectorization optimizations that Acts uses.

It is very well possible that this bug has been fixed in newer versions of Acts already but we are unable to verify that. That would be the first step in fixing the issue linked above.

veprbl commented 1 year ago

I gave a try at bumping EICrecon to support Acts 21.x. The API is not compatible between 20.x and 21.x. I think a good path for migration is to implement a parallel support for both versions using #ifdefs, then test for possible effects.

The current version of Acts (21.1.1) has crashes under certain conditions (e.g. https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/jobs/1724160) which prevent us from compiling for the actual CPU architecture that is used on OSG nodes. This means that we run for an older CPU architecture which does not have certain vectorization optimizations that Acts uses.

Have you tried compiling with -march=x86-64-v4?

wdconinc commented 1 year ago

I haven't tried with v4. It's not used widely enough on OSG for it to be viable for us (would reduce pool to 30% of what we have access to with v2 and v3; with v3 we lose about 5% of what we have access to with v2).

I think whatever breaks in v3 would probably break in v4 as well since the CPU features are incremental (and if it doesn't break I probably wouldn't trust it for a production run). I brought this up with the Acts folks at CHEP and there is no testing of higher CPU features sets much in CI, and even the WLCG runs on bare x64_64.

wdconinc commented 1 year ago

We're at 21.1.1 already, by the way. And ifdefs are hard to do because of the types of changes. During ATHENA we've been able to do a few changes that way for single version upgrades but it got cumbersome quickly.

veprbl commented 1 year ago

Correction, I was doing 21.1.0 -> 22.0.1

DraTeots commented 1 year ago

@veprbl What is your status on it? It was recently discussed in the tracking-reconstruction group and corresponding TODO list and I will try to push it to later versions (the latest eventually). I wanted to synchronize on that so no work done is wasted.

veprbl commented 1 year ago

I'm not working on this right now. I'd check with @wdconinc too just in case.

wdconinc commented 1 year ago

I'm not working on this. I was going to assist @Simple-Shyam with this task, but I see you've already answered his question on Mattermost of whether you are taking this over.

Simple-Shyam commented 1 year ago

Yes, I am looking at it as discussed @wdconinc in mattermost and also in meeting. I need some of the steps.

  1. I need to install epic as usual locally.
  2. I need to install acts with the new version.
  3. I need to compile EICRecon with new Acts path. Can you please send me the command for the step 2 and 3? I seen you posted on zoom which are lost. Let me see what are the errors and process for the fixing of them.
wdconinc commented 1 year ago

No need to install epic. That's independent (no changes there are expected).

Here is how to compile acts locally, than use it for a local eicrecon installation:

cd ~
git clone --branch v28.1.0 https://github.com/acts-project/acts
cd acts
cmake -Bbuild -S. -DACTS_BUILD_PLUGIN_DD4HEP=ON -DCMAKE_INSTALL_PREFIX=~/acts/install
make -Cbuild -j8 install
cd ~
git clone https://github.com/eic/EICrecon
cd EICrecon
cmake -Bbuild -S. -DActs_ROOT=~/acts/install
make -Cbuild -j8 install

Changes paths and -j flags as needed.

Simple-Shyam commented 1 year ago

Thanks @wdconinc for quick reply on Sunday. I am just compiling it.

Simple-Shyam commented 1 year ago

I just checked that the compilation is failed in this case with v28.1.0, I will approach step by step to understand and fix the issue.

wdconinc commented 1 year ago

Yeah, not a surprise that the move to 28.1.0 fails out of the gate. I'd do this in smaller steps, if possible, fixing the issues in EICrecon along the way and making dedicated commits for each version change.

Simple-Shyam commented 1 year ago

Thanks, I am approaching that way only otherwise it's very hard to understand for me. I am approaching in smaller steps.

wdconinc commented 12 months ago

Acts logging to Spdlog in #1012 is unable to define the name and clone functionality introduced in the last refactoring of the Acts logger in v22.0.0.

wdconinc commented 8 months ago

This issue is not applicable anymore. We currently support up to the most recent version of Acts (31.2.0 as of writing).