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

Sort module summary reports by module label #13225

Merged
merged 1 commit into from Feb 11, 2016

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented Feb 8, 2016

The modules in the trigger summary report and timing summary report are output in an order that is meaningless to the user. This PR orders the module summary reports by the module label.
This PR should solve issue #13195, as revised to sort by module label, rather than by time spent.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2016

A new Pull Request was created by @wmtan for CMSSW_8_0_X.

It involves the following packages:

FWCore/Framework

@cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @wddgit this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11080/console

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 8, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@Martin-Grunewald
Copy link
Contributor

Hi,

I disagree with such re-ordering, especially in the NON-multi-threading case where the order is preserved. It allows us to understand better the trigger sequences as defined, and when making
a diff. Please do not reorder!
On second thought, a diff between nonMT and MT would then still be useless,
so a common ordering principle which is reproducible(!) would be best.
In the past it was the ordering of how the modules were lined up in the SCHEDULE.
If this is now gone?

@Dr15Jones
Copy link
Contributor

The reports based on Path and EndPath were not changed, they still are in the path order. What was changed was only the 'Module Summary' list. The previous ordering for that list was
-each path (in order) prints the results for the module in path order
-if a module appears on multiple paths, its results are only printed the first path (so the results are not in dependency order)
-unscheduled modules are printed last in a random order (I think the order is the python hash value of the module label).

Such an ordering makes it difficult to find the time results for a given module (remember, time results for a Path are printed seperately and remain in Path order).

@Dr15Jones
Copy link
Contributor

@smuzaffar The tests say they aborted but the bot doesn't seem to recognize that fact.

@Dr15Jones
Copy link
Contributor

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2016

Pull request has been put on hold by @Dr15Jones
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@Martin-Grunewald
Copy link
Contributor

@Dr15Jones

OK, thanks for the clarifications!

@Dr15Jones
Copy link
Contributor

@Martin-Grunewald Are you OK with this change then? If so, I will remove the hold.

@Martin-Grunewald
Copy link
Contributor

@Dr15Jones
Yes, it is fine, as alphabetically is reproducible and fixed
(as opposed to timing after tuning etc.).
So from my side you can remove the "hold"

@Dr15Jones
Copy link
Contributor

unhold

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2016

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

@smuzaffar
Copy link
Contributor

@Dr15Jones , in this case jenkins aborted the tests, so the script was not able to update the github PR.
For now, you can remove your "please test" comment and add a new "please test" comment to restart the tests.
We will check if we can workout this issue within jenkins e.g. by creating a stamp file to do some post processing

@Dr15Jones
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11092/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 9, 2016

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Feb 11, 2016
Sort module summary reports by module label
@cmsbuild cmsbuild merged commit d0a1a70 into cms-sw:CMSSW_8_0_X Feb 11, 2016
@wmtan wmtan deleted the SortModuleSummaryReports branch February 16, 2016 17:38
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

6 participants