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 converter logic for o2-analysis-v0converter in test workflow #1493

Merged
merged 3 commits into from
Mar 2, 2024

Conversation

jezwilkinson
Copy link
Contributor

Since o2-analysis-v0converter produces the v0_002 table, it should be checking for that and not O2v0_001 to decide whether to run the converter

@benedikt-voelkel
Copy link
Contributor

fyi: Don't worry about the failed "Collect and print async labels" CI for now.

@benedikt-voelkel benedikt-voelkel merged commit c95a048 into AliceO2Group:master Mar 2, 2024
6 checks passed
@benedikt-voelkel
Copy link
Contributor

@jezwilkinson should this be contained in any of the production tags for async reconstruction/anchored MC? It would depend on whether the corresponding O2 developments for the v0_002 table are contained in those tags. Do you know?

Otherwise, would the worst-case scenario be that the v0_002 table would be simply overwritten the converter?

@jezwilkinson
Copy link
Contributor Author

Hi @benedikt-voelkel ,
I am not sure if the script is actively used with a configuration that would be affected in the production pipeline so can't really speak to that. But the worst-case is a crash of the analysis testing workflow (reason for spotting this issue was some tests ongoing in the TPC reconstruction group that were failing - if the v0 converter tries to run with a missing v0s_001, as would be triggered before the change, then it will crash with a "missing tree" error)

@benedikt-voelkel
Copy link
Contributor

I see, the converter relies on the presence to convert 001 to 002?
Sorry, I don't know exactly what is exactly converted from what to what.

The default version of v002 for V0s was set on the 13.12. by this commit: AliceO2Group/AliceO2@2fdcd65

I would assume that it Is from then onwards that your fix makes sense, right?
So I can see it is contained in O2@stable-async. I would mark this then with the corresponding label @chiarazampolli

@benedikt-voelkel benedikt-voelkel added async-2023-pbpb-apass async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 labels Mar 4, 2024
@jezwilkinson
Copy link
Contributor Author

I see, the converter relies on the presence to convert 001 to 002? Sorry, I don't know exactly what is exactly converted from what to what.

Exactly, the v0-converter consumes V0s_001 and produced V0s_002: https://github.com/AliceO2Group/O2Physics/blob/master/Common/TableProducer/v0converter.cxx and did not exist prior to the _002 version being introduced

The default version of v002 for V0s was set on the 13.12. by this commit: AliceO2Group/AliceO2@2fdcd65

I would assume that it Is from then onwards that your fix makes sense, right? So I can see it is contained in O2@stable-async. I would mark this then with the corresponding label @chiarazampolli

I would think so, but then the other change related to the "ft0corrected" table is probably more related to O2Physics versions than to AliceO2

@benedikt-voelkel benedikt-voelkel removed the async-2023-pbpb-apass4 Request porting to async-2023-pbpb-apass4 label Apr 24, 2024
@benedikt-voelkel
Copy link
Contributor

To get back to this @jezwilkinson
This is not yet included in the corresponding async-2023-pbpb-apass3 software. However, I am not aware of any issues due to the fact that it is not included. Could you comment?
If for any reason strictly necessary, let's include it. Otherwise, could we also think about removing the label and not include it?
Thanks a lot!

@jezwilkinson
Copy link
Contributor Author

as this was to fix a bug that @wiechula and @aschmah were encountering I leave it for them to comment whether it is still required

@wiechula
Copy link
Collaborator

wiechula commented Jun 4, 2024

It might be we only use it for the local testing. So I cannot really comment. If there we no issues reported up to now, I would guess not.

@benedikt-voelkel
Copy link
Contributor

Thanks @jezwilkinson and @wiechula for your feedback. So for now, I removed the label. As long as people use the daily builds (for testing), everything should be fine either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants