Skip to content
Snippets Groups Projects

Need to pass DETECTOR variable to downstream benchmarks

    • Closed Issue created by Whitney Armstrong @whit

    In order to keep benchmarks generic, a DETECTOR variable must be set as a default is no longer assumed in common_bench https://eicweb.phy.anl.gov/EIC/benchmarks/physics_benchmarks/-/jobs/1991098

    Designs

      Child items 0

      2. No child items are currently assigned. Use child items to break down this issue into smaller parts.

      Activity

      • Whitney Armstrong assigned to @wdconinc

        assigned to @wdconinc

      • Wouter Deconinck
        Wouter Deconinck @wdconinc ·
        Maintainer

        Please don't break workflows with changes in common_bench. We (the EIC project detector) are an active project on a tight schedule. That timeline includes tagged and benchmarked geometry, reconstruction and container releases on the first Monday of the month. It is frustrating to see workflow failures introduced a day before in service of the 2nd detector, and then be told to just fix it. Please revert the changes in common_bench, first file an issue giving us notice, and then make breaking changes only after this has been addressed. I can address this later in the week but not on a Sunday after stuff is already broken.

      • Wouter Deconinck mentioned in merge request !730 (merged)

        mentioned in merge request !730 (merged)

      • Whitney Armstrong
        Whitney Armstrong @whit ·
        Author Owner

        I am unbreaking workflows. There are no issues other than triggering from this repo. Unless I am missing something?

      • Wouter Deconinck
        Wouter Deconinck @wdconinc ·
        Maintainer

        As an intermediate policy until we can get something else worked out, please don't make changes without review from someone in ePIC. It can be @cpeng, @veprbl, @sly2j, etc. If workflows are already breaking in week 1 of det2 efforts, then we're doing something wrong and it's going to lead to (more) frustration. Hyrum's law applies whether it's intentional or not...

        One of the main things we've been trying to instill in developers in ePIC is that code needs to be reviewed and needs to pass all checks before it gets merged, including for changes from all of us in development leadership.

      • Whitney Armstrong
        Whitney Armstrong @whit ·
        Author Owner

        I agree this is a good idea. This isn't week 1 of det2 efforts. There is no detector 2. All I did was copy athena which shouldn't be broken.

        To the point of this issue was to make you aware of this (trivial) fix. Again what else is broken? Please create an issue.

      • Wouter Deconinck
        Wouter Deconinck @wdconinc ·
        Maintainer

        As far as I know, there are no other issues. I'll post issues for any problems I encounter (or, more likely, submit merge requests with fixes, if past experience is a guide).

        Don't get me wrong: I do appreciate all the design that will make this able to support both ePIC and detector 2 (or even just support ePIC better). Nevertheless, there are ways in which we, inevitably and probably unknowingly, have started to depend on undocumented interfaces (or edge case behaviors of documented interfaces). It makes sense to make those instances explicit, of course, but in the meantime it is also easy to break things; even today it took some time for us to figure out why we suddenly were getting git clone errors. That's where the reviews/involvement will help with getting at least more of us in the loop on what may be happening.

      • Wouter Deconinck closed with merge request !730 (merged)

        closed with merge request !730 (merged)

      Please register or sign in to reply