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

JEventProcessorPODIO: treat all unhandled exceptions as fatal #1069

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Oct 11, 2023

We are now at a point where quality of the code in factories has been much improved. The exceptions can finally become failures to ensure that we catch the regressions early.

Resolves: #1052
Resolves: #625
Closes: #1053

@veprbl veprbl temporarily deployed to github-pages October 11, 2023 06:12 — with GitHub Actions Inactive
@veprbl veprbl temporarily deployed to github-pages October 11, 2023 17:39 — with GitHub Actions Inactive
@veprbl veprbl changed the title JEventProcessorPODIO: avoid looping over all collections twice JEventProcessorPODIO: treat all unhandled exceptions as fatal Oct 11, 2023
@veprbl veprbl force-pushed the pr/JEventProcessorPODIO_avoid_fixing_collections branch 3 times, most recently from 8691e61 to e88b22b Compare October 13, 2023 15:33
@wdconinc wdconinc force-pushed the pr/JEventProcessorPODIO_avoid_fixing_collections branch from e88b22b to 73d9ed3 Compare November 25, 2023 16:22
@wdconinc
Copy link
Contributor

Is this something we are closer to being able to merge?

@wdconinc
Copy link
Contributor

wdconinc commented Dec 6, 2023

What's holding this up now?

@wdconinc wdconinc force-pushed the pr/JEventProcessorPODIO_avoid_fixing_collections branch from 20c5767 to ab4a6c6 Compare January 8, 2024 00:54
@wdconinc
Copy link
Contributor

wdconinc commented Jan 8, 2024

@veprbl What's holding up this PR? It's a nice bit of simplification.

@veprbl
Copy link
Member Author

veprbl commented Jan 8, 2024

This doesn't work. We need to figure out how to disable factories without relying on exceptions for control flow.

@nathanwbrei
Copy link
Contributor

Looks like the problem is that in the reconstruction step, the EcalBarrelScFiClusters algorithm needs EcalBarrelScFiHits, but they haven't been put in the file in the digitization step.

@nathanwbrei
Copy link
Contributor

I figure we should change CalorimeterClusterRecoCoG so that the mchits input is std::optional. Right now the algorithm expects the collection to exist, and simply be empty.

@veprbl
Copy link
Member Author

veprbl commented Jan 13, 2024

Looks like the problem is that in the reconstruction step, the EcalBarrelScFiClusters algorithm needs EcalBarrelScFiHits, but they haven't been put in the file in the digitization step.

I'm confused about how this even works now.

@nathanwbrei
Copy link
Contributor

Have we been validating anywhere that this test produces meaningful output?

@nathanwbrei
Copy link
Contributor

More specifically, what happens downstream of:

    - name: Upload reconstruction output
      uses: actions/upload-artifact@v3
      with:
        name: rec_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.edm4eic.root
        path: rec_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.edm4eic.root
        if-no-files-found: error

@wdconinc
Copy link
Contributor

More specifically, what happens downstream of:

The only thing we do in CI is run a diff with the same file in the main branch. We don't validate performance or require certain performance parameters.

@veprbl
Copy link
Member Author

veprbl commented Jan 13, 2024

More specifically, what happens downstream of:

    - name: Upload reconstruction output
      uses: actions/upload-artifact@v3
      with:
        name: rec_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.edm4eic.root
        path: rec_${{ matrix.particle }}_1GeV_20GeV_${{ matrix.detector_config }}.edm4eic.root
        if-no-files-found: error

Interestingly enough, what happens next is this artifact gets overwritten by a result of eicrecon-gun job:

# gets first step
capybara capy rev HEAD~ --artifact-name raw_e_1GeV_20GeV_craterlake.edm4eic.root
# should get the second step
capybara capy rev HEAD~ --artifact-name rec_e_1GeV_20GeV_craterlake.edm4eic.root

@veprbl
Copy link
Member Author

veprbl commented Jan 13, 2024

And, yes, @nathanwbrei, you were right, the second step "works" by simply producing empty output collections:
image

@veprbl
Copy link
Member Author

veprbl commented Jan 13, 2024

I also discovered some older discussion in otherwise invalid PR #1071

@nathanwbrei
Copy link
Contributor

@veprbl points out that the current failure is probably the same as #1226

@veprbl veprbl marked this pull request as ready for review January 13, 2024 23:18
@veprbl veprbl force-pushed the pr/JEventProcessorPODIO_avoid_fixing_collections branch from fbf96a4 to 679dfb2 Compare April 13, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants