star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Fix EMC data in picoDst data in DAQ production #295

Closed starsdong closed 2 years ago

starsdong commented 2 years ago
starsdong commented 2 years ago

Updated following Gene's suggestion

genevb commented 2 years ago

The relocation of setup specifics to StTriggerSimuMaker looks good to me.

One more thing: I expect that the "picoWrite" option in BigFulLChain.h should now include "trgSimu" among the options it turns on (fourth argument becomes "trgSimu,picoDst"), unless the default should be that we wouldn't want trgSimu (but I get the impression that we do). This in particular ensures that the StTriggerSimuMaker runs before the StPicoDstMaker in the chain.

Thanks, -Gene

starsdong commented 2 years ago

Thanks for the suggestion, Gene. The picoWrite option is updated.

starsdong commented 2 years ago

Per today's discussion, we would like to include this PR for the upcoming SL22a lib release. Could you please review the latest commit, comment or approve it soon? Thanks

starsdong commented 2 years ago

Jason, I agree with your concerns and am also in strong favor that we have long term maintainer and maybe have it reviewed too. One question, maybe to Gene. The TriggerSimuMaker is included in genDst.C that is used for our MuDst->picoDst production. So presumably is it already included in our production lib build?

genevb commented 2 years ago

Hi,

One question, maybe to Gene. The TriggerSimuMaker is included in genDst.C that is used for our MuDst->picoDst production. So presumably is it already included in our production lib build?

1) StTriggerSimuMaker was in genDst.C from its original entry into CVS by @nigmatkulov (I never altered that part): https://www.star.bnl.gov/cgi-bin/protected/viewvc.cgi/cvsroot/StRoot/StPicoDstMaker/macros/genDst.C?hideattic=0&revision=1.1&view=markup There was a software peer review in 2007, but noted as unofficial because it wasn't used in production at the time. Another review was done in 2010 as it began to be used in official simulation/embedding productions, and it moved forward with a flag about database maintenance: https://drupal.star.bnl.gov/STAR/comp/sofi/soft-n-libs/peer-review

2) We skip Pool directories, but not Utils/Utilities directories in compilation.

-Gene

starsdong commented 2 years ago

Gene, thank for pointing out this history. It seems the review was concluded and the code moved forward with some concern on the DB access. I am not sure if we want to reopen this and address the concern especially at this moment considering we are all tied with many things. I would suggest we move forward and if this causes an issue with the production, we should flag as priority and address it asap.

zlchang commented 2 years ago

Hi Xin,

I believe the concern was the older version was using MySQL query to access online database server. I have fixed that seven years ago to query the trigger information from offline DB. That’s why Zhenyu is now trying to populate the offline database. Please let me know if any further review is required.

Zilong

On Feb 15, 2022, at 5:31 PM, Xin Dong @.***> wrote:



Gene, thank for pointing out this history. It seems the review was concluded and the code moved forward with some concern on the DB access. I am not sure if we want to reopen this and address the concern especially at this moment considering we are all tied with many things. I would suggest we move forward and if this causes an issue with the production, we should flag as priority and address it asap.

— Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/295#issuecomment-1040863384, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3IWWAU342YG6KR75B3U3LH3PANCNFSM5NTNSPJA. You are receiving this because your review was requested.Message ID: @.***>

starsdong commented 2 years ago

Thanks Zilong for the comment. It looks to be that there is no major concern regarding the StTriggerSimuMaker (besides its location under StTriggerUtilities as Jason mentioned). I would then suggest we go ahead and merge the request in and prepare for SL22a library. Dmitri, could you help help merge this one?

genevb commented 2 years ago

Merging this PR immediately broke FastOffline jobs in DEV after the nightly AutoBuild ran. I backed out a couple of the changes to get it running again. Someone needs to debug this.

Example job:

root4star -b -q -l 'bfc.C(4,"pp2022,StiCA,BEmcChkStat,epdhit,-hitfilt,-evout,-fcsCluster,-fcsPoint","/gpfs01/star/daq/2022/032/23032031/st_physics_adc_23032031_raw_6500079.daq")'

What I'm seeing at the crash:

StTriggerSimuMaker:INFO  - runNumber=23032031 with DB timestamp 2022-02-01 16:59:15
root4star: .sl73_gcc485/OBJ/StRoot/StTriggerUtilities/Eemc/StEemcTriggerSimu.cxx:737: void StEemcTriggerSimu::getEemcFeeMask(): Assertion `mDbE' failed.

-Gene

veprbl commented 2 years ago

The same crash is now seen in CI: https://github.com/star-bnl/star-sw/runs/5221232278?check_suite_focus=true Could be an interplay between two PRs that were merged together?

genevb commented 2 years ago

The same crash is now seen in CI: https://github.com/star-bnl/star-sw/runs/5221232278?check_suite_focus=true Could be an interplay between two PRs that were merged together?

Which two? I thought this was the only one for Feb. 16, other than a CODEOWNERS one.

-Gene

veprbl commented 2 years ago

The same crash is now seen in CI: https://github.com/star-bnl/star-sw/runs/5221232278?check_suite_focus=true Could be an interplay between two PRs that were merged together?

Which two? I thought this was the only one for Feb. 16, other than a CODEOWNERS one.

It's just a guess. The CI for this PR was run 9 days ago, so it couldn't have taken into account whatever was merged in that time.

I'm trying reverting this in my fork https://github.com/veprbl/star-sw/pull/15

genevb commented 2 years ago

I'm trying reverting this in my fork veprbl#15

I reverted the changes to StPicoDstMaker.cxx and BigFullChain.h to get DEV working a few hours ago (after making sure the jobs ran with some local tests). Seems to be fine without those. I suspect it's more to do with support for recent data (e.g. 2022) with StTriggerSimuMaker than a conflicting code commit. A maintenance issue.