Skip to content

Skip invalid timeframes during ROOT input - #15576

Open
autumn-mck wants to merge 15 commits into
AliceO2Group:devfrom
autumn-mck:fault-tolerence
Open

Skip invalid timeframes during ROOT input#15576
autumn-mck wants to merge 15 commits into
AliceO2Group:devfrom
autumn-mck:fault-tolerence

Conversation

@autumn-mck

@autumn-mck autumn-mck commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Handle corrupt reads as recoverable, discarding the timeframe

@ktf

ktf commented Jul 6, 2026

Copy link
Copy Markdown
Member

thank you for your contribution. somehow i did not get notified about this. i will have a look at it tomorrow, time permitting. One comment i already have is that you can probably split the changes to LifetimeHolder as a separate PR.

ktf
ktf reviewed Jul 22, 2026
View reviewed changes
Comment thread Framework/Core/include/Framework/DataAllocator.h Outdated
Comment thread Framework/AnalysisSupport/src/TTreePlugin.cxx
@autumn-mck
autumn-mck marked this pull request as ready for review August 3, 2026 13:30
@autumn-mck
autumn-mck requested a review from a team as a code owner August 3, 2026 13:30
@ktf

ktf commented Aug 3, 2026

Copy link
Copy Markdown
Member

The part about the MessageContext can also be merged separately. Can you spawn a new PR?

@jgrosseo

jgrosseo commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@ktf Do you have other comments apart from the splitting?

@ktf

ktf commented Aug 4, 2026

Copy link
Copy Markdown
Member

I did not look into the details of the rest. I will do once I am back, tomorrow.

@ktf

ktf commented Aug 4, 2026

Copy link
Copy Markdown
Member

We should also think how to test this, e.g. making sure it actually does not get stuck or silently skips stuff.

@jgrosseo

jgrosseo commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

We should also think how to test this, e.g. making sure it actually does not get stuck or silently skips stuff.

Once we can enable it by environment variable, we plan to clone some trains which had large number of failures, enable the feature only for those, and do a detailed inspection of the logs and output.

@alibuild

alibuild commented Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Error while checking build/O2/fullCI_slc9 for 39c7515 at 2026-08-05 03:44:

## sw/BUILD/O2-latest/log
CMake Error in Detectors/Base/CMakeLists.txt:

Full log here.

ktf
ktf reviewed Aug 6, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
if (!treeRead) {
if (!first) {
LOGP(fatal, "Can not retrieve tree for table {}: fileCounter {}, timeFrame {}", concrete.origin.as<std::string>(), fcnt, ntf);
throw std::runtime_error("Processing is stopped!");

@ktf ktf Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this actually trigger? I suspect this is old dead code.

@autumn-mck autumn-mck Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My understanding is it can still trigger if a later requested table uses a separate DataInputDescriptor whose file has fewer timeframes than the first table’s file, but for the normal case this is effectively unreachable. Should this be removed?

Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 6, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 13, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/Core/src/CommonServices.cxx
MetricSpec{.name = "dropped_computations", .metricId = static_cast<short>(ProcessingStatsId::DROPPED_COMPUTATIONS), .kind = Kind::UInt64, .minPublishInterval = quickUpdateInterval},
MetricSpec{.name = "dropped_incoming_messages", .metricId = static_cast<short>(ProcessingStatsId::DROPPED_INCOMING_MESSAGES), .kind = Kind::UInt64, .minPublishInterval = quickUpdateInterval},
MetricSpec{.name = "relayed_messages", .metricId = static_cast<short>(ProcessingStatsId::RELAYED_MESSAGES), .kind = Kind::UInt64, .minPublishInterval = quickUpdateInterval},
MetricSpec{.name = "aod-invalid-read-skipped-timeframes",

@ktf ktf Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The extra metric can go in even without the rest of the code, if you want.

ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx Outdated
ktf
ktf reviewed Aug 17, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/DataInputDirector.cxx Outdated
@alibuild

alibuild commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Error while checking build/O2/fullCI_slc9 for 0b77c70 at 2026-08-17 17:56:

## sw/BUILD/O2Physics-latest/log
c++: fatal error: Killed signal terminated program cc1plus
ninja: build stopped: subcommand failed.

Full log here.

@alibuild alibuild mentioned this pull request Aug 18, 2026
Merged
@jgrosseo

jgrosseo commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

@ktf We just discussed and think it's best we do the parent file exception implementation later. In a first step we will test this feature only on demand on certain datasets on Hyperloop and we can start testing with non-parent file datasets.

@ktf

ktf commented Aug 21, 2026

Copy link
Copy Markdown
Member

Ok, the scoped lock was apparently a bad idea, since it does not work in the CI. Can you take it our for now?

Apart from that I still see the old code for the env variable. Can you resurrect the one you had at some point?

ktf
ktf reviewed Aug 21, 2026
View reviewed changes
Comment thread Framework/AnalysisSupport/src/AODJAlienReaderHelpers.cxx
@ktf

ktf commented Aug 24, 2026

Copy link
Copy Markdown
Member

Ok, now testing on hyperloop. Let's see.

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

Reviewers

@ktf ktf ktf left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@autumn-mck @ktf @jgrosseo @alibuild