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

Removed PolyFit3DParametrizedMagneticField from Threaded branch #3200

Conversation

Dr15Jones
Copy link
Contributor

The PolyFit3DParametrizedMagneticField is not presently thread safe. To avoid
problems with the static analyzer we are removing it from the threaded
branch. If the code is made thread safe, we will add it back.

The PolyFit3DParametrizedMagneticField is not presently thread safe. To avoid
problems with the static analyzer we are removing it from the threaded
branch. If the code is made thread safe, we will add it back.
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2014

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_1_THREADED_X.

Removed PolyFit3DParametrizedMagneticField from Threaded branch

It involves the following packages:

MagneticField/ParametrizedEngine

@nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks.
@namapane this is something you requested to watch as well.
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.

@@ -1,40 +0,0 @@
#ifndef PolyFit3DParametrizedMagneticField_h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may suggest to put an error directive with some info in case someone tries to use this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was in the plugins directory and therefore it was impossible to link to the implementation of this class. So even if someone tried to include that header their code would have failed at link time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true, nothing to do here then

@slava77
Copy link
Contributor

slava77 commented Apr 4, 2014

is this going to go to the main 71X branch, eventually?

@Dr15Jones
Copy link
Contributor Author

When we switch 7_1_THREADED_X to be 7_1_X it becomes the default. If the thread safety issues are fixed before that then it will be available.

@slava77
Copy link
Contributor

slava77 commented Apr 4, 2014

(these questions are somewhat beyond the scope of this PR)
what is the meaning of "becomes"?
Do we have regular tests confirming there are no regressions between 71X and the threaded?

Curiously, in https://github.com/cms-sw/cmssw/compare/CMSSW_7_1_X...CMSSW_7_1_THREADED_X?w=1
I see a new .root file.

(more specific to this PR)
I suppose we will need to check and sign off on the changes.
@ktf Giulio,
will there be a regular jenkins report here?

@Dr15Jones
Copy link
Contributor Author

@nclopezo ping?

@nclopezo
Copy link
Contributor

nclopezo commented Apr 8, 2014

Hi,
When I run the relvals to generate the baseline files I always get errors. For example here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-baseline-tests/CMSSW_7_1_THREADED_X_2014-04-07-0200/slc6_amd64_gcc481/matrix-results/

In the workflows
1001.0
1000.0
1003.0
1306.0
25.0

So the .root files are not generated, and the comparison cannot be run. Do I have to run them in a different way in this case?

@Dr15Jones
Copy link
Contributor Author

It is necessary to add a special customize function to the job or make a any RelVals run. @ktf could you show David?

@ktf
Copy link
Contributor

ktf commented Apr 8, 2014

I will. DavidM pass by tomorrow.

On 8 Apr 2014, at 14:10, Chris Jones wrote:

It is necessary to add a special customize function to the job or make
a any RelVals run. @ktf could you show David?


Reply to this email directly or view it on GitHub:
#3200 (comment)

@Dr15Jones Dr15Jones mentioned this pull request Apr 8, 2014
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2014

-1
When I ran the RelVals I found an error in the following worklfows:
25.0 step3

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT/step3_TTbar+TTbar+DIGI+RECO+HARVEST+ALCATT.log

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3200/843/summary.html

@VinInn
Copy link
Contributor

VinInn commented Apr 9, 2014

is this multi-thread?
how can we have multiple segfaults?

@Dr15Jones
Copy link
Contributor Author

@nclopezo @ktf The seg faults are because an existing DQM module is marked thread-safe but it is not. You'll see similar crashes in the IB RelVals. [The problem has been reported to them]. Given that the crash is unrelated to this code change please go ahead and merge it.

@Dr15Jones
Copy link
Contributor Author

@VinInn Vincenzo, yes this particular test was using 4 threads. You can actually see that in the log message at the beginning of the log file.

nclopezo added a commit that referenced this pull request Apr 9, 2014
…edEngineFromThreaded

MagneticField/ParametrizedEngine -- Removed PolyFit3DParametrizedMagneticField from Threaded branch
@nclopezo nclopezo merged commit 1cb3abe into cms-sw:CMSSW_7_1_THREADED_X Apr 9, 2014
@Dr15Jones Dr15Jones deleted the removeMagneticFieldParametrizedEngineFromThreaded branch April 14, 2014 19:53
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