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

fix: Do not use requires in class EDM definition #3731

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

CarloVarni
Copy link
Collaborator

@CarloVarni CarloVarni commented Oct 14, 2024

It looks like this is creating issues in Athena with

In file included from libActsEventDict dictionary payload:146:
In file included from /cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/Athena/25.0.20/InstallArea/x86_64-el9-gcc13-opt/include/ActsEvent/Seed.h:8:
/cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/AthenaExternals/25.0.20/InstallArea/x86_64-el9-gcc13-opt/include/Acts/Acts/EventData/Seed.hpp:17:11: error: requires clause differs in template redeclaration
  requires(N >= 3ul)
          ^
Forward declarations from /cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/Athena/25.0.20/InstallArea/x86_64-el9-gcc13-opt/lib/Athena.rootmap:773:18: note: previous template declaration is here
namespace Acts { template <typename external_spacepoint_t, size_t> class Seed; }

/cc @krasznaa

@Ragansu and @Corentin-Allaire managed to make it work by:

removed requires(N >= 3ul) in Seed.h and Seed.ipp and recompiled ACTS
commented out parts in CxxUtils.h which delt with std::range when cling is enabled. and recompiled Athena with a modified Package Filter.

Quoting:

For information I think part of the issue might be that Cling doesn't like the requiere in the seed definition:
The other issue seem to be that athena redefine the std::range::end function when Cling is used (in CxxUtils/range). Probably because it was not implemented before, but now this create a conflict with newer root version I guess ?

See: https://atlas-art-data.web.cern.ch/atlas-art-data/grid-output/main/Athena/x86_64-el9-gcc13-opt/2024-10-08T2101/InDetPhysValMonitoring/test_run4_acts_vertex_PU200/tarball_logs/payload.stdout

@CarloVarni CarloVarni added this to the next milestone Oct 14, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Event Data Model labels Oct 14, 2024
@CarloVarni CarloVarni added the Bug Something isn't working label Oct 14, 2024
@CarloVarni CarloVarni changed the title refactor: Do not use requires in class EDM definition fix: Do not use requires in class EDM definition Oct 14, 2024
@krasznaa
Copy link
Member

No, let's not be hasty with this. I'm still absolutely sure that we can convince ROOT to accept the current code. Apparently we need a slightly different incantation than what I told you before lunch. But there must be some valid incantation...

@CarloVarni CarloVarni marked this pull request as draft October 14, 2024 12:45
@pbutti
Copy link
Contributor

pbutti commented Oct 29, 2024

Hi @krasznaa,
I would like to revive this PR. Honestly I don't see a major problem with having a static_assert. The issue is that this is blocking ATHENA tests and IDPVM on xAOD produced with the Acts workflow, which I find it quite urgent to resolve.
After this is fixed I propose to add additional CI tests and then check if it is possible to use requires and make it work with root.

Copy link

github-actions bot commented Oct 29, 2024

📊: Physics performance monitoring for e091213

Full contents

physmon summary

@CarloVarni CarloVarni marked this pull request as ready for review October 29, 2024 16:39
@CarloVarni
Copy link
Collaborator Author

cc @paulgessinger

@paulgessinger
Copy link
Member

Ok for me @CarloVarni @pbutti

I'll add the ART test to our CI tests so we catch breakage earlier.

Fixing the root dictionary generation seems orthogonal to working around the issue here.

Copy link

sonarcloud bot commented Oct 29, 2024

@paulgessinger
Copy link
Member

@pbutti @CarloVarni should I backport this to a 37.2.1 or do we wait for 37.3.0 on thursday?

@kodiakhq kodiakhq bot merged commit 0b8e768 into acts-project:main Oct 29, 2024
44 checks passed
@CarloVarni CarloVarni deleted the SeedEdmWithoutRequires branch October 29, 2024 20:30
@pbutti
Copy link
Contributor

pbutti commented Oct 30, 2024

@pbutti @CarloVarni should I backport this to a 37.2.1 or do we wait for 37.3.0 on thursday?

I’m happy with either solution with a slight preference to have this in Athena as soon as possible :)

@CarloVarni
Copy link
Collaborator Author

we can wait for thursday and have it in v37.3.0.

@paulgessinger paulgessinger modified the milestones: next, v37.4.0, v37.3.0 Nov 8, 2024
Rosie-Hasan pushed a commit to Rosie-Hasan/acts that referenced this pull request Nov 13, 2024
It looks like this is creating issues in Athena with
```console
In file included from libActsEventDict dictionary payload:146:
In file included from /cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/Athena/25.0.20/InstallArea/x86_64-el9-gcc13-opt/include/ActsEvent/Seed.h:8:
/cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/AthenaExternals/25.0.20/InstallArea/x86_64-el9-gcc13-opt/include/Acts/Acts/EventData/Seed.hpp:17:11: error: requires clause differs in template redeclaration
  requires(N >= 3ul)
          ^
Forward declarations from /cvmfs/atlas-nightlies.cern.ch/repo/sw/main--ACTS_Athena_x86_64-el9-gcc13-opt/2024-10-09T2101/Athena/25.0.20/InstallArea/x86_64-el9-gcc13-opt/lib/Athena.rootmap:773:18: note: previous template declaration is here
namespace Acts { template <typename external_spacepoint_t, size_t> class Seed; }
```

/cc @krasznaa 

@Ragansu and @Corentin-Allaire managed to make it work by:
> removed requires(N >= 3ul) in Seed.h and Seed.ipp and recompiled ACTS
> commented out parts in CxxUtils.h which delt with std::range when __cling__ is enabled. and recompiled Athena with a modified Package Filter.

Quoting:
> For information I think part of the issue might be that Cling doesn't like the requiere in the seed definition:
> The other issue seem to be that athena redefine the std::range::end function when Cling is used (in CxxUtils/range). Probably because it was not implemented before, but now this create a conflict with newer root version I guess ?  

See: https://atlas-art-data.web.cern.ch/atlas-art-data/grid-output/main/Athena/x86_64-el9-gcc13-opt/2024-10-08T2101/InDetPhysValMonitoring/test_run4_acts_vertex_PU200/tarball_logs/payload.stdout 

Co-authored-by: Pierfrancesco Butti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Component - Core Affects the Core module Event Data Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants