Arvados

root@e9972d7acb01:~# cat > after-inst <<EOF
> #!/bin/sh
> cd /usr/bin
> ln -s /usr/share/python/arvados-cwl-runner/bin/arvados-cwl-runner
> ln -s /usr/share/python/arvados-cwl-runner/bin/arvados-cwl-runner cwl-runner
> EOF
root@e9972d7acb01:~# fpm --after-install=after-inst -s virtualenv -t deb -n arvados-cwl-runner arvados-cwl-runner 

Related, using fpm+gem to produce self-contained Ruby packages: http://blog.gemnasium.com/post/60091718748/package-your-ruby-based-tool-the-safe-and-easy-way

Ready for (initial?) review at 5a09ff5d42b0f8b71ca6775813e0844067363e12

This still builds our backported libcloud package.

I'm not sure why this

set -e
arvados-node-manager --version
set +e

PYTHON=`ls /usr/share/python2.7/dist/arvados-node-manager/bin/python2.7`

if [ "$PYTHON" != "" ]; then
  set -e
  exec $PYTHON <<EOF
...
else
  exit 1
fi

...isn't it equivalent to just using an explicit python binary like this?

set -e
arvados-node-manager --version
exec /usr/share/python2.7/dist/arvados-node-manager/bin/python2.7 <<EOF
...

• In the arvados-node-manager case, it tests some libcloud thing that (I'm guessing) isn't tested by "arvados-node-manager --version", which seems fine.
• In the arvados-cwl-runner case, I'm not sure what the inline script tells us that `arvados-cwl-runner --version` doesn't.
• In the arvados-fuse case, it looks like we test a script that imports arvados_fuse (which nobody does, afaik) but we don't try running "arv-mount --version", which seems more relevant. This seems like a pre-existing bug, but now might be an appropriate time to fix this one, since "run arv-mount from PATH and expect it to import all its things, including llfuse C libs" is the sort of thing we could be breaking here.
• Likewise, in arvados-python-client, we should add something like "arv-put --help".

In fpm_build_virtualenv...

The "*" should be quoted or escaped here:

+  find build -iname *.pyc -exec rm {} \;
+  find build -iname *.pyo -exec rm {} \;

There are some explicit checks for $? so I'm suspecting "set -e" isn't in force in fpm_build_virtualenv, so there are some unchecked errors (including the above "find" commands, "pip install pip", and "cp -f"). I suppose we can't do "set -e" somewhere without accidentally making global changes and having to rewrite Everything...

Suggest

-if [[ "${DEBUG:-0}" != "0" ]]; then ...
+if [[ -n "${DEBUG}" ]]; then ...

Some typos "Arvadow", "execurin", "sectiontoo"

What's the story behind changing the version discovery? It used to be passed in from run-build-packages ("ARVADOS_BUILDING_VERSION if given, else extract from egg PKG-INFO / ARVADOS_BUILDING_ITERATION if given, else depends on package"); now, run-library runs "setup.py sdist" and parses the resulting tarball filename to get VERSION, and defaults to iteration "-1". (I think setting iteration to -1 is a good move, except that if we want this merge to produce a set of dev packages, we might need to either set iteration to -5 or update the last-change-detection hairball to notice changes in /build. Ugh)

I think source:services/dockercleaner/arvados-docker-cleaner.service needs to be updated: python33→python35, and fix the hardcoded bin path used to detect scl:

ExecStart=/bin/sh -c 'if [ -e /opt/rh/python33/root/bin/arvados-docker-cleaner ]; then exec scl enable python33 arvados-docker-cleaner; else exec arvados-docker-cleaner; fi'

• The new prerm/postinst scripts duplicate some stuff from the old ones, and don't seem to enable the systemd units, or check whether $1 indicates the appropriate hook (the old ones do this, so I'm guessing it's needed despite the names of the fpm args)
• The new prerm/postinst scripts get autogenerated seems messy so I'm wondering if there's another way. Could it list files in /usr/share/python*/dist/$pkg/bin/ at runtime? Or are there some fpm args we can use to install our own bin stubs/symlinks as /usr/bin/whatever? Even if we still need to autogenerate the stubs, at least that way yum/apt would know where they came from, avoid installing other packages with conflicting binaries, etc.
• The new fpm_build_virtualenv has a lot of overlap with fpm_build, but does some things differently (e.g., it calls test_package_presence instead of letting the caller do that). Meanwhile the old fpm_build still has lots of (now unused?) python-specific stuff. I don't know which is less messy (two similar funcs vs. lots of if-else). Splitting might be better but it would be good to keep them congruent.

Nit/style thing: I find it easier to follow "if ! foo; then ...; fi" than "foo; if $? != 0 ; then ...; fi". The first form also has an advantage that it works the same way in "set -e" and "set +e" contexts.

The deletion of build.list (and its version compatibility maintenance nightmare) sure makes me happy... this is very encouraging. Thanks!

Updated branch ready for another look at 89bc5fc945891bec4322fe14b6d11d0cdc1ca267

Tom Clegg wrote:

I'm not sure why this

[...]

...isn't it equivalent to just using an explicit python binary like this?

[...]

Of course, the whole idea of an "import" test seems a bit weird here. Mostly this is scope creep, but I'll observe anyway...

• In the arvados-node-manager case, it tests some libcloud thing that (I'm guessing) isn't tested by "arvados-node-manager --version", which seems fine.
• In the arvados-cwl-runner case, I'm not sure what the inline script tells us that `arvados-cwl-runner --version` doesn't.
• In the arvados-fuse case, it looks like we test a script that imports arvados_fuse (which nobody does, afaik) but we don't try running "arv-mount --version", which seems more relevant. This seems like a pre-existing bug, but now might be an appropriate time to fix this one, since "run arv-mount from PATH and expect it to import all its things, including llfuse C libs" is the sort of thing we could be breaking here.
• Likewise, in arvados-python-client, we should add something like "arv-put --help".

Agreed with all that, and all fixed.

In fpm_build_virtualenv...

The "*" should be quoted or escaped here:

[...]

Done.

There are some explicit checks for $? so I'm suspecting "set -e" isn't in force in fpm_build_virtualenv, so there are some unchecked errors (including the above "find" commands, "pip install pip", and "cp -f"). I suppose we can't do "set -e" somewhere without accidentally making global changes and having to rewrite Everything...

Yeah. I added some explicit exit code testing to the calls that are important (pip, etc). The find calls don't really matter that much, they just clean up compiled objects, but there shouldn't be any to start with since this typically runs in a clean checkout.

Suggest
[...]

The default being set here (0) was superfluous (fixed), we already do that higher in the script. But we can't just use the -n test, because $DEBUG is either 0 or 1.

Some typos "Arvadow", "execurin", "sectiontoo"

All fixed.

What's the story behind changing the version discovery? It used to be passed in from run-build-packages ("ARVADOS_BUILDING_VERSION if given, else extract from egg PKG-INFO / ARVADOS_BUILDING_ITERATION if given, else depends on package"); now, run-library runs "setup.py sdist" and parses the resulting tarball filename to get VERSION, and defaults to iteration "-1". (I think setting iteration to -1 is a good move, except that if we want this merge to produce a set of dev packages, we might need to either set iteration to -5 or update the last-change-detection hairball to notice changes in /build. Ugh)

Good call. I changed it to the original behavior, which is more succinct too. I'm not too worried about resetting the iteration to 1, because this branch touches sdk/python we're getting a new version for everything anyway.

I think source:services/dockercleaner/arvados-docker-cleaner.service needs to be updated: python33→python35, and fix the hardcoded bin path used to detect scl:
[...]

Yes! fixed.

I'm wary of the way the prerm/postinst scripts and the fpm_build function are getting forked here.

• The new prerm/postinst scripts duplicate some stuff from the old ones, and don't seem to enable the systemd units, or check whether $1 indicates the appropriate hook (the old ones do this, so I'm guessing it's needed despite the names of the fpm args)
• The new prerm/postinst scripts get autogenerated seems messy so I'm wondering if there's another way. Could it list files in /usr/share/python*/dist/$pkg/bin/ at runtime? Or are there some fpm args we can use to install our own bin stubs/symlinks as /usr/bin/whatever? Even if we still need to autogenerate the stubs, at least that way yum/apt would know where they came from, avoid installing other packages with conflicting binaries, etc.

Yes. I've removed the new prerm/postinst scripts and done this the right way now. Thanks!

• The new fpm_build_virtualenv has a lot of overlap with fpm_build, but does some things differently (e.g., it calls test_package_presence instead of letting the caller do that).

I moved that particular test into fpm_build_virtualenv to make run-build-packages simpler. There was a lot of code duplication there running that test.

Meanwhile the old fpm_build still has lots of (now unused?) python-specific stuff. I don't know which is less messy (two similar funcs vs. lots of if-else). Splitting might be better but it would be good to keep them congruent.

I left the code in place that builds our own copy of libcloud. That uses the old fpm_build, which is why I didn't remove all the python stuff from that yet. I'm kind of tempted to leave things like this until we retire node manager. Unless you think that would take too long?

Libcloud is the only loose end I'm still not clear on. We often need to fork it, and building arvados-node-manager with virtualenv is going to require a new strategy for that, which I haven't figured out yet.

Nit/style thing: I find it easier to follow "if ! foo; then ...; fi" than "foo; if $? != 0 ; then ...; fi". The first form also has an advantage that it works the same way in "set -e" and "set +e" contexts.

Yeah, I cleaned that up in the fpm_build_virtualenv function.

The deletion of build.list (and its version compatibility maintenance nightmare) sure makes me happy... this is very encouraging. Thanks!

Indeed! I like deleting a lot of crufty code.

This looks much better than the previous postinst approach, but shouldn't we be installing to /usr/bin rather than /usr/local/bin?

  # Make sure to install all our package binaries in /usr/local/bin.

• maybe it's only a small amount of work to give it the virtualenv treatment too (I'm hoping so -- is this a matter of find a way to substitute our own fork for the pypi version while building the virtualenv?), or
• otherwise, it seems like we'd be unable to update node manager's dependencies because we no longer build deb/rpm packages that could satisfy them at runtime, which seems like a precarious situation wrt security fixes and cloud API changes. crunch-dispatch-cloud hasn't even started AWS/GCE drivers yet...

1. use "import arvados" without setting up a virtualenv
2. use "import arvados" in a virtualenv where other python packages are installed (perhaps from setup.py / requirements.txt), but arvados is not installed

(The first case is currently our recommended setup -- If you are logged in to an Arvados VM, the Python SDK should be installed. and if not, "option 1" is to install the deb/rpm package.)

• provide a workaround -- perhaps change shebang to "#!/usr/share/python2.7/dist/python-arvados-python-client/bin/python" as in the package-testing script? (this only works for the first case)
• help people migrate away from this setup by installing arvados from pypi into a virtualenv (this should be an easy fix in the second case)
• warn about all this on https://doc.arvados.org/admin/upgrading.html

See 6e1f3a9a91a694f3ea547f23f924de299f481902

Tom Clegg wrote:

This looks much better than the previous postinst approach, but shouldn't we be installing to /usr/bin rather than /usr/local/bin?
[...]

Yes, corrected.

It would be nice not to let node manager's special needs hold this up, but I'm wondering...

• maybe it's only a small amount of work to give it the virtualenv treatment too (I'm hoping so -- is this a matter of find a way to substitute our own fork for the pypi version while building the virtualenv?), or
• otherwise, it seems like we'd be unable to update node manager's dependencies because we no longer build deb/rpm packages that could satisfy them at runtime, which seems like a precarious situation wrt security fixes and cloud API changes. crunch-dispatch-cloud hasn't even started AWS/GCE drivers yet...

arvados-node-manager is already using virtualenv. I removed the code that makes a deb/rpm package from our backported libcloud; if we need a backport for libcloud, it will need to be pushed to pypi and listed as a dependency for arvados-node-manager in setup.py, and then it will get pulled into the arvados-node-manager virtualenv on the next package build.

Before this branch, with the arvados-python-client debian package installed, you can
use "import arvados" without setting up a virtualenv
use "import arvados" in a virtualenv where other python packages are installed (perhaps from setup.py / requirements.txt), but arvados is > not installed
(The first case is currently our recommended setup -- If you are logged in to an Arvados VM, the Python SDK should be installed. and if > not, "option 1" is to install the deb/rpm package.)

I've reordered the options, since using the deb/rpm package is probably not the default workflow for software developers that want to use the Python SDK.

I suppose we need to
provide a workaround -- perhaps change shebang to "#!/usr/share/python2.7/dist/python-arvados-python-client/bin/python" as in the
package-testing script? (this only works for the first case)

I have done that.

help people migrate away from this setup by installing arvados from pypi into a virtualenv (this should be an easy fix in the second case)

I'm not 100% sure what you mean here?

warn about all this on https://doc.arvados.org/admin/upgrading.html

Done.

Ward Vandewege wrote:

arvados-node-manager is already using virtualenv. I removed the code that makes a deb/rpm package from our backported libcloud; if we need a backport for libcloud, it will need to be pushed to pypi and listed as a dependency for arvados-node-manager in setup.py, and then it will get pulled into the arvados-node-manager virtualenv on the next package build.

Ah, I see. The only issue is that we used to rely on a deb/rpm-based solution for switching between upstream and fork, so we don't have a Python-native way yet. This seems fine.

Ward Vandewege wrote:

help people migrate away from this setup by installing arvados from pypi into a virtualenv (this should be an easy fix in the second case)

I'm not 100% sure what you mean here?

1. don't create a virtualenv
2. write a script that imports arvados

1. create a virtualenv
2. install some unrelated python package "foobar" in the virtualenv
3. write a script that uses the virtualenv and imports both foobar and arvados packages

In both cases, updating the shell node to a new python-arvados-python-client debian package will break the script. I was just making the point that cases 1 and 2 need different fixes.

• create a virtualenv, run "pip install arvados" there, and update shebang (or script invocation) to use the virtualenv (this way is surely better)
• update shebang (or script invocation) to use the virtualenv that ships with the new python-arvados-python-client package (this way might be a quicker emergency fix so it seems nice to mention it)

• run "$virtualenv/bin/pip install arvados" on existing virtualenv.

• Should we really recommend "git clone; setup.py install" rather than "pip install arvados-python-client"? (Avoiding pip seems to imply there's some reason not to use it, which would mean there's a reason not to use setup.py, requirements.txt, etc. as well.)
• I think the upgrade notes should give specific instructions for bringing broken scripts back to life.
• Perhaps the SDK install page should say that option 1 is the recommended way to make the SDK available for use in your own python programs, while option 3 is the recommended way to make the CLI tools available system-wide.

(It might actually be less confusing if we had two different package names -- "arvados-python-cli-tools" (normally installed via deb/rpm, comes with its own virtualenv under the hood) and "arvados-python-client" (normally installed via pypi into your own virtualenv) -- but we should probably save that discussion for another day.)

See 7995d73085a0fd459edafa679e85c08c9c2b5605

Tom Clegg wrote:

Ward Vandewege wrote:

help people migrate away from this setup by installing arvados from pypi into a virtualenv (this should be an easy fix in the second case)

I'm not 100% sure what you mean here?

On a shell node with our recommended setup, you could do this ("case 1")

1. don't create a virtualenv
2. write a script that imports arvados

...or this ("case 2")

1. create a virtualenv
2. install some unrelated python package "foobar" in the virtualenv
3. write a script that uses the virtualenv and imports both foobar and arvados packages

In both cases, updating the shell node to a new python-arvados-python-client debian package will break the script. I was just making the point that cases 1 and 2 need different fixes.

Case 2 isn't real, because the virtualenv will not pull in the Arvados package that was installed system-wide:

~$ dpkg -l |grep arvados
ii  arvados-src                           1.3.0-1                                    all          The Arvados source code
ii  libpam-arvados                        1.3.0-1                                    all          PAM module for authenticating shell logins using Arvados API tokens
ii  python-arvados-cwl-runner             1.3.0-1                                    all          The Arvados CWL runner
ii  python-arvados-fuse                   1.3.0-1                                    all          The Keep FUSE driver
ii  python-arvados-python-client          1.3.0-1                                    all          The Arvados Python SDK
~$ virtualenv x
Running virtualenv with interpreter /usr/bin/python2
New python executable in /data-sdc/home/wardv/x/bin/python2
Also creating executable in /data-sdc/home/wardv/x/bin/python
Installing setuptools, pkg_resources, pip, wheel...source xdone.
~$ source x/bin/activate
(x) ~$ python
Python 2.7.12 (default, Dec  4 2017, 14:50:18) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import arvados
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named arvados
>>> 

Comments on the doc updates at 6e1f3a9a9:

• Should we really recommend "git clone; setup.py install" rather than "pip install arvados-python-client"? (Avoiding pip seems to imply there's some reason not to use it, which would mean there's a reason not to use setup.py, requirements.txt, etc. as well.)

I have no opinion on this so I agree with you. I have deleted that installation option from our documentation.

• I think the upgrade notes should give specific instructions for bringing broken scripts back to life.

Yes, done.

• Perhaps the SDK install page should say that option 1 is the recommended way to make the SDK available for use in your own python programs, while option 3 is the recommended way to make the CLI tools available system-wide.

Yes, also done.

(It might actually be less confusing if we had two different package names -- "arvados-python-cli-tools" (normally installed via deb/rpm, comes with its own virtualenv under the hood) and "arvados-python-client" (normally installed via pypi into your own virtualenv) -- but we should probably save that discussion for another day.)

Agree, and let's postpone that like you say :)

Ward Vandewege wrote:

Case 2 isn't real, because the virtualenv will not pull in the Arvados package that was installed system-wide:

Ah, that's a relief.

Doc updates LGTM.

1fd7b7089dd0027d36e46c2e1fbb44aee4cc1e66 on branch 9945-crunch-job-python-path is ready for review

There's another use of python ("Using Arvados SDK version...") -- does this also need the same change?

Another thought -- would it be better to prepend the virtualenv bin dir to PATH in the run script (see https://doc.arvados.org/install/install-crunch-dispatch.html)?

Tom Clegg wrote:

There's another use of python ("Using Arvados SDK version...") -- does this also need the same change?

This looks like an "ancient API server compatibility" thing so probably not.

Another thought -- would it be better to prepend the virtualenv bin dir to PATH in the run script (see https://doc.arvados.org/install/install-crunch-dispatch.html)?

If there's only one use, let's stick to the current way.

Tom Clegg wrote:

There's another use of python ("Using Arvados SDK version...") -- does this also need the same change?

No; that other use of python executes inside the docker container. Our standard 1.4 jobs image already makes sure that running 'python' just works.