Update clusterAnaScurve.py for the new environment #105

lmoureaux · 2018-05-31T21:19:34Z

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

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):

Checklist:

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 · 2018-05-31T21:30:48Z

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

Some clarification and follow-up testing requested. Overall good starting point.

bdorney · 2018-05-31T21:48:43Z

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 · 2018-06-01T09:15:28Z

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 · 2018-06-01T11:23:04Z

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 · 2018-06-01T11:52:03Z

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 (Bug Report: macros lack executable permissions post release install #102)
Produce the map files again
Run -- everything works

lmoureaux · 2018-06-04T17:26:31Z

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.

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.

lmoureaux · 2018-06-06T11:10:15Z

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).

jsturdy

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

jsturdy · 2018-06-06T15:13:14Z

anautilities.py

@@ -66,7 +66,7 @@ def getDirByAnaType(anaType, cName, ztrim=4):
        pass

    # Check Paths
-    from ..utils.wrappers import envCheck
+    from gempython.utils.wrappers import envCheck


Simultaneous changes, but the others were merged in earlier, can you revert yours, or make them conform to the current develop? (also L128, L335)

If you're talking about commit 2ac2945, read again: this reverts your change (which crashed the program). This PR is based on today's develop.

Context: anautilities.py is installed in gempython/gemplotting/utils, while wrappers.py is in gempython/utils

ah, ok, then the relative import should be ...utils.wrappers, and if that doesn't work, we'll skip the relative imports for now

For my own education: N leading dots are translated into N-1 ../ relative to the current file ?

For reference

bdorney · 2018-06-06T15:20:55Z

@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.

They were broken in commit 2ac2945

lmoureaux · 2018-06-06T16:21:51Z

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

bdorney

Re-approved after reviewing updated commit. Looks good still.

bdorney requested review from bdorney, jsturdy and mexanick May 31, 2018 21:30

bdorney assigned lmoureaux May 31, 2018

bdorney added Type: Maintenance Status: Review Needed Priority: High Type: Enhancement labels May 31, 2018

bdorney suggested changes May 31, 2018

View reviewed changes

This was referenced Jun 1, 2018

Bug Report: make rpm fails to build the package on lxplus #107

Closed

Update setup instructions #109

Merged

Fix clusterAnaScurve.py to work with packages

68f4703

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.

lmoureaux force-pushed the fix_clusterAnaScurve branch from a2464de to 99adad7 Compare June 6, 2018 11:05

jsturdy suggested changes Jun 6, 2018

View reviewed changes

bdorney previously approved these changes Jun 6, 2018

View reviewed changes

Repair imports in anautilities.py

45b6c23

They were broken in commit 2ac2945

lmoureaux dismissed bdorney’s stale review via 45b6c23 June 6, 2018 16:17

lmoureaux force-pushed the fix_clusterAnaScurve branch from 99adad7 to 45b6c23 Compare June 6, 2018 16:17

bdorney approved these changes Jun 6, 2018

View reviewed changes

jsturdy approved these changes Jun 6, 2018

View reviewed changes

jsturdy merged commit 32c3e2f into cms-gem-daq-project:develop Jun 6, 2018

lmoureaux deleted the fix_clusterAnaScurve branch June 7, 2018 09:13

bdorney mentioned this pull request Jun 28, 2018

Update clusterAnaScurve.py for new computing environment. #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update clusterAnaScurve.py for the new environment #105

Update clusterAnaScurve.py for the new environment #105

lmoureaux commented May 31, 2018

bdorney commented May 31, 2018 •

edited

Loading

bdorney left a comment

bdorney commented May 31, 2018

lmoureaux commented Jun 1, 2018

bdorney commented Jun 1, 2018

lmoureaux commented Jun 1, 2018

lmoureaux commented Jun 4, 2018

lmoureaux commented Jun 6, 2018 •

edited

Loading

jsturdy left a comment •

edited

Loading

jsturdy Jun 6, 2018

lmoureaux Jun 6, 2018

jsturdy Jun 6, 2018 •

edited

Loading

lmoureaux Jun 6, 2018

jsturdy Jun 6, 2018

bdorney commented Jun 6, 2018

lmoureaux commented Jun 6, 2018 •

edited

Loading

bdorney left a comment

Update clusterAnaScurve.py for the new environment #105

Update clusterAnaScurve.py for the new environment #105

Conversation

lmoureaux commented May 31, 2018

Description

Types of changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

bdorney commented May 31, 2018 • edited Loading

bdorney left a comment

Choose a reason for hiding this comment

bdorney commented May 31, 2018

lmoureaux commented Jun 1, 2018

bdorney commented Jun 1, 2018

lmoureaux commented Jun 1, 2018

lmoureaux commented Jun 4, 2018

lmoureaux commented Jun 6, 2018 • edited Loading

Force-pushed changes

Testing update

jsturdy left a comment • edited Loading

Choose a reason for hiding this comment

jsturdy Jun 6, 2018

Choose a reason for hiding this comment

lmoureaux Jun 6, 2018

Choose a reason for hiding this comment

jsturdy Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

lmoureaux Jun 6, 2018

Choose a reason for hiding this comment

jsturdy Jun 6, 2018

Choose a reason for hiding this comment

bdorney commented Jun 6, 2018

lmoureaux commented Jun 6, 2018 • edited Loading

bdorney left a comment

Choose a reason for hiding this comment

bdorney commented May 31, 2018 •

edited

Loading

lmoureaux commented Jun 6, 2018 •

edited

Loading

jsturdy left a comment •

edited

Loading

jsturdy Jun 6, 2018 •

edited

Loading

lmoureaux commented Jun 6, 2018 •

edited

Loading