[RFC] Integrate gem-plotting-tools with Travis CI #27

lmoureaux · 2017-08-24T15:10:58Z

Travis CI is an online platform for continuous integration, free-to-use for open source projects. It is deeply integrated with Github and can block merge just like landscape can. This PR adds support for Travis CI for gem-plotting-tools.

Description

Travis is very generic and can be told to run arbitrary commands. The "build" fails if any of the commands fails, which is the ideal behavior for unit tests. Travis uses Ubuntu 14.04 as their default environment, and I don't change this.

This PR adds a .travis.yml file to enable Travis. This file and the scripts in .travis setup the following testing environment:

A Linux machine based on Ubuntu 14.04 (Travis' default; some cosmetic changes were needed for the code to run there)
ROOT packages from the ROOT website [edited: was Ubuntu's ROOT packages]
Python 2.6 and python 2.7 (tests will be run in both environments)
gem-plotting-tools' dependencies (as per requirements.txt)
cmsgemos from the default branch [edited: was master]
cmsgemos' dependencies (as per requirements.txt) [edited: new]

In addition, this PR adds a test of ana_scans.py --runType=scurve. The test will catch syntax errors and fatal exceptions in ana_scans.py and anaUltraScurve.py, as well as any file they import. The test will not catch other problems. Some changes to ana_scans.py were required in order to make it fail when the subcommands fail.

If this PR gets merged, an administrator can subscribe to Travis and enable it within minutes.

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

See #26 for context. Having Travis run every tool for every commit would catch code typos early.

How Has This Been Tested?

The changes were continuously tested on Travis itself (and only there). See here for an example of the test running without error, here for the output of Travis catching bug #24.

Screenshots

This is the current Travis result for my branch travis...

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.

This is required for the scripts to run on Debian-based machines (Travis CI uses Ubuntu). /usr/bin/env is also valid on cosmicstandtif.

This will fix bug 24 and should turn Travis green again

lmoureaux · 2017-08-24T15:12:38Z

This PR isn't ready yet, but I'd like to get comments on the architecture before more tests get added to it.

bdorney · 2017-08-25T09:49:07Z

Ubuntu's ROOT packages

Can you confirm this is:

ROOT 5.34/36 (heads/v5-34-00-patches@v5-34-34-76-g57bae4c, Apr 08 2016, 12:05:00 on linuxx8664gcc)

Python 2.6 and python 2.7 (tests will be run in both environments):

Can you confirm these are:

Python 2.6.6
Python 2.7.5

As these are the defaults on slc6 and cc7 operating systems.

cmsgemos from branch master

Please use generic-amc-release-v3 of cmsgemos instead, found here.

One question. How does Travis CI update? e.g. if there is an update to cmsgemos what does one do to ensure that this update is included?

Also I think we need to expand this unit testing:

In addition, this PR adds a test of ana_scans.py --runType=scurve. The test will catch syntax errors and fatal exceptions in ana_scans.py and anaUltraScurve.py, as well as any file they import. The test will not catch other problems. Some changes to ana_scans.py were required in order to make it fail when the subcommands fail.

Let's expand this to include:

anaUltraThreshold.py
anaUltraLatency.py
anaXDAQLatency.py

Also can the input files that we use as the "test data" be stored in the same file structure that ana_scans.py expects. This could also help us test chamberInfo.py.

bdorney · 2017-08-25T09:54:00Z

.travis.yml

+before_script:
+ - .travis/install_cmsgemos.sh
+ # Copy Travis chamber info
+ - cp .travis/chamberInfo.py mapping/chamberInfo.py


Looking at the file below this will work for testing. But as a counter question how do you intend to use this to test potential changes to chamberInfo.py itself?

For example what happens if someone adds a dictionary to this file or changes the default behavior. Seems like we'd then have to change .travis/chamberInfo.py as well to test this.

I think we should rely on using the default mapping/chamberInfo.py or at least one of the other links (e.g. chamberInfo_904.py or chamberInfo_qc8.py with a preference for the first).

Your PR for chamberInfo.py isn't merged yet, so mine isn't built on top of it :-) The current default is for P5, meaning 12x more data in the repo, which I don't want. My plan is to move this file to mapping/chamberInfo_Travis.py when merging your changes.

Also, the default for 904 includes detector names, which could change at some point. I don't want the build to break just because we switched detectors, which is why I used a custom version. We can in principle mix data from different detectors in the repo (as long as they don't have different vfatmasks, which currently limits the number of available options).

IMHO updates to the data files should be limited to file format changes.

I see your points. Let's not build off unnerved PR's your right. And let's not rely on potentially transient setups.

However the detectors in P5 will not changed (even post LS2 there will be a GEMINIm27L1). And it is for the production level machines that we should be concerned with. I think ensuring the integrity tests are valid for the real system is the primary goal so let's at least have the P5 system and the /current directory for each. This would be the most representative case.

Using P5 data would also mean that PRs that depend on file format changes would have to be tested on P5 data (or fake P5 data 👎, or we'd have to merge with tests failing 👎). Generating this data would slow down development a lot (it could even deadlock in case of circular dependencies).

bdorney · 2017-08-25T09:54:53Z

.travis.yml

+ - source .travis/setupenv.sh
+
+script:
+ - ana_scans.py --scandate=current --anaType=scurve


As mentioned we should expand to test the full range of scripts. This here only tests anaUltraScurve.py and one piece of ana_scans.py.

bdorney · 2017-08-25T09:56:19Z

.travis/data/travis/.gitignore

+# Don't ignore data files
+!scurve
+!scurve/current
+!scurve/current/SCurveData.root


Why is a duplicate .gitignore file needed at this location: .travis/data/travis/?

I didn't want to add many long paths to the main .gitignore. I chose to move all data-related ignore directives into another file.

I think this will be harder to maintain in the long run...now developers have to check 2 files instead of one. And a new developer may not even know that a second .gitignore exists. Let's revert this and use only one file.

bdorney · 2017-08-25T09:57:59Z

.travis/install_cmsgemos.sh

+# Travis script to install cmsgemos
+
+cd $TRAVIS_BUILD_DIR/..
+git clone --depth 1 https://github.com/cms-gem-daq-project/cmsgemos.git


I see...so everytime someone makes a PR this is going to clone cmsgemos. So bugs in cmsgemos might cause a unit test to this repo to fail? One needs to then keep this in mind when interpreting results.

Right. We could specify a specific revision (or tag?), but would have to update it manually.

No let's keep it this way and rely the HEAD of cmsgemos being correct

bdorney · 2017-08-25T09:59:08Z

.travis/setupenv.sh

+# Script to setup data paths
+
+# For Ubuntu 14.04: add pyroot to PYTHON_PATH
+export PYTHONPATH="/usr/share/python-support/root:/usr/lib/python2.7/dist-packages/"


Let's test on python 2.6.6 for now.

Also what will $ROOTSYS be? I think this might be important no?

There is no distribution of pyroot built against Python 2.6 in Debian, but it seems binary compatible.

Again check @jsturdy's implementation to see if this sort of issue is resolved

bdorney · 2017-08-25T10:03:39Z

anaUltraLatency.py

@@ -1,4 +1,4 @@
-#!/bin/env python
+#! /usr/bin/env python


What is the rationale for this change (and in subsequent files: anaUltraScurve.py, anaUltraThreshold.py, anaXDAQLatency.py, ana_scans.py, and anautilities.py)?

Are you sure this change, while possibly motivated for making Travis CI work, will not impact performance on production machines? Specifically the P5 machines?

Debian/Ubuntu don't have /bin/env in any supported version. Performance won't be affected at all, env is just a tool that searches $PATH and standard locations for an executable and launches it. The only possible problem would be SL to remove /usr/bin/env in a future version, but IIRC there is some standard for Linux distros that says /usr/bin should be the default for executables.

bdorney · 2017-08-25T10:05:34Z

ana_scans.py

+ returncode = runCommand(cmd)
+ if returncode != 0:
+ print "Error: command exited with non-zero code %d" % returncode
+ return returncode


What's the reason behind this? This block is already inside a try: block so any error results in the code executing the except portion and issues an error message already.

Non-zero return values and exceptions aren't the same thing. I'd prefer runCommand to throw an exception on non-zero return status, but such a change is out of the scope of this PR.

Sure but I think this should be a separate PR. Could you revert here and open a new issue that is related to this? That would allow us to be a bit more organized and focused in our PR's

bdorney · 2017-08-25T10:07:13Z

ana_scans.py

+ for returncode in res.get():
+ if returncode != 0:
+ sys.exit(returncode)
+ pass


Again could you describe the rationale here? We're inside a try block already?

Also what are the possible returncode values?

returncode is the return value from the called command, and will be non-zero if it fails (Python returns non-zero on exceptions, SIGSEGV, SIGKILL, ...). For this to work, we should ensure that our tools also return non-zero on "soft" failures (i.e. no crash), but that's a different issue.

As above let's do this in a separate PR with a new issue

bdorney

Good start to implementing this feature but changes and clarifications needed.

Also I think additional reviews beyond myself are needed due to the importance of some of these changes.

lmoureaux · 2017-08-25T10:09:13Z

Ubuntu's ROOT packages

Can you confirm this is:
ROOT 5.34/36 (heads/v5-34-00-patches@v5-34-34-76-g57bae4c, Apr 08 2016, 12:05:00 on linuxx8664gcc)

amd64 root-system-bin amd64 5.34.14-1build1

Same architecture, major and minor version. Patch number is different, though. I don't know of a simple way to install a specific ROOT version on Debian (and I don't want to compile it).

Can you confirm these are:

Python 2.6.6
Python 2.7.5

Python 2.6.13 and 2.7.13 (more recent patch versions, no behavior change expected). I can look into specifying patch numbers, but I'm not sure whether Travis still supports 2.6.6 (since even 2.6.13 is deprecated as it's not supported by the Python team anymore).

cmsgemos from branch master

Please use generic-amc-release-v3 of cmsgemos instead, found here.

Checked, it uses the default branch, which is generic-amc-release-v3 for now. Should I specify the branch explicitly?

One question. How does Travis CI update? e.g. if there is an update to cmsgemos what does one do to ensure that this update is included?

The latest cmsgemos if fetched from git every time the test is run.

bdorney · 2017-08-25T10:18:15Z

Same architecture, major and minor version. Patch number is different, though. I don't know of a simple way to install a specific ROOT version on Debian (and I don't want to compile it).

Compiled sources are available, for example for v5.34.36:
https://root.cern.ch/content/release-53436

Checked, it uses the default branch, which is generic-amc-release-v3 for now. Should I specify the branch explicitly?

No let's rely on the correct setup of the cmsgemos repo to point git to this branch as default. This can only be changed by an admin of the cmsgemos repo which I think includes only @jsturdy presently.

The latest cmsgemos if fetched from git every time the test is run.

Yeah I saw after looking at the code.

jsturdy · 2017-08-25T10:20:00Z

Sorry to not jump on this earlier, but it would be better if you can set it up to work with the docker container I set up for cmsgemos CI (work in progress, and once it's sorted, the plan was to simply plug in the other packages)

Here's the travis.yml I've been working with: https://github.com/jsturdy/cmsgemos/blob/jsturdy-travis-int-test/.travis.yml (I also have templates in vfatqc-python-scripts
Here's the docker I currently test against: https://gitlab.cern.ch/sturdy/gemdaq_ci_worker, though I was having some issues getting things working the way I wanted from github and moved to other tasks with higher priority

Other things to probably keep in mind moving forward:

Using the docker container (which can be updated on an as-needed basis) allows to maintain a testing environment that will be as close to operation as possible
- I will update the container semi-regularly to make sure it mimics the machines we typically run software on
- The container can even be set up to have a directory structure that hosts some dummy data to run the unit tests against
Testing against the "latest" cmsgemos isn't the best practice, but can suffice for now. At a future point you can add the gempython package (to be) produced by cmsgemos to the list of requirements, and then specify a version
You can also look into using tox for python unit testing, which integrates easily with travis, though I've not looked deeply into what it offers over simple unit testing run inside travis

lmoureaux · 2017-08-25T16:24:07Z

In today's update:

Using ROOT binaries downloaded directly from the ROOT website (v5.34.36)
Install cmsgemos' Python dependencies (none atm)
Cleanup .gitignores as requested
The test script now checks that all output files are present and non-empty
Ensures that tests are run from $DATA_PATH
Changes to ana_scans.py were moved to PR Let ana_scans fail when any command fails #29. The code is kept here because it's needed for this branch to work properly. I will revert just before merging master once PR Let ana_scans fail when any command fails #29 gets merged.

I considered using @jsturdy's Docker image, but I couldn't even run ls from inside the container. As I don't have the expertise to fix it (both on Docker and on how to setup cmsgemos), I went ahead with Ubuntu as the second-best solution. The tests themselves will be easily reusable on Docker anyway.

I considered using tox, but it looked like overkill (and overlapping with Travis). I considered using Python's own unittest module but IMHO it's a lot of code for little added value.

Still not addressed:

/bin/env -> /usr/bin/env (?)
Using the exact same Python dot versions as in prod (not supported by Travis, I don't think it's worth the effort given that the Docker image will provide a well-controlled environment)
Use P5 data (unsure, see discussion above)
Test other scripts than anaUltraScurve.py (will start working on it once the principle is 👍'ed, we might as well have one PR/tested script)

jsturdy · 2017-08-29T13:08:33Z

I tracked down a couple of issues with the docker container that was being used, and have managed to get things looking pretty much as expected from within the travis environment. If you can let me know what you would need to add to the container to complete your tests (unless you just want to run the installation commands in your travis script), I can update the container.

In the docker image you can just use yum install root\* to get the version of root provided with slc6/cc7
Extra python dependencies should be handled only through a requirements.txt or a requirements-dev/prod/test.txt and pip
It would be best to have the test data and directories set up in the container itself, or, at the very least, pulled via wget into the travis build, rather than stored in the repo
While adding the test script (there should really be a single test driver to test all aspects of the package, which was why I had suggested tox) it would be good to add the test coverage (coveralls, codecov, etc., you can also look at my integrations branch for some testing I had been setting up, though not yet operational)

One other thing that would be optimal would be to separate the unit testing from the CI/CD setup, such that the unit tests can be run independently of the CI/CD, and will then easily plug into a functional CI/CD framework (there will be two, by virtue of the fact that the gitlab mirrors are using gitlab-ci, while the github repositories will use travis)

While moving along these lines, the best way forward will be for you to regularly do a git rebase to the mainline branch, which should be straightforward as long as you're not modifying any of the code unrelated to the travis aspects (which this PR shouldn't be doing anyway...)

jsturdy · 2017-10-07T15:56:04Z

closing as "On Hold"

lmoureaux added 12 commits August 24, 2017 11:09

Initial Travis CI file without any testing

48ca629

Install packages listed in requirements.txt

051a87c

Fetch cmsgemos

0781a91

Setup environment

bbe34ec

Setup data path and chamber config for Travis

726c190

Add S-curve analysis test

c7ca1a0

Use /usr/bin/env instead of /bin/env

7374482

This is required for the scripts to run on Debian-based machines (Travis CI uses Ubuntu). /usr/bin/env is also valid on cosmicstandtif.

Fix call to ana_scans.py in .travis.yml

373b5f0

Add Ubuntu 14.04's pyroot to PYTHON_PATH

e4bc9c1

Let ana_scans.py fail when the analysis fails

b4709ee

Merge remote-tracking branch 'origin/master' into travis

c0b7ed8

This will fix bug 24 and should turn Travis green again

Install root histogram plugins in .travis.yml

b768a4b

bdorney added Priority: High Type: Maintenance Type: Enhancement and removed Type: Maintenance labels Aug 24, 2017

Add missing .gitignore in .travis/data/travis

11592d9

bdorney reviewed Aug 25, 2017

View reviewed changes

bdorney suggested changes Aug 25, 2017

View reviewed changes

This was referenced Aug 25, 2017

ana_scans.py doesn't fail if one of the subcommand fails #28

Closed

Let ana_scans fail when any command fails #29

Merged

lmoureaux added 12 commits August 25, 2017 15:39

Use ROOT 5.34.36 from ROOT's website

5dde8d1

Move ROOT installation to before_install

2268c3f

Source thisroot.sh before trying to install root-numpy

fedbf59

Don't source .travis/install_root.sh

f3f3d69

Install dependencies listed in cmsgemos' requirements.txt

7df8e66

Cleanup .gitignore files

c6af9fe

Move S-curve test to a Python script

d6162a1

Check that all of anaUltraScurve's output files exist

658566a

Add test scripts to $PATH

1cdb7a5

Run tests from $DATA_PATH

3febef0

Fix filename of SCurveFitData.root

8100fe4

Request that all output files are at least 1kB

2bc0340

jsturdy added the Status: On Hold label Oct 7, 2017

jsturdy closed this Oct 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Integrate gem-plotting-tools with Travis CI #27

[RFC] Integrate gem-plotting-tools with Travis CI #27

lmoureaux commented Aug 24, 2017 •

edited

Loading

lmoureaux commented Aug 24, 2017

bdorney commented Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

bdorney Aug 25, 2017 •

edited

Loading

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017 •

edited

Loading

lmoureaux Aug 25, 2017 •

edited

Loading

bdorney Aug 25, 2017

bdorney Aug 25, 2017

lmoureaux Aug 25, 2017

bdorney Aug 25, 2017

bdorney left a comment •

edited

Loading

lmoureaux commented Aug 25, 2017

bdorney commented Aug 25, 2017

jsturdy commented Aug 25, 2017 •

edited

Loading

lmoureaux commented Aug 25, 2017 •

edited

Loading

jsturdy commented Aug 29, 2017

jsturdy commented Oct 7, 2017

[RFC] Integrate gem-plotting-tools with Travis CI #27

[RFC] Integrate gem-plotting-tools with Travis CI #27

Conversation

lmoureaux commented Aug 24, 2017 • edited Loading

Description

Types of changes

Motivation and Context

How Has This Been Tested?

Screenshots

Checklist:

lmoureaux commented Aug 24, 2017

bdorney commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdorney Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdorney Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

lmoureaux Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdorney left a comment • edited Loading

Choose a reason for hiding this comment

lmoureaux commented Aug 25, 2017

bdorney commented Aug 25, 2017

jsturdy commented Aug 25, 2017 • edited Loading

lmoureaux commented Aug 25, 2017 • edited Loading

jsturdy commented Aug 29, 2017

jsturdy commented Oct 7, 2017

lmoureaux commented Aug 24, 2017 •

edited

Loading

bdorney Aug 25, 2017 •

edited

Loading

bdorney Aug 25, 2017 •

edited

Loading

lmoureaux Aug 25, 2017 •

edited

Loading

bdorney left a comment •

edited

Loading

jsturdy commented Aug 25, 2017 •

edited

Loading

lmoureaux commented Aug 25, 2017 •

edited

Loading