Add to provenance log #968

shanaxel42 · 2019-01-23T16:56:04Z

Added fields for:
-core dependencies
-git hash
-os information

Also refactored the LogEncoder to be in the new utils.logging file.

codecov-io · 2019-01-23T17:00:46Z

Codecov Report

Merging #968 into master will increase coverage by 0.55%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   88.58%   89.14%   +0.55%     
==========================================
  Files         164      164              
  Lines        5977     6570     +593     
==========================================
+ Hits         5295     5857     +562     
- Misses        682      713      +31

Impacted Files	Coverage Δ
starfish/types/__init__.py	`100% <ø> (ø)`	⬆️
starfish/types/_constants.py	`97.61% <100%> (+0.05%)`	⬆️
starfish/__init__.py	`93.33% <100%> (+0.47%)`	⬆️
starfish/pipeline/algorithmbase.py	`100% <100%> (ø)`	⬆️
starfish/imagestack/imagestack.py	`86.2% <100%> (+2.23%)`	⬆️
starfish/util/logging.py	`96.66% <96.66%> (ø)`
starfish/starfish.py	`95.77% <0%> (-4.23%)`	⬇️
...h/spots/_detector/trackpy_local_max_peak_finder.py	`87.65% <0%> (-3.88%)`	⬇️
sptx_format/validate_sptx.py	`83.51% <0%> (-1.73%)`	⬇️
starfish/image/_filter/gaussian_low_pass.py	`95.65% <0%> (-1.02%)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 800bb18...e4ffa7e. Read the comment docs.

joshmoore · 2019-01-24T14:15:56Z

starfish/util/logging.py

+
+@lru_cache(maxsize=1)
+def get_git_commit_hash():
+    return subprocess.check_output(["git", "describe", "--always"]).strip()


This will fail if either or both of starfish was installed via pip or user is not int the starfish directory hold true

That is an excellent point

ttung · 2019-01-24T21:44:57Z

starfish/util/logging.py

+def get_core_dependency_info():
+    dependency_info = dict()
+    for dependency in CORE_DEPENDENCIES:
+        ps = Popen(('pip', 'show', dependency), stdout=PIPE)


it is highly likely that pkg_resources is a more portable way of determining this.

ttung · 2019-01-24T21:50:22Z

starfish/util/logging.py

+@lru_cache(maxsize=1)
+def get_git_commit_hash():
+    # First check for git repo
+    if call(["git", "branch"], stderr=STDOUT, stdout=open(os.devnull, 'w')) != 0:


So this breaks down if someone has a different git repo that pip installs starfish. Then we would be picking up the hash of the repo that depends on starfish.

I think what we want is to detect that starfish was installed via pip install -e . (and its conda equivalent), and only in that case do we log the commit hash.

wait you mean only if it wasn't installed with pip or conda then we check for the git hash right?

Ah wait I think I understand what you mean. What do you think is the best way to check for that? Checking is the starfish package location doesn't include site-packages?

So here's my guess, with basically zero time put into proving its feasibility:

get the path of some module in starfish. resolve it to its .py file (https://stackoverflow.com/questions/7162366/get-location-of-the-py-source-file might give you an idea of how to do this, but I would test it out) as opposed to the .pyc / .pyo file.

run git status --porcelain -- 123 $file. this output will be different, depending on whether the file is tracked by a git repo or not. You may need to cd into the directory first though.

Also, I would investigate pkg_resources to see if you can determine how a package was installed.

Having some issues with git status --porcelain but alternate simpler idea; what if we just check if a starfish.py is being tracked by git with git ls-files --error-unmatch starfish.py and if so do a git descibe

Having some issues with git status --porcelain but alternate simpler idea; what if we just check if a starfish.py is being tracked by git with git ls-files --error-unmatch starfish.py and if so do a git descibe

What issues are you having with git status --porcelain?

The --porcelain flag doesn't produce any output. Git status does

ttung · 2019-01-24T21:50:52Z

starfish/util/logging.py

+
+@lru_cache(maxsize=1)
+def get_os_info():
+    return {"Platform": platform.system(), "Version:": platform.version()}


python_version is not os_info, but also worth logging.

ttung

definitely on the right track! I'm curious what issues you have with git status --porcelain. it's supposed to be a very stable API.

ttung · 2019-01-26T01:11:06Z

starfish/util/logging.py

+
+
+@lru_cache(maxsize=1)
+def get_core_dependency_info():


Suggested change

def get_core_dependency_info():

def get_core_dependency_info() -> Mapping[str, str]:

ttung · 2019-01-26T01:11:16Z

starfish/util/logging.py

+
+
+@lru_cache(maxsize=1)
+def get_git_commit_hash():


Suggested change

def get_git_commit_hash():

def get_git_commit_hash() -> str:

ttung · 2019-01-26T01:12:13Z

starfish/util/logging.py

+    # First check if in starfish repo
+    try:
+        check_output(["git", "ls-files", "--error-unmatch", starfish.__file__])
+        return check_output(["git", "describe", "--always"]).strip()


This should not be in the try-block. It would seem that a failure here is indicative of a bug, rather than starfish is not under git tracking.

Are you talking about the git describe or the git ls-files --error-unmatch? Because the second is just list the files under tracking and error if starfish.file is not one of them, isn't that the behavior we want?

I am pretty sure that all you need to do is to execute git ls-files os.path.basename(starfish.__file__), after you chdir to os.path.dirname(starfish.__file__).

git ls-files of a file tracked by git, but not while you are in the directory fails:

[tt]:~> git ls-files microscopy/starfish-git/starfish/__init__.py fatal: not a git repository (or any of the parent directories): .git

but chdir and ls-files and it works:

[tt]:~> cd microscopy/starfish-git/starfish/ [tt]:~/microscopy/starfish-git/starfish:tonytung-pr-684> git ls-files __init__.py __init__.py [tt]:~/microscopy/starfish-git/starfish:tonytung-pr-684>

ttung · 2019-01-26T01:12:27Z

starfish/util/logging.py

+
+
+@lru_cache(maxsize=1)
+def get_os_info():


Suggested change

def get_os_info():

def get_os_info() -> Mapping[str, str]:

starfish/imagestack/imagestack.py

ttung · 2019-01-26T01:15:23Z

starfish/util/logging.py

+
+
+@lru_cache(maxsize=1)
+def get_git_commit_hash():


this method, as written, is subject to strange working directory effects. i suspect you should run the commands in the parent path of starfish.__file__.

starfish.file outputs the file directory path though so does it matter?

ttung · 2019-02-07T23:13:35Z

starfish/types/_constants.py

@@ -34,6 +34,10 @@ class Coordinates(AugmentedEnum):
 """
 This is name of the provenance log attribute stored on the IntensityTable
 """
+CORE_DEPENDENCIES = ['numpy', 'scikit-image', 'pandas', 'scikit-learn', 'scipy', 'xarray', 'sympy']


I would make this a tuple or a set.

ttung · 2019-02-09T05:17:32Z

starfish/util/logging.py

+    # First check if in starfish repo
+    try:
+        check_output(["git", "ls-files", "--error-unmatch", starfish.__file__])
+        return check_output(["git", "describe", "--always"]).strip()


I am pretty sure that all you need to do is to execute git ls-files os.path.basename(starfish.__file__), after you chdir to os.path.dirname(starfish.__file__).

git ls-files of a file tracked by git, but not while you are in the directory fails:

[tt]:~> git ls-files microscopy/starfish-git/starfish/__init__.py fatal: not a git repository (or any of the parent directories): .git

but chdir and ls-files and it works:

[tt]:~> cd microscopy/starfish-git/starfish/ [tt]:~/microscopy/starfish-git/starfish:tonytung-pr-684> git ls-files __init__.py __init__.py [tt]:~/microscopy/starfish-git/starfish:tonytung-pr-684>

ttung · 2019-02-09T05:18:28Z

I also strongly encourage you to add some tests.

joshmoore

Thinking about the testing strategy here, in either docker or just via virtualenvs there should likely be three-ish different install strategies each which call get_git_commit_hash:

python -m starfish ...
pip install -e . && starfish ..
python setup.py sdist && pip install dist/build/starfish... && starfish ...

starfish/util/logging.py

joshmoore · 2019-02-18T07:49:14Z

starfish/util/logging.py

+@lru_cache(maxsize=1)
+def get_git_commit_hash() -> str:
+    # First check if in starfish repo
+    os.chdir(os.path.dirname(starfish.__file__))


You'll likely want a finally along with this chdir returning the user to getcwd otherwise file look ups will fail.

ttung

Thinking about the testing strategy here, in either docker or just via virtualenvs there should likely be three-ish different install strategies each which call get_git_commit_hash:

python -m starfish ...

pip install -e . && starfish ..

python setup.py sdist && pip install dist/build/starfish... && starfish ...

I would expand the matrix a bit further to state that logging has to work regardless of cwd.

Would recommend shoving this into new travis stages. Can run on a lower frequency (only on commits to master? or if @joshmoore builds a release process, we can have tests that only run when we attempt to cut a release?) if it takes too much time.

ttung · 2019-02-23T23:26:20Z

starfish/util/logging.py

+@lru_cache(maxsize=1)
+def get_git_commit_hash() -> str:
+    # First check if in starfish repo
+    os.chdir(os.path.dirname(starfish.__file__))


shanaxel42 · 2019-02-26T00:25:22Z

Thinking about the testing strategy here, in either docker or just via virtualenvs there should likely be three-ish different install strategies each which call get_git_commit_hash:

python -m starfish ...

pip install -e . && starfish ..

python setup.py sdist && pip install dist/build/starfish... && starfish ...

I would expand the matrix a bit further to state that logging has to work regardless of cwd.

Would recommend shoving this into new travis stages. Can run on a lower frequency (only on commits to master? or if @joshmoore builds a release process, we can have tests that only run when we attempt to cut a release?) if it takes too much time.

I don't understand the third example of install? Also this seems like a lot just to test this one command. Creating a virtual env. or docker container and installing starfish three different ways. One of them being getting travis to checkout from git, while already hooked up to run from a branch seems messy. I wanna push back a little and say "is anyone actually asking for this git hash info to be included in the log"? For most users (that installed through pip) it's going to result in a line that says "Starfish project not under git tracking" for every command, which might confuse them.

joshmoore · 2019-02-26T08:46:18Z

I wanna push back a little and say "is anyone actually asking for this git hash info to be included in the log"?

I think this matches my concern: basically getting (and keeping) this right is going to be high cost for low value.

For most users (that installed through pip) it's going to result in a line that says "Starfish project not under git tracking" for every command, which might confuse them.

nods The reverse strategy be to encode the git during build and otherwise UNKNOWN.

ttung · 2019-02-26T09:24:50Z

I wanna push back a little and say "is anyone actually asking for this git hash info to be included in the log"? For most users (that installed through pip) it's going to result in a line that says "Starfish project not under git tracking" for every command, which might confuse them.

Yes, I think @berl asked for it...? My guess is that you are right: people should not be processing data in a permanent way with unreleased versions, and if that happens, we should just assume that all bets are off. Recording the hash is pretty meaningless when people can be mucking around with the source files.

Proposal: record that it's being run from source and not a released version. Probably the best way to do that is to have the release build set a magic variable.

berl · 2019-02-26T17:16:56Z

I mentioned the git hash as a possibility for more fine-grained tracking of the codebase to augment the version number if applicable. @ttung proposal sounds good. Of course no one will need to ever muck about with source files in starfish anyway!

ttung

out of curiosity, can you dump what the log looks like now? might be good to have the community (i.e., #starfish-dev) look it over to see if it covers all the bases.

ttung · 2019-02-28T06:03:59Z

starfish/__init__.py

@@ -22,6 +22,9 @@
 # NOTE: if we move to python 3.7, we can produce this value at call time via __getattr__
 __version__ = pkg_resources.require("starfish")[0].version

+# Variable to be set by release process


I would make it super explicit, e.g., is_released_version or something like that.

also, cc: #761

shanaxel42 · 2019-02-28T16:59:22Z

out of curiosity, can you dump what the log looks like now? might be good to have the community (i.e., #starfish-dev) look it over to see if it covers all the bases.

Example from ISS pipeline:

{'method': 'WhiteTophat', 
'arguments': 
    {'masking_radius': 15, 'is_volume': False}, 
    'os': {'Platform': 'Darwin', 'Version:': 'Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64', 
    'Python Version': '3.6.5'}, 
    'dependencies': {'scikit-learn': '0.20.2', 'pandas': '0.24.1', 'scikit-image': '0.14.2', 'scipy': '1.2.1', 'numpy': '1.16.1', 'sympy': '1.3', 'xarray': '0.11.3'}, 
    'release tag': 'Running starfish from source', 'starfish version': '0.0.33'},
{'method': 'FourierShiftRegistration', 
    'arguments': 
        {'upsampling': 1000, 'reference_stack': '"<starfish.ImageStack (r: 1, c: 1, z: 1, y: 140, x: 200)>"'}, 
        'os': {'Platform': 'Darwin', 'Version:': 'Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64', 'Python Version': '3.6.5'}, 
        'dependencies': {'scikit-learn': '0.20.2', 'pandas': '0.24.1', 'scikit-image': '0.14.2', 'scipy': '1.2.1', 'numpy': '1.16.1', 'sympy': '1.3', 'xarray': '0.11.3'}, 
        'release tag': 'Running starfish from source', 'starfish version': '0.0.33'}, 
{'method': 'BlobDetector', 
    'arguments': 
        {'min_sigma': 1, 
        'max_sigma': 10, 
        'num_sigma': 30, 
        'threshold': 0.01, 
        'overlap': 0.5, 
        'is_volume': True, 
        'measurement_function': '"<function mean at 0x10adbabf8>"', 
        'detector_method': '"<function blob_log at 0x12af9a048>"'}, 
        'os': {'Platform': 'Darwin', 'Version:': 'Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64', 'Python Version': '3.6.5'}, 
        'dependencies': {'scikit-learn': '0.20.2', 'pandas': '0.24.1', 'scikit-image': '0.14.2', 'scipy': '1.2.1', 'numpy': '1.16.1', 'sympy': '1.3', 'xarray': '0.11.3'}, 
        'release tag': 'Running starfish from source', 'starfish version': '0.0.33'}
        ]```

Shannon Axelrod added 3 commits January 22, 2019 15:29

implementation of extra log info

5511ea3

changing format

534867e

changing approach for dependencies

e56debc

shanaxel42 requested a review from ttung January 23, 2019 16:56

joshmoore reviewed Jan 24, 2019

View reviewed changes

checking for git rep first

7850516

ttung requested changes Jan 24, 2019

View reviewed changes

Shannon Axelrod added 2 commits January 25, 2019 11:50

using pkg_resources

5fbd9b4

check for tracked git file

26904fc

shanaxel42 requested a review from ttung January 26, 2019 00:52

ttung requested changes Jan 26, 2019

View reviewed changes

adding starfish version

e4fcc71

shanaxel42 requested a review from ttung January 29, 2019 19:56

ttung requested changes Feb 9, 2019

View reviewed changes

changed git hash function

74bd003

joshmoore requested changes Feb 18, 2019

View reviewed changes

shanaxel42 requested a review from ttung February 20, 2019 17:53

ttung requested changes Feb 23, 2019

View reviewed changes

joshmoore mentioned this pull request Feb 25, 2019

make release target #761

Closed

10 tasks

Shannon Axelrod added 3 commits February 25, 2019 15:55

changes.

492c32b

adding new step

36b4c75

Merge remote-tracking branch 'origin/master' into saxelrod-logging

5283eaf

Shannon Axelrod added 2 commits February 25, 2019 16:36

testing

b33c976

fixing makefile

4e67eac

Shannon Axelrod added 3 commits February 26, 2019 17:09

changing git hash to release tag

24d89a2

fixing notebook weirdness

1e59ad5

changing core dependencies to a set

fa77b2e

shanaxel42 requested a review from ttung February 27, 2019 01:38

ttung approved these changes Feb 28, 2019

View reviewed changes

changing name

e4ffa7e

shanaxel42 merged commit 3904381 into master Feb 28, 2019

shanaxel42 deleted the saxelrod-logging branch February 28, 2019 17:58

	def get_core_dependency_info():
	def get_core_dependency_info() -> Mapping[str, str]:

Add to provenance log #968

Add to provenance log #968

Conversation

shanaxel42 commented Jan 23, 2019

codecov-io commented Jan 23, 2019 • edited Loading

Codecov Report

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

ttung Jan 25, 2019 • 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

Choose a reason for hiding this comment

ttung 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

ttung commented Feb 9, 2019

joshmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanaxel42 commented Feb 26, 2019 • edited Loading

joshmoore commented Feb 26, 2019

ttung commented Feb 26, 2019

berl commented Feb 26, 2019

ttung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanaxel42 commented Feb 28, 2019

codecov-io commented Jan 23, 2019 •

edited

Loading

ttung Jan 25, 2019 •

edited

Loading

shanaxel42 commented Feb 26, 2019 •

edited

Loading