Save git info

[zakv]

Proposed changes:

• Support saving git repo information to a shot file, similar to how hg info is saved. The output from following commands are saved:
  • git branch --show-current gives the name of the current branch.
  • git describe --tags --always HEAD gives the name of the most recent tag reachable from the current commit.
    • If the current commit isn't tagged, a suffix showing the number of commits since the most recent tag and a shortened hash for the current commit are shown.
    • The --tags flag makes it look for lightweight tags as well instead of just annotated tags.
    • The --always flag makes it so that If no tag is reachable from the current commit, just the shortened hash is returned.
  • git rev-parse HEAD returns the full hash of the current commit.
  • git diff HEAD module_filename returns a diff between the file on disk and the version in the current commit.
• Allow turning on/off saving of git/hg info with setting in labconfig.
  • These are controlled by the save_hg_info and save_git_info options in the [labscript] section of the labconfig.
  • To avoid changing the existing behavior, save_hg_info defaults to True and save_git_info defaults to False.
• Run the calls to git/hg for a given file in parallel. This is straightforward since they run in separate processes anyway.
  • On my machine, these are the timings to run the git/hg commands on five files during compilation:
    • hg: 1.788 seconds without parallelization and 0.695 seconds with parallelization, a 2.57x speedup.
    • git: 0.707 seconds without parallelization and 0.283 seconds with parallelization, a 2.50x speedup.
• Cache results from calls to git/hg to speed up compilation as suggested in Saving of hg info slows down compilation #38
  • A FileWatcher is used to track when files are modified, then the callback updates the cache immediately.
  • Files are added to the FileWatcher during the first call to save_labscripts() that uses it, so the first shot compilation is a bit slower than others. The cache is updated asynchronously so future calls to save_labscripts are fast even if the file is modified. After the first run, they typically take 20 ms to 40 ms for 6 files on my machine.

Resolves #38.

[zakv]

This partially resolves #38 since running some of the calls to git/hg in parallel reduces the time required to save vcs info. It's certainly not as fast as using ModuleWatcher/FileWatcher though, as suggested in that issue.

Update: commit 0f3f187 implements caching, which resolves #38

[philipstarkey]

I think there are a couple of thread safety issues here:

1. FileWatcher.files is not thread safe. Internally access to the files attribute is always serialised by a threading.Lock(). Technically you could use this lock (it's in the .lock attribute) to access it safely from the labscript code but really the lock, files and folders attributes of the filewatcher should be private. FileWatcher needs a cleanup in this regard (I've logged this as FileWatcher has thread unsafe attributes exposed labscript-utils#68). Rather than checking to see if filepath is in the filewatchers list of files, I would check if it is in _vcs_cache. However, you will also need a lock for this too!

2. Because _vcs_cache is being accessed by multiple threads (filewatcher runs the check in another trhead), you need a threading.Lock object to serialise access. Actually, you probably want a threading.RLock() so you can put it inside _file_watcher_callback as well as using it for the change in 1 (without having to worrying about releasing the lock before calling _file_watcher_callback) and the extraction of results on line #2293.

I was also kind of imaging the cache would be in runmanager, but I think this is better (more self contained). It makes it easy to clear the cache (just restart the runmanager subprocess!). It does mean there would be duplication of git calls if/when we parallelise the runmanager compilation, but that would have been hard to avoid anyway!

[zakv]

I think there are a couple of thread safety issues here:

Yeah I haven't worked very directly with threading in python before so I'm not surprised. Thanks for taking the time to point them out!

2. Because _vcs_cache is being accessed by multiple threads (filewatcher runs the check in another trhead), you need a threading.Lock object to serialise access.

I had thought about that a bit. Doing for command, info, err in _vcs_cache[path] certainly wouldn't be thread save. I ended up convincing myself that results = _vcs_cache[path]; for command, info, err in results would be fine though because _vcs_cache[path] always points to a list no matter where the other thread is in executing _file_watcher_callback(), even partway through its assignment to _vcs_cache[name]. Then results would stay pointed at that list even if the other thread modifies _vcs_cache. I'm quite likely wrong though and I'll need to add a lock to address your point (1) anyway, so I'll get started on that!

[philipstarkey]

I had thought about that a bit. Doing for command, info, err in _vcs_cache[path] certainly wouldn't be thread save. I ended up convincing myself that results = _vcs_cache[path]; for command, info, err in results would be fine though because _vcs_cache[path] always points to a list no matter where the other thread is in executing _file_watcher_callback(), even partway through its assignment to _vcs_cache[name]. Then results would stay pointed at that list even if the other thread modifies _vcs_cache. I'm quite likely wrong though and I'll need to add a lock to address your point (1) anyway, so I'll get started on that!

So I did some reading and you are technically correct in that reading values for existing keys is thread safe in current CPython implementations even if another thread overwrites the value. But it is not guaranteed to stay thread safe in future Python versions and would not be thread safe should our code change in the future (say if a future change meant it was possible that one thread might insert while another was reading). Basically I find it's safer to always use a lock on most datatypes, and the overhead is pretty minimal!

[zakv]

So I did some reading and you are technically correct in that reading values for existing keys is thread safe in current CPython implementations even if another thread overwrites the value. But it is not guaranteed to stay thread safe in future Python versions and would not be thread safe should our code change in the future (say if a future change meant it was possible that one thread might insert while another was reading). Basically I find it's safer to always use a lock on most datatypes, and the overhead is pretty minimal!

Yeah it sounds like even if it works now, it's very prone to breakage whenever something else changes in labscript or elsewhere. Locks are definitely the way to go!

[zakv]

I believe I've got the lock implemented correctly now, though feel free to tell me otherwise

[philipstarkey]

So I've made some further comments. They're minor improvements and only come into play if filewatcher is trying to update the vcs data while a compilation is going on (which probably won't happen very often!). But I figured going into details with these concepts might be useful for you when working on other parts of the labscript suite (where lock contention is more common)

[philipstarkey]

So the lock is now being held for however long it takes to query the subprocess information. You can reduce lock contention by doing something like temp = _run_vcs_commands(name) outside of the lock and then only holding the lock when you place that into the _vcs_cache dictionary.

[zakv]

Believe it or not I actually originally set things up as you're suggesting in both of these comments with Lock in place of RLock, but then changed it. For the lock here in _file_watcher_callback(), if the _file_watcher thread called _file_watcher_callback() then at least some of the cache data is out of date, so it is actually better for it to block the save_labscripts() thread from writing cached results to drive until the cache is updated.

[philipstarkey]

Similarly here, you could do:

with _vcs_cache_rlock:
    condition = path not in _vcs_cache
if condition:
    #add to filewatch, and call callback
with _vcs_cache_rlock:
    for command, info, err in _vcs_cache[path]:

to reduce lock contention. This actually might mean I was wrong about needing the RLock over a plain Lock (but whatever...RLocks are safer in the long run).

[zakv]

When I did it this way I thought I was just making things more complicated than necessary. As soon as the lock is released after the first with statement, it is almost immediately re-acquired either by the explicit callback call or the following with statement.

But now that you brought it up again, I think you're right that this is better. Freeing up the lock even briefly gives the _file_watcher thread an opportunity to run _file_watcher_callback() between those statements so it could potentially update the cache last minute right before last with statement there. I'll make this change.

I also considered reducing lock contention even further by locking just to do file_cache_entry = _vcs_cache[path].copy() then freeing the lock during for loop which is slowed by writing to disk. I think that would work fine as long as nothing in the list returned by _vcs_cache[path] is mutable and edited by both threads. Currently the list only has tuples of strings so that should be ok unless something changes. Alternatively copy.deepcopy() could be used to play it safe. Thoughts?

[zakv]

Commit 4716ceb implements your suggestion, but doesn't move the for loop out of the locked section as mentioned above.

[zakv]

@philipstarkey I got a request for the git features in this PR recently, so I'm thinking of merging it. Would you like to take another look at it beforehand or should I just move forward with it?

I also plan on testing then (presumably) merging #73 too while I'm at it.

@dihm Do you have an interest in taking a look at this or #73? You've been very active in the development lately so I thought I should give you the opportunity to add any input if you're interested

[philipstarkey]

Happy for you to go ahead as you see fit!

[zakv]

I believe all of the comments could be considered resolved now and I've been using the code in this PR for the better part of a year without issue. I'll probably merge this PR sometime in the next few days.

[dihm]

I tested this on top of #73. The speed improvement is pretty significant if you aren't using any versioning! I'm much less experienced with all of the locks but it appears to be OK, especially if Phil says so.

It would be good to update the default profile with the new keys over in labscript_utils once this gets merged.

[zakv]

Dope, glad it works for your setup as well!

Actually the new keys already got added to the default profile in labscript_utils (labscript-suite/labscript-utils#66). That PR got merged when it started looking like this PR would likely get merged at some point

[dihm]

So it is! That's what I get for assuming my installation was up to date.

Good work getting this sorted!