Skip to content

DPL: add combine method to (optionally) run DataProcessors as a single#8548

Merged
ktf merged 1 commit intoAliceO2Group:devfrom
ktf:combine-data-processors
Apr 21, 2022
Merged

DPL: add combine method to (optionally) run DataProcessors as a single#8548
ktf merged 1 commit intoAliceO2Group:devfrom
ktf:combine-data-processors

Conversation

@ktf
Copy link
Member

@ktf ktf commented Apr 11, 2022

task

@ktf ktf requested a review from a team as a code owner April 11, 2022 12:33
@ktf ktf changed the title DPL: add combine method to (optionally) run DataProcessors as a single [WIP] DPL: add combine method to (optionally) run DataProcessors as a single Apr 11, 2022
* Use concat to join multiple WorkflowSpec.
* Use combine to (optionally) merge all the DataProcessors
  into a single one.
@ktf ktf force-pushed the combine-data-processors branch from b4d8923 to c89da59 Compare April 21, 2022 08:41
@ktf ktf changed the title [WIP] DPL: add combine method to (optionally) run DataProcessors as a single DPL: add combine method to (optionally) run DataProcessors as a single Apr 21, 2022
@ktf
Copy link
Member Author

ktf commented Apr 21, 2022

Ok, this works fine. One can just change the bool in the diamond workflow to verify.

@sawenzel I would suggest we adapt #8529 to use o2::framework::workflow::combine, once this is merged.

I also introduced a workflow::concat method which simplifies merging multiple workflows.

@ktf
Copy link
Member Author

ktf commented Apr 21, 2022

@sawenzel I have seen the o2Sim G4 tests hitting timeouts in multiple places today. Did anything relevant change in the last few days?

@ktf ktf merged commit 6000477 into AliceO2Group:dev Apr 21, 2022
@sawenzel
Copy link
Collaborator

I will close #8529. There is no need to adapt it but it would have been nice to mention in your commit the original idea.

@sawenzel
Copy link
Collaborator

Concerning o2sim: I am not aware of changes. Maybe it hangs during CCDB fetching.

@ktf ktf deleted the combine-data-processors branch April 21, 2022 17:38
@ktf
Copy link
Member Author

ktf commented Apr 21, 2022

@sawenzel Apologies (and proper kudos in #8630) for the missing acknowledgement. I would have, of course, given the due credit next Monday in the WP4 meeting and in the PDP report.

I would still find #8529 useful if it actually reduces the number of tasks in production. If everything is in a single executable, I think it might actually be trivial to automatically collapse parallel paths after the topological sort.

@ktf
Copy link
Member Author

ktf commented Apr 21, 2022

Re CCDB, maybe it would make sense to have a log warning if CCDBApi takes too long to fetch something.

@sawenzel
Copy link
Collaborator

One more thing comes to my mind. The implementation here will work nicely for specs that don't have overlapping option fields. However, I assume it will run into troubles when trying to combine processors that have common option keys. This is the case for RootTreeWriter for instance (they all have --filename etc). So likely some further development is needed.

(I have solved it in my local development with keeping track of provenance of options and reinserting them on the fly during init and by auto-adjusting these option fields with the device prefix.)

@ktf
Copy link
Member Author

ktf commented Apr 25, 2022

I thought about that, but what should be the behaviour? I guess we do not want to have different options for the combined case vs the non combined one, no? Shouldn't the options be adapted in the first place?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants