Automating transplanting version in code #437

Hedingber · 2020-09-12T23:50:20Z

Background:
We don't want users to have to change their code with each new version of MLRun, therefore we let them set their jobs images untagged (e.g. mlrun/mlrun), and MLRun API enrich the image with tag (e.g. mlrun/mlrun:0.5.2).
For this to work MLRun code need to be aware of its version.
So far this version was hard coded so we had to change it when we release.
Also between releases we were hard coding it to unstable so that it will be possible to work with the "unstable version".

Requirements we had:

Hard coding this whole thing is a lot of mess, it requires a bump commit before releasing, and setting back to unstable after releasing, we would prefer something less manual and more automated.
We wanted to start using per-commit builds, e.g. unstable-<commit-hash> (currently this is for the system tests), in order for a commit builds to be a "version" the code inside the API need to be aware of that, we can't do manual bump commit per commit.
Currently those per-commit builds will be pushed only to Github Container registry - we need to add logic to the API to enrich the docker registry (in addition to the tag) + we need a way to configure that registry on runtime
It would be nice if beside of the version we will have info about the specific commit

Decisions & Solution:

Moved the version info outside of the code to a version.json file - currently looks like:

{
  "git_commit": "889e8bca5472cefa3290fed4218d0e8ad53fd6c7",
  "version": "unstable"
}

Removed some duplications with images enrichment + added registry enrichment
Added a script that override this file, some usage examples:

# can run with no inputs, this will just update the git commit and use defaults for the rest
python ./automation/version/run.py run 
make update-version-file
# with inputs
python ./automation/version/run.py run 0.5.2 
MLRUN_VERSION=0.5.2 make update-version-file

Added the update-version-file makefile target as a dependency of each targets that builds something, i.e. all images targets + package building target
The only exceptional case (I can think of) in which the version won't be set correctly is when a user (probably a developer) doing something like:
pip install git+https://github.com/mlrun/mlrun@development - The problem here is that this is basically installing an un-built version - straight from the source code, I tried to play a little bit with git hooks and stuff to check maybe we can make sure the version.json is getting automatically updated in every commit, but this is a bit of chicken and egg - the commit id is based on some checksum of the contents, and putting the id in the version.json file changing the contents...
so I decided that by default (sitting in two places, setup.py and version.py I'll return these values:

{
  "git_commit": "unknown",
  "version": "unstable"
}

…talization)

quaark

Awesome, just a couple of things

quaark · 2020-09-14T06:38:53Z

.github/workflows/ci.yaml

+      with:
+        python-version: 3.7
+    - name: Install automation scripts dependencies
+      run: pip install click


If there is now an automation directory, and scripts there have their own independent requirements, maybe we should have a requirements.txt in that dir and install it here with -r instead of adding another requirement here every time we'll need too.

quaark · 2020-09-14T06:39:07Z

.github/workflows/ci.yaml

+        with:
+          python-version: 3.7
+      - name: Install automation scripts dependencies
+        run: pip install click


quaark · 2020-09-14T06:47:32Z

setup.py

+    with open("version.json") as version_file:
+        for line in version_file:
+            if '"version"' in line:
+                # '  "version": "<version>",' -> ['  ', 'version', ': ', '<version>', ',']
+                return line.split('"')[3]


I think doing this here will be safer code:

with open("version.json") as version_file: try: version_metadata = json.load(version_file) return version_metadata['version'] except (ValueError, KeyError): return None

This is json. parse it as such
Also if possible (without circular import issues) to bring in a logger here and output something in case of error inferring the version - it might be very useful, otherwise missing version information will be hard to debug

tl;dr - json yes, logger no
I was thinking that it is impossible to import stuff in setup.py cause it's running before installation (before installing the requirements) but apparently it is possible using the pyproject.toml file (https://www.python.org/dev/peps/pep-0518/#build-system-table), also I wasn't aware but apparently anyway json is part of the standard library so need to pre-install anything. w.r.t. logger, it is impossible, I can't use the package here, because it is not installed yet

logger is python standard library :)

Indeed, thought you meant MLRun logger, added

quaark · 2020-09-14T06:48:46Z

version.json

@@ -0,0 +1,5 @@
+{


you added this file to .gitignore yet it is still tracked here.

probably added it by accident before putting it in .gitignore

Did it purposely, from the PR description:

and also the file will be committed to git but added to .gitignore.

why ?
I want to commit the file so it will have some sane defaults, I add it to git ignore so people won't have un-comitted changed everytime they do make api (or any other build)

Apparently this does not behave as I thought, as soon as the file is in the tree, git will track its changes even if its in .gitignore.
Removing from .gitignore for now

@Hedingber - As per the purpose of this file - it should not be tracked from a correctness point of view. I'm not sure what you mean by sane defaults but they can be in the parsing code, in the absence of this file if you must

Hedingber · 2020-09-19T21:04:06Z

Talked offline with @omesser and decided to remove the docker registry from the version file.
MLRun-wise it's easy to override the registry on runtime, simply adding the MLRUN_IMAGES_REGISTRY env var.
Iguazio-wise (also CI-wise) I will introduce some changes in the helm chart so we will be able to easily set that on a system

quaark

LGTM! Only a couple of styling and grammar issues in comments/doc strings. Otherwise, the code looks good.

quaark · 2020-09-21T06:05:27Z

mlrun/config.py

@@ -166,6 +186,11 @@ def _do_populate(env=None):
    if data:
        config.update(data)

+    # HACK to enable kfp_image property to both have dynamic default and to have use the value from dict/env like


Suggested change

# HACK to enable kfp_image property to both have dynamic default and to have use the value from dict/env like

# HACK to enable kfp_image property to both have dynamic default and to use the value from dict/env like

quaark · 2020-09-21T06:13:24Z

mlrun/config.py

+        """
+        When this configuration is not set we want to set it to mlrun/mlrun but we need to use the enrich_image method
+        The problem is that the mlrun.utils.helpers module is importing the config (this) module, so we must calculate
+        this property value here (and not initalization) and import inside
+        """


Suggested change

"""

When this configuration is not set we want to set it to mlrun/mlrun but we need to use the enrich_image method

The problem is that the mlrun.utils.helpers module is importing the config (this) module, so we must calculate

this property value here (and not initalization) and import inside

"""

"""

When this configuration is not set we want to set it to mlrun/mlrun, but we need to use the enrich_image method.

The problem is that the mlrun.utils.helpers module is importing the config (this) module, so we must import the module inside this function (and not on initalization), and then calculate this property value here.

"""

mlrun/config.py

mlrun/utils/version/version.py

setup.py

Hedingber added 25 commits September 10, 2020 14:45

Adding script that generates the version file

7f214c6

Don't use mlrun logger

2313ea7

Add version file

88253b8

read version from version file

da92541

tag_image -> enrich_image and add tests

551fcc1

tag_image -> enrich_image one more

ef31022

Add image_regsitry option to config

a0d03d0

Add version util and use it

dcab738

Move config computed properties to be resolved on get (and not on ini…

d09b115

…talization)

Use enrich_image in config also for kfp_image

85bf4a1

Trying

371d101

revert

1409bc0

default values for version.json file

878e014

default values for version.json file

3215dcb

Add version.json to git ignore

b75a570

Add update-version-file to makefile

1607ee6

what

0454610

Depend makefile targets on update-version-file and comments

6c8c9ff

rename

d54188a

rename ci directory to automation

9b489f0

Alinging Jenkinsfile

889e8bc

linting

4b38d8d

Adding version.json to git

b32948a

fixes

138b869

fixes

c969b12

Hedingber requested review from omesser and quaark September 13, 2020 00:03

Hedingber added 2 commits September 13, 2020 18:01

Fix path

a09ad23

Merge branch 'development' of github.com:mlrun/mlrun into version_stuff

3fad29d

quaark requested changes Sep 14, 2020

View reviewed changes

Hedingber added 10 commits September 15, 2020 16:52

.run instead

51b0f7f

rename file and command

0905b09

no need getter

d6594e8

no need getter

dbbe353

Move version.json inside the package

b253615

fix version file generation

e0dafb6

commit version file

f381d8b

remove docker registry from version file

320c757

linting

15bb42a

fix imports

2d301a0

Hedingber added 9 commits September 20, 2020 03:33

remove from git ignore

df893ec

Adding logging to setup.py

02a7c78

enrich_image -> enrich_image_url

2153942

Version to singleton class

980191f

add back to gitignore and default to sane defaults

c393ea9

remove version.json from tree

919291e

add comment

7c25490

linting

80614f1

what

da52256

omesser approved these changes Sep 20, 2020

View reviewed changes

oops

72e3e87

quaark requested changes Sep 21, 2020

View reviewed changes

Hedingber added 3 commits September 21, 2020 11:33

Merge branch 'development' of github.com:mlrun/mlrun into version_stuff

e2e3d2c

grammer

4b095e3

grammer

9c8094d

quaark approved these changes Sep 21, 2020

View reviewed changes

Hedingber merged commit 33aef35 into mlrun:development Sep 21, 2020

Hedingber mentioned this pull request Sep 22, 2020

Run update-version-file in docker #443

Merged

Hedingber mentioned this pull request Oct 25, 2020

Enable installing on python < 3.7 #494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automating transplanting version in code #437

Automating transplanting version in code #437

Hedingber commented Sep 12, 2020 •

edited

quaark left a comment

quaark Sep 14, 2020

Hedingber Sep 14, 2020

quaark Sep 14, 2020

quaark Sep 14, 2020

omesser Sep 14, 2020

Hedingber Sep 15, 2020

omesser Sep 20, 2020

Hedingber Sep 20, 2020

quaark Sep 14, 2020

omesser Sep 14, 2020

Hedingber Sep 15, 2020 •

edited

Hedingber Sep 20, 2020 •

edited

omesser Sep 20, 2020

Hedingber commented Sep 19, 2020

quaark left a comment

quaark Sep 21, 2020

quaark Sep 21, 2020

	# HACK to enable kfp_image property to both have dynamic default and to have use the value from dict/env like
	# HACK to enable kfp_image property to both have dynamic default and to use the value from dict/env like

Automating transplanting version in code #437

Automating transplanting version in code #437

Conversation

Hedingber commented Sep 12, 2020 • edited

quaark left a 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

Hedingber Sep 15, 2020 • edited

Choose a reason for hiding this comment

Hedingber Sep 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hedingber commented Sep 19, 2020

quaark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hedingber commented Sep 12, 2020 •

edited

Hedingber Sep 15, 2020 •

edited

Hedingber Sep 20, 2020 •

edited