[AIRFLOW-116] Add a version view to display airflow version info #1504

r39132 · 2016-05-16T22:37:43Z

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

https://issues.apache.org/jira/browse/AIRFLOW-116

The code supports both release and develop versions and works for both python setup.py develop/install.

aoen · 2016-05-16T22:54:36Z

airflow/www/views.py

+        # Get the Git repo and git hash : first try pip freeze, then look at git
+        git_repo = git_hash = None
+        try:
+            p = subprocess.Popen('pip freeze | grep airflow',


I'm a little bit uncomfortable about adding a dependency on PIP (also it's possible that the running version of airflow doesn't match this version if e.g. it has a different PYTHONPATH). One idea is to show the hard-coded airflow version plus have a version file generated by setup-tools if this is possible indicating the SHA of the repo used to generate the package if relevant (this can be in v2). Since Airflow is aware of where it is running from it would always use the right "version" file. With this approach we would also remove the dependency on git (since the version would just be read from a file). The git dependency would be moved to the deploy process (generating the version file with the SHA) which is cleaner.

Pip is the recommended way to install python packages as it lets you easily rollback, not to mention its the first step all of us took as per the folowing link http://pythonhosted.org/airflow/start.html

Also, this is a soft dependency. If PIP is not available, it will fall through to the git implementation.

After a while, most people decided to run off their own forks. This is partly due to the unpredictably long release cycles. For those folks, they will install from their own git repos.

I understand I'm just not sure it's a good idea to create a dependency on PIP/git in the code as it violates separation of concerns (the code shouldn't know anything about what service it is deployed with), might be worth getting some more opinions on this. Also there is the issue with pip returning the wrong version (e.g. due to PYTHONPATH being off or airflow being run in a virtualenv). Same goes for the git dependency (unless we can move that to the deploy process).

Would be useful to get some more opinions on this:
cc @criccomini @plypaul @jlowin

Check out how I do it in Caravel:
https://github.com/airbnb/caravel/blob/master/setup.py#L4

In the git case, I load the repo based on where $AIRFLOW_HOME points. That pointer is authoritative. In terms of the PYTHONPATH and venv cases, I'd love some examples. I personally run off a git fork and use python setup.py install to install the code. However, many in the community don't maintain forks and instead using pip to install

Consider that we have users and developers. Most users tend to install using standard package managers. Today, since our user documentation points to pip and also since pip plays better with automation (e.g. easy to roll forward and backward), I don't think pip will go away.

-s

mistercrunch · 2016-05-17T00:06:06Z

airflow/www/views.py

+
+        # Render information
+        title = "Version Info"
+        git_proj_uri = git_repo.replace('git@','').replace('.git','').replace(":","/")


space after commas in calls

mistercrunch · 2016-05-17T00:08:54Z

airflow/www/views.py

@@ -15,6 +15,8 @@
 import sys

 import os
+import pip


mistercrunch · 2016-05-17T00:15:59Z

airflow/www/views.py

+                repo = Repo(os.path.expanduser(settings.AIRFLOW_HOME))
+                head_commit = repo.head.commit
+                git_hash = head_commit.hexsha
+                git_repo=repo.remotes.origin.url


oddly my git repo for airflow doesn't have an origin since I prefer using org names, this would raise

r39132 · 2016-05-17T02:21:08Z

airflow/www/templates/airflow/version.html

+{% block body %}
+    {{ super() }}
+    <h2>{{ title }}</h2>
+    {% if airflow_version %}


@aoen
I suspect your comment was on the wrong line. There was a duplicate title earlier, but that has been removed yet your comment persists. Please clean up and I will do the same of this comment.

criccomini · 2016-05-17T03:44:59Z

I like the way version is handled now (I didn't like depending on pip for it). The git stuff makes me nervous, though. I think if we want to include git checksums, we have to back the checksum into the package when it's built--we can't depend on the Airflow instance having a git repo in its source tree.

mistercrunch · 2016-05-17T04:08:42Z

I've been thinking about this and I think the right time to get a git SHA is at build time (so in setup.py). After that you can't assume that the python files are in a Git repo. They're most likely in a virtualenv or /usr/lib, in which case there's no git repo info.

r39132 · 2016-05-17T14:16:33Z

Maybe a dumb question. What happens for the vast majority of people who are using pip to install? I'm not familiar with what happens during the Pypi creation process -- @aoen are you ensuring that a git hash file is created at that time?

Adding support in setup.py handles a small minority of cases (e.g. developers), but ignores the vast majority of cases (users).
cc @mistercrunch

criccomini · 2016-05-17T16:04:35Z

Thinking about the build-time commit hash a bit. A (hacky) way of doing this would be to just add a subproc call in setup.py that runs:

git log --pretty=format:'%H' -n 1 > CHECKSUM_LOCATION

And then making sure that setup.py bundles the file in the package appropriately.

A more elegant solution would be to use the git library that Sid is using in setup.py.

aoen · 2016-05-17T17:29:24Z

In my initial comment:

One idea is to show the hard-coded airflow version plus have a version file generated by setup-tools if this is possible

I meant something like @criccomini mentioned (although more along the lines of git rev-parse HEAD or using something like this https://github.com/pyfidelity/setuptools-git-version , I'm OK with either)

r39132 · 2016-05-17T22:03:34Z

I'm moving ahead with moving the "git code" above to setup.py.

r39132 · 2016-05-17T22:34:11Z

It looks like this (infrequent releases and getting a git hash at setup time) is a problem others have discussed as well : Lasagne/Lasagne#574

@criccomini @mistercrunch @aoen
Note the discussion in the link above. I'm still a bit new to setup tools (coming from the Java world), but I've tested some of these changes. Appears that this solution may not work under python setup.py develop. I'm testing this for myself to verify if this is true of course.

r39132 · 2016-05-18T23:49:18Z

@aoen I tried https://github.com/pyfidelity/setuptools-git-version, the link you posted above. Where is the git version written for setup.py develop and install?

aoen · 2016-05-18T23:56:24Z

I don't think it is unfortunately, this small function on SO might be helpful though (taking the output of that and writing it to a file):
http://stackoverflow.com/questions/17812164/embed-git-hash-into-python-file-when-installing

r39132 · 2016-05-18T23:57:54Z

@aoen What about python setup.py develop? That would helpful as well

aoen · 2016-05-19T00:59:40Z

airflow/www/templates/airflow/version.html

+        <h4>Version : Not Available</a></h4>
+    {% endif %}
+
+    {% if git_version %}


Nit: use same title for both of these (git version vs git commit tree), can also refactor this to avoid duplication (the only difference being "{{ git_version }}" vs "Not Available"

Can you provide a snippet? It might be clearer.

Oh I see vis-a-vis title, but I suspect you are thinking of a ternary operator to squeeze this in to a single line

You can keep the current operator just bring it's scope in a little:

<h4>Git Version :{% if git_version %} {{ git_version }} {% else %} Not Available {% endif %}</a> <h4>

Thx for the template foo!

Eh. looks like these comments are on the wrong lines.

r39132 · 2016-05-19T03:03:54Z

setup.py

+                  },
+    )
+
+


2 seems standard. I need to install flake8 on my box now that we don't have landscape.io running. But, I'm pretty sure 2 is adequate. Your earlier comment was when there were 3.

Please remove comment.

PEP8 says 2 lines before classes and function definition but not before an if. It's really not shocking though when landscape.io starts policing again it will note this as a "code smell". You can get flake8 to install commit hooks so that you see those on commit. flake8 --install-hook

These are all addressed in a new PR : https://github.com/apache/incubator-airflow/pull/1523/files

mistercrunch · 2016-05-19T15:17:37Z

setup.py

@@ -35,6 +38,44 @@ def run(self):
        os.system('rm -vrf ./build ./dist ./*.pyc ./*.tgz ./*.egg-info')


+def git_version(version):


+doc string

aoen reviewed May 16, 2016
View reviewed changes

r39132 force-pushed the master branch 3 times, most recently from 51ec852 to ae02ea7 Compare May 16, 2016 23:22

mistercrunch reviewed May 17, 2016
View reviewed changes

r39132 force-pushed the master branch from ae02ea7 to 6aeb614 Compare May 17, 2016 00:08

mistercrunch reviewed May 17, 2016
View reviewed changes

airflow/www/views.py

@@ -15,6 +15,8 @@

import sys

import os

import pip

Copy link

Member

mistercrunch May 17, 2016

Choose a reason for hiding this comment

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

dd

r39132 changed the title ~~Add a version view to display airflow version info~~ [AIRFLOW-116] Add a version view to display airflow version info May 17, 2016

mistercrunch reviewed May 17, 2016
View reviewed changes

r39132 force-pushed the master branch from 6aeb614 to 4f908d5 Compare May 17, 2016 00:52

r39132 reviewed May 17, 2016
View reviewed changes

aoen reviewed May 19, 2016
View reviewed changes

r39132 force-pushed the master branch from 76960a6 to 1f5e451 Compare May 19, 2016 02:59

r39132 reviewed May 19, 2016
View reviewed changes

r39132 force-pushed the master branch 3 times, most recently from 6747bde to a331167 Compare May 19, 2016 06:03

Add a version view to display airflow version info

7d32c17

r39132 force-pushed the master branch from a331167 to 7d32c17 Compare May 19, 2016 06:24

asfgit merged commit 7d32c17 into apache:master May 19, 2016

mistercrunch reviewed May 19, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-116] Add a version view to display airflow version info #1504

[AIRFLOW-116] Add a version view to display airflow version info #1504

r39132 commented May 16, 2016 •

edited

Loading

aoen May 16, 2016 •

edited

Loading

r39132 May 16, 2016

aoen May 16, 2016

mistercrunch May 16, 2016

r39132 May 16, 2016

mistercrunch May 17, 2016 •

edited

Loading

mistercrunch May 17, 2016

mistercrunch May 17, 2016

r39132 May 17, 2016 •

edited

Loading

criccomini commented May 17, 2016

mistercrunch commented May 17, 2016

r39132 commented May 17, 2016 •

edited

Loading

criccomini commented May 17, 2016

aoen commented May 17, 2016

r39132 commented May 17, 2016

r39132 commented May 17, 2016 •

edited

Loading

r39132 commented May 18, 2016

aoen commented May 18, 2016

r39132 commented May 18, 2016

aoen May 19, 2016

r39132 May 19, 2016

r39132 May 19, 2016

aoen May 19, 2016

r39132 May 19, 2016

r39132 May 19, 2016

r39132 May 19, 2016

mistercrunch May 19, 2016

r39132 May 19, 2016

r39132 May 19, 2016

mistercrunch May 19, 2016

r39132 May 19, 2016

		@@ -35,6 +38,44 @@ def run(self):
		os.system('rm -vrf ./build ./dist ./.pyc ./.tgz ./*.egg-info')


		def git_version(version):

[AIRFLOW-116] Add a version view to display airflow version info #1504

[AIRFLOW-116] Add a version view to display airflow version info #1504

Conversation

r39132 commented May 16, 2016 • edited Loading

aoen May 16, 2016 • 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

mistercrunch May 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r39132 May 17, 2016 • edited Loading

Choose a reason for hiding this comment

criccomini commented May 17, 2016

mistercrunch commented May 17, 2016

r39132 commented May 17, 2016 • edited Loading

criccomini commented May 17, 2016

aoen commented May 17, 2016

r39132 commented May 17, 2016

r39132 commented May 17, 2016 • edited Loading

r39132 commented May 18, 2016

aoen commented May 18, 2016

r39132 commented May 18, 2016

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

Choose a reason for hiding this comment

r39132 commented May 16, 2016 •

edited

Loading

aoen May 16, 2016 •

edited

Loading

mistercrunch May 17, 2016 •

edited

Loading

r39132 May 17, 2016 •

edited

Loading

r39132 commented May 17, 2016 •

edited

Loading

r39132 commented May 17, 2016 •

edited

Loading