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

Allow for more than 3 skipped LHE files. #7795

Merged
merged 1 commit into from Jun 4, 2015

Conversation

cmsbuild
Copy link
Contributor

Currently, the LHESource reads by calling nextEvent(), which in turn
calls LHEReader::next(). The latter method returns an empty reference
at the end of a file 1.

When reading an event in a fourth file, nextEvent() is first called in
the constructor of LHESource 2, returning an empty reference at the
end of the fist file. The other calls to nextEvent(), 3 and 4,
return an empty reference at the end of the second and third file.
But there is a check after 4, and LHESource quits reading files
without attempting to open the fourth file.

Try to address the situation by keeping reading files as long as new
files are opened and empty events are returned. This potentially also
removes the need for the check after 4.

Also pertains to dmwm/CRABServer#4659.

Automatically ported from CMSSW_7_4_X #7642

Currently, the LHESource reads by calling nextEvent(), which in turn
calls LHEReader::next().  The latter method returns an empty reference
at the end of a file [1].

When reading an event in a fourth file, nextEvent() is first called in
the constructor of LHESource [2], returning an empty reference at the
end of the fist file.  The other calls to nextEvent(), [3] and [4],
return an empty reference at the end of the second and third file.
But there is a check after [4], and LHESource quits reading files
without attempting to open the fourth file.

Try to address the situation by keeping reading files as long as new
files are opened and empty events are returned.  This potentially also
removes the need for the check after [4].

Also pertains to dmwm/CRABServer#4659.

[1]: http://cmslxr.fnal.gov/lxr/source/GeneratorInterface/LHEInterface/src/LHEReader.cc#0491
[2]: http://cmslxr.fnal.gov/lxr/source/GeneratorInterface/LHEInterface/plugins/LHESource.cc#0042
[3]: http://cmslxr.fnal.gov/lxr/source/GeneratorInterface/LHEInterface/plugins/LHESource.cc?v=CMSSW_7_4_0_pre5#0166
[4]: http://cmslxr.fnal.gov/lxr/source/GeneratorInterface/LHEInterface/plugins/LHESource.cc?v=CMSSW_7_4_0_pre5#0169
@cmsbuild
Copy link
Contributor Author

A new Pull Request was created by @cmsbuild for CMSSW_7_5_X.

Allow for more than 3 skipped LHE files.

It involves the following packages:

GeneratorInterface/LHEInterface

@vciulli, @covarell, @thuer, @cmsbuild, @nclopezo, @bendavid can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@nclopezo
Copy link
Contributor

+1
Tests already approved in the equivalent of this PR in CMSSW_7_4_X

@davidlange6
Copy link
Contributor

I assume this is obsolete. Will close

@matz-e
Copy link
Contributor

matz-e commented Mar 20, 2015

How so? I've asked for some more feedback via email a while ago, but not received any. We have added a warning to CRAB when processing more than one LHE file, but merging several LHE files does not seem trivial to do (from a user's perspective.)

Having this fixed one way or another would be nice.

@bendavid
Copy link
Contributor

Indeed this is not obsolete, but we would really like framework experts to at least have a quick look at the code.

@davidlange6
Copy link
Contributor

Seems everyone is too patient if waiting since a month for comments from @wmtan. Maybe @Dr15Jones is better to contact here.

@Dr15Jones
Copy link
Contributor

I don't see any obvious problems. Of course, I'm assuming someone actually tested that the change works.

@Dr15Jones
Copy link
Contributor

What happens if the last file in the list doesn't have anything? Does it properly stop?

@matz-e
Copy link
Contributor

matz-e commented Mar 25, 2015

I've set skippedEvents to an unreasonably large value and deleted every <event>…</event> in the last file of the test case I have. In this case, all files were opened and closed and cmsRun terminated properly. If I completely remove the file contents, it crashes with an XML parser exception.

Does that answer your question?

@Dr15Jones
Copy link
Contributor

I guess it answers the question. Seems lik the LHE file format has no way of specifying a file containing no events.

@matz-e
Copy link
Contributor

matz-e commented Apr 7, 2015

This, #7795, was the actual pull request (ported from 7_4_X). While @Dr15Jones has looked at this, I'm still not sure what the status w.r.t. merging is?

@bendavid
Copy link
Contributor

bendavid commented Jun 4, 2015

+1
(and indeed this should be reopened)

Turns out that it's also relevant for pLHE production, which is obvious in retrospect.

@davidlange6 davidlange6 reopened this Jun 4, 2015
davidlange6 added a commit that referenced this pull request Jun 4, 2015
Allow for more than 3 skipped LHE files.
@davidlange6 davidlange6 merged commit 9381dfe into cms-sw:CMSSW_7_5_X Jun 4, 2015
@matz-e matz-e deleted the LHESource_fix_many_files branch September 15, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants