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

refactor: Remove Sequencer dataflow override #3625

Merged

Conversation

paulgessinger
Copy link
Member

This option does more harm than it does good, so I propose we remove it altogether.

This option does more harm than it does good, so I propose we remove it
altogether.
@paulgessinger paulgessinger added this to the next milestone Sep 18, 2024
@asalzburger asalzburger self-requested a review September 18, 2024 11:04
@asalzburger
Copy link
Contributor

This option does more harm than it does good, so I propose we remove it altogether.

You want to always check the dataflow then?

Both @benjaminhuth and I had problems with the check, i.e. we then need to get to the bottom of it why this could potentially fail.

@paulgessinger
Copy link
Member Author

paulgessinger commented Sep 18, 2024

@asalzburger Exactly. If you can reproduce it, it would be great to investigate why it fails.
You can also hack it out for a quick test, but I think having a flag in the main branch is dangerous.

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

I also believe the check is fine and it is a good protection mechanism against inconsistent data types on the whiteboard of our Examples Framework. If there are problems with that mechanism it is highly likely that it fails due to inconsistent compilation units which might be cause by build caches like ccache or dirty build folders. For more details see the discussion here https://mattermost.web.cern.ch/acts/pl/teonnaeyzjf1xqzgcz9nuyobzy

I would generally argue against these kinds of workarounds against protection mechanisms as this looks like we are hacking them and distrusting them on purpose. If people experience problems with it, it is most likely a symptom of a deeper issue and the actual issue should be treated instead of the symptom. No one will stop one from hacking it locally, this is simply one line change to "make it work" but I would urge to keep the main repo clean of it.

@benjaminhuth
Copy link
Member

So I agree that if the check fails, this is a sign that something in the build that is used is wrong, so removing the option alltogether is also fine...

But yeah, as @paulgessinger said, I see myself already hacking this in cases this happens and rebuild of the whole acts+dependencies is not an option immediatly...

@paulgessinger
Copy link
Member Author

paulgessinger commented Sep 18, 2024

@benjaminhuth If we understand why it happens we could try to identify which parts need to be rebuild. The mismatch that @asalzburger reported this morning, flat_map, is a header-only part of boost. That would indicate to me that it's not a dependency issue, but a build consistency issue, possibly ccache.

It's possible that the check is faulty, in which case it should be improved rather than just ignored. I can't reproduce this issue at this moment, so I can't debug it myself, which is why I'd love if you or @asalzburger could help debugging this.

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Sep 18, 2024
@benjaminhuth
Copy link
Member

benjaminhuth commented Sep 18, 2024

@asalzburger did you also used ccache when that occurred? The thing that we have in common I think that it also only happened to me when I use clang as compiler...

So when it happens the next time I will try again to dig into this, but I really think this is due to some build incompatibilities and not solvable from the repository side.

Copy link

github-actions bot commented Sep 18, 2024

📊: Physics performance monitoring for acdb2db

Full contents

physmon summary

@asalzburger
Copy link
Contributor

Let's merge this one then.

Copy link

@kodiakhq kodiakhq bot merged commit 9ee3a10 into acts-project:main Sep 24, 2024
42 checks passed
@paulgessinger paulgessinger deleted the refactor/remove-dataflow-override branch September 24, 2024 07:31
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Sep 24, 2024
@paulgessinger paulgessinger modified the milestones: next, v37.0.0 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants