Update clusterAnaScurve.py for the new environment

lmoureaux commented 6 years ago

Description

From the commit message:

Fix clusterAnaScurve.py to work with packages

Copy all environment variables and activate the virtualenv in the batch job.
Expected to work with Python 2.6 and 2.7, regardless of how the user's
virtualenv was setup.

No behavior change, therefore no need to update the documentation.

Depends on #103.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

This addresses the urgent part of #95. HTCondor and not using afs left for later.

How Has This Been Tested?

Used this virtualenv with Python 2.7
Started the script on all data for GEMINIm30L2, killed it after three jobs were submitted.
Checked that anaUltraScurve.py is being called
Checked that the summary plot of the one anaUltraScurve.py that didn't crash is produced (see below)
Launched an analysis of all S-curves to run overnight

Screenshots (if appropriate):

screenshot_20180531_231132

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed. --> there's a test bot, right?

bdorney commented 6 years ago

Used this virtualenv with Python 2.7

Can you try again using: https://gist.github.com/bdorney/5f3965bb8b26858a91ef8087137d1ec9

Example call:

source setup_gemdaq.sh -c 0.3.1 -g 1.0.0 -G 5 -v 2.0.0 -V 3 -p $PWD/venv

Checked that the summary plot of the one anaUltraScurve.py that didn't crash is produced (see below)

For those that did crash is the reason understood? e.g. is it related to the software or for example a bad input file.

Additionally your tests built the rpm after developing the festure?

bdorney commented 6 years ago

Additionally before this is approved I'd like the instructions in the README.md to be updated to reflect necessary changes (this can come as a later commit):

I think only the area under setup requires changes from what I've seen in this PR.

lmoureaux commented 6 years ago

Used this virtualenv with Python 2.7

Can you try again using: https://gist.github.com/bdorney/5f3965bb8b26858a91ef8087137d1ec9

Installed using:

python -m pip install -U rpm/dist/gempython_gemplotting-1.0.0.tar.gz

chmod +x'ed the scripts
Produced the map files
Launched one non-failing job again

=> Everything works as expected

Checked that the summary plot of the one anaUltraScurve.py that didn't crash is produced (see below)

For those that did crash is the reason understood? e.g. is it related to the software or for example a bad input file.

It complains about the S-curve tree not being present, and the input file size is <700 bytes. Bad input file.

Additionally your tests built the rpm after developing the festure?

Couldn't get the rpm to build in my afs area (even before my changes): make rpm fails but produces the pip package anyway. Not sure if it's because development on lxplus isn't supported.

The rpm was built in Travis. I don't know if it works and have no way to test this (ie no admin access to any slc6/cc7 machine).

Additionally before this is approved I'd like the instructions in the README.md to be updated to reflect necessary changes (this can come as a later commit): I think only the area under setup requires changes from what I've seen in this PR.

The updated clusteraAnaScurve would work in the old environment. It's independent of how one sets up its virtualenv, as long as there is one.

Updating the README would require a lot more testing (incl. at P5, where I've been unable to create a working env without a hack).

bdorney commented 6 years ago

Can you try again using: https://gist.github.com/bdorney/5f3965bb8b26858a91ef8087137d1ec9 Installed using: python -m pip install -U rpm/dist/gempython_gemplotting-1.0.0.tar.gz

The goal was to try to setup the env by executing the script in the above gist link using the calling syntax illustrated here. This is related to the comment about changing the README.md documentation. We want to give the user something foolproof that they can press enter, once. Rather than a string of commands that they may not understand or miss and cause an issue.

Can you please try again and report back on issues encountered? Showing the exact steps would be useful.

It complains about the S-curve tree not being present, and the input file size is <700 bytes. Bad input file.

Okay so a failed scan and not a SW issue.

Couldn't get the rpm to build in my afs area (even before my changes): make rpm fails but produces the pip package anyway. Not sure if it's because development on lxplus isn't supported.

Can you open a separate github issue to explain the problem? Can you also try on one of the gem904 machines? PM me on mattermost if you've forgotten their network aliases.

The updated clusteraAnaScurve would work in the old environment. It's independent of how one sets up its virtualenv, as long as there is one.

Right, updating the twiki to include the instructions on how to do this is what's being asked.

Updating the README would require a lot more testing (incl. at P5, where I've been unable to create a working env without a hack).

Indeed, this is what's being asked. Did you try from the cc7 vm which has native py2.7? PM me on mattermost if you need the network info.

lmoureaux commented 6 years ago

Can you please try again and report back on issues encountered? Showing the exact steps would be useful.

I had no issue with the setup script on lxplus:

Download the script: wget raw.githubusercontent.com/whereever-the-script-was
chmod +x the script
source setup_gemdaq.sh -c 0.3.1 -g 1.0.0 -G 5 -v 2.0.0 -V 3 -p GEM/venv from bash
python -m pip install -U rpm/dist/gempython_gemplotting-1.0.0.tar.gz from the root of my local checkout
chmod +x the gemplotting scripts (#102)
Produce the map files again
Run -- everything works

lmoureaux commented 6 years ago

The updated clusteraAnaScurve would work in the old environment. It's independent of how one sets up its virtualenv, as long as there is one.

Right, updating the twiki to include the instructions on how to do this is what's being asked.

Updating the README would require a lot more testing (incl. at P5, where I've been unable to create a working env without a hack).

Indeed, this is what's being asked.

In order to keep this discussion focused, I opened #109 about updating the README. I'll test again when the environment is set in stone.

lmoureaux commented 6 years ago

Force-pushed changes

Rebased on top of develop
Fix a bug introduced upstream

Testing update

Now tested with setup instructions from the new README on lxplus:

<follow README instructions>
make pip
pip install -U rpm/gempython_gemplotting-1.0.0.tar.gz
chmod +x venv/slc6/py2.7/lib/python2.7/site-packages/gempython/gemplotting/macros/*.py
clusterAnaScurve.py --anaType=scurve -i /afs/cern.ch/work/l/lmoureau/GEM/data/gemdata/GEMINIm30L2/scurve/listOfScanDates.txt

Three jobs submitted, all produced the expected output (2 fails because of bad data, 1 success with plot produced).

bdorney commented 6 years ago

@bdorney, anything else you are still waiting on? Will plan on putting this into a stable released version tonight

For the purposes of clusterAnaScurve.py I think this is suitable. The issues I raised here are being addressed by #109.

lmoureaux commented 6 years ago

@bdorney I force-pushed to update the last commit, will need your approval again

cms-gem-daq-project / gem-plotting-tools