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.

Sign up for GitHub

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

Jump to bottom

FFTJet* modules still needed? #35578

Closed
Dr15Jones opened this issue Oct 7, 2021 · 29 comments
Closed

FFTJet* modules still needed? #35578

Dr15Jones opened this issue Oct 7, 2021 · 29 comments
Labels
fully-signed reconstruction-approved

Comments

@Dr15Jones
Copy link
Contributor

The FFTJetProducer, FFTJetPileupProcessor, FFTJetCorrectorDBReader, and FFTJetCorrectionProducer modules are thread unsafe, do not use esConsumes and still inherit from the legacy modules. A quick check on FFTJetProducer and FFTJetCorrectionProducer did not show up any instances of configurations using the modules.
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2021

A new Issue was created by @Dr15Jones Chris Jones.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

assign reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2021

New categories assigned: reconstruction

@slava77,@jpata you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Oct 7, 2021

@igv4321
please take a note of this issue and perhaps comment.
One option is to modernize the code, the other is to remove.

@Dr15Jones is this urgent for the core group?

@makortel
Copy link
Contributor

makortel commented Oct 7, 2021

is this urgent for the core group?

Since they appear to be unused, these are not urgent, but they would be good to be addressed in some way within the next months.

@Dr15Jones
Copy link
Contributor Author

@slava77 for perspective, the package and things using it are generating something like 400+ warnings in the CMSDEPRECATED IB, which is more than 10% of all warnings.

@igv4321
Copy link
Contributor

igv4321 commented Oct 7, 2021

I do plan to use these modules in the future, but I need a student... Would fixing the problems by the end of the year be a reasonable time scale?

@makortel
Copy link
Contributor

makortel commented Oct 7, 2021

By the end of the year would be fine, thanks!

Just note that in (early) 12_2_X code accessing EventSetup without ESGetToken will stop working.

@makortel
Copy link
Contributor

@igv4321 Would you have any news on these modules? The esConsumes() is now enforced at run time (since CMSSW_12_3_X_2021-12-13-1100).

@igv4321
Copy link
Contributor

igv4321 commented Dec 13, 2021 via email

@makortel
Copy link
Contributor

@igv4321 Would you happen to have any news on these modules? Thanks!

@igv4321
Copy link
Contributor

igv4321 commented Jan 24, 2022 via email

@makortel
Copy link
Contributor

makortel commented Mar 2, 2022

@igv4321 Any news?

@igv4321
Copy link
Contributor

igv4321 commented Mar 2, 2022 via email

@jpata
Copy link
Contributor

jpata commented May 17, 2022

kind ping @igv4321

@igv4321
Copy link
Contributor

igv4321 commented May 18, 2022

Some fixes are provided in the PR #37999. Sorry for slow response on this.

@jpata
Copy link
Contributor

jpata commented May 23, 2022 •
edited

+reconstruction

  • the FFTJet modules were migrated to esConsumes in Changing FFTJet modules to stream modules #37999, so they can now in principle run
    • EDIT: the modules were migrated from legacy to stream, as Matti pointed out
  • it was stated that there are plans to revive studies with them, even though they are currently not running in any workflow

@jpata
Copy link
Contributor

jpata commented May 23, 2022

@cmsbuild please close

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

@makortel
Copy link
Contributor

#37999 changed only some EDModules from legacy to stream, but did not migrate anything to esConsumes. In CMSSW_12_5_X_2022-05-20-2300, that includes #37999, the CMSDEPRECATED_X build still reports 417 warnings for JetMETCorrections/FFTJetModules and JetMETCorrections/FFTJetObjects. On a quick look majority of those warnings are for esConsumes, but there were some legacy EDModules as well.

I'd ask to reopen this issue.

@igv4321
Copy link
Contributor

igv4321 commented May 23, 2022

@makortel How do I turn these messages on? I don't see any warning messages with the usual "scram b" command.

@makortel
Copy link
Contributor

I just tried the following in 12_5_0_pre1

git cms-addpkg JetMETCorrections/FFTJetModules JetMETCorrections/FFTJetObjects
scram b

and saw the 417 warnings.

@igv4321
Copy link
Contributor

igv4321 commented May 23, 2022

Ah, sorry, I was looking at RecoJets/FFTJetProducers that were updated by #37999. The contents of JetMETCorrections are still to be fixed.

@perrotta perrotta reopened this May 23, 2022
@makortel
Copy link
Contributor

Did the bot close this because of the earlier "please close" message even if @perrotta reopened this issue? Do we need to make the bot to recognize reopening?

@smuzaffar
Copy link
Contributor

please reopen

@cmsbuild cmsbuild reopened this May 24, 2022
@smuzaffar
Copy link
Contributor

yes @makortel bot closed it due to #35578 (comment)

@makortel
Copy link
Contributor

Hi @igv4321, would you have any update on schedule to get FFTJetModules and FFTJetObjects migrated?

@igv4321
Copy link
Contributor

igv4321 commented Aug 1, 2022

@makortel This issue should be fixed by PR #38932. Sorry for the delay.

@makortel
Copy link
Contributor

makortel commented Aug 1, 2022

Thanks @igv4321!

@qliphy qliphy closed this as completed Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fully-signed reconstruction-approved
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@makortel @jpata @Dr15Jones @perrotta @smuzaffar @cmsbuild @slava77 @qliphy @igv4321