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

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

Merged
merged 1 commit into from
Apr 21, 2022

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