SIRF-Exercises icon indicating copy to clipboard operation
SIRF-Exercises copied to clipboard

git merge conflicts due to python 2 vs 3

Open KrisThielemans opened this issue 6 years ago • 20 comments

We use nbstripout to prevent conflicts due to notebook output, and to prevent output to be commited. Sadly, we still have conflicts when a notebook is commited with one Python version but run with another. Example:

$ git diff
--- a/notebooks/MR/interactive/a_fully_sampled.ipynb
+++ b/notebooks/MR/interactive/a_fully_sampled.ipynb
@@ -340,21 +340,21 @@
  ],
  "metadata": {
   "kernelspec": {
-   "display_name": "Python 3",
+   "display_name": "Python 2",
    "language": "python",
-   "name": "python3"
+   "name": "python2"
   },
   "language_info": {
    "codemirror_mode": {
     "name": "ipython",
-    "version": 3
+    "version": 2
    },
    "file_extension": ".py",
    "mimetype": "text/x-python",
    "name": "python",
    "nbconvert_exporter": "python",
-   "pygments_lexer": "ipython3",
-   "version": "3.7.1"
+   "pygments_lexer": "ipython2",
+   "version": "2.7.15rc1"
   }
  },
  "nbformat": 4,

looks harmless but

$ git pull
Updating 2e88c7a..471f7f4
error: Your local changes to the following files would be overwritten by merge:
	notebooks/MR/interactive/a_fully_sampled.ipynb
Please commit your changes or stash them before you merge.
Aborting

KrisThielemans avatar Apr 11 '19 07:04 KrisThielemans

I actually submitted a PR to nbstripout to address this ages ago https://github.com/kynan/nbstripout/pull/92 (I say ages - just realised it's only been 2 months)

casperdcl avatar Apr 11 '19 08:04 casperdcl

ah. too bad they didn't accept it yet.

I guess for now we'll just have to reopen with python2 and recommit.

KrisThielemans avatar Apr 11 '19 08:04 KrisThielemans

any suggestions on how to do that (possibly have to remove nbstripout first?)

KrisThielemans avatar Apr 11 '19 08:04 KrisThielemans

no, they accepted it fine. Sorry i seem to have implied it was still open

casperdcl avatar Apr 11 '19 08:04 casperdcl

ah. not yet on pip?

KrisThielemans avatar Apr 11 '19 08:04 KrisThielemans

nbstripout>=0.3.4

casperdcl avatar Apr 11 '19 08:04 casperdcl

~/.local/bin/nbstripout  --version
0.3.5

KrisThielemans avatar Apr 11 '19 08:04 KrisThielemans

you'll also have to do (at least once):

git config --global filter.nbstripout.extrakeys '
  metadata.celltoolbar metadata.kernel_spec.display_name
  metadata.kernel_spec.name metadata.language_info.codemirror_mode.version
  metadata.language_info.pygments_lexer metadata.language_info.version'

casperdcl avatar Apr 11 '19 08:04 casperdcl

That seems to have worked. thanks!

Could you maybe add this to the instructions here, but also in https://github.com/CCPPETMR/CCPPETMR_VM/blob/master/scripts/UPDATE.sh

or ask for help from someone else...

KrisThielemans avatar Apr 11 '19 08:04 KrisThielemans

@paskino FYI I just updated the VM repo

casperdcl avatar Apr 11 '19 08:04 casperdcl

~casperdcl, the files now after #24 still contain kernelspec still giving conflicts unfortunately. feel like correcting it? Could you also say how we correct it?

Will a git pull after correcting it resolve everything, or do we need a new clone?

KrisThielemans avatar Apr 11 '19 20:04 KrisThielemans

fixed again. I was filtering out kernel_spec rather than kernelspec.

casperdcl avatar Apr 12 '19 16:04 casperdcl

@KrisThielemans in case you want to know:

pip install -U nbstripout
git config --global filter.nbstripout.extrakeys '
  metadata.celltoolbar metadata.kernelspec.display_name
  metadata.kernelspec.name metadata.language_info.codemirror_mode.version
  metadata.language_info.pygments_lexer metadata.language_info.version'
find SIRF-Exercises -name '*.ipynb' -exec nbstripout \{} \;

casperdcl avatar Apr 12 '19 16:04 casperdcl

thanks Casper. So I guess @johannesmayer will have to run this on #25 before merging? (or we could do it for him). (as the git config isn't a hookon github, but one installed locally).

I'm just not 100% sure what this does if you then do a git commit with nbstripout installed. I'd imagine that it doesn't do anything at all then. So my guess is

git config ....
cd SIRF-Exercises
find . -name '*.ipynb' -exec nbstripout \{} \;
nbstripout --uninstall
git commit -a -m "removed some notebook metadata"
nbstripout --install
git push

correct?

KrisThielemans avatar Apr 12 '19 17:04 KrisThielemans

On second thought, I agree that this is uninstall/reinstall is not necessary. As far as I can see, here's how this works:

  1. we do the nbstripout to cut out some fields
  2. we do a commit, which will first do the nbstripout with all the fields configured before passing it on to git

As far as I can see, the first step is only there to make sure that git thinks the file is modified. So now I think that to be 100% safe, you'd need to add extra arguments to the explicit nbstripout line as well (but I don't know how to do that).

KrisThielemans avatar Apr 12 '19 22:04 KrisThielemans

@casperdcl, here's an illustration of my struggles, despite all this.

My local SIRF-Exercises is at ba34dd4af4d0f2e0928bb87e14389b5849364d65. Due to whatever happened before, I have 2 "changed" files

sirfuser@vagrant:~/devel/SIRF-Exercises$ git status
On branch master
Your branch is behind 'origin/master' by 31 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   notebooks/MR/interactive/a_fully_sampled.ipynb
	modified:   notebooks/MR/interactive/d_undersampled_reconstructions.ipynb

no changes added to commit (use "git add" and/or "git commit -a")

However, the changes are "irrelevant":

sirfuser@vagrant:~/devel/SIRF-Exercises$ git diff

(i.e. there's no diff after nbstripout). Nevertheless

sirfuser@vagrant:~/devel/SIRF-Exercises$ git pull
Updating ba34dd4..c13ab98
error: Your local changes to the following files would be overwritten by merge:
	notebooks/MR/interactive/a_fully_sampled.ipynb
	notebooks/MR/interactive/d_undersampled_reconstructions.ipynb
Please commit your changes or stash them before you merge.
Aborting

git reset --hard doesn't help, as the files remain as they were. The only way that I get round this is to junk the repo and clone again.

Any suggestions?

KrisThielemans avatar Apr 12 '19 22:04 KrisThielemans

Also, I suspect that @DANAJK's observation that github no longer displays our notebooks nicely is probably because we stripped a bit too much. That's not a show-stopper, but a bit sad.

KrisThielemans avatar Apr 12 '19 23:04 KrisThielemans

@KrisThielemans

  • sounds like you need nbstripout --uninstall before git reset --hard. Maybe should open an issue on the nbstripout repo related to this?
  • I don't know if github ever would've displayed those images properly regardless of stripping - would be nice to confirm.

casperdcl avatar Apr 13 '19 08:04 casperdcl

I don't know if github ever would've displayed those images properly regardless of stripping - would be nice to confirm.

sorry. ignore my comment. I thought I saw some notebooks displayed in raw format, but it seems fine now.

KrisThielemans avatar Apr 13 '19 08:04 KrisThielemans

summary of status:

  • we need kernelspec.name and .display_name as otherwise jupyter complains about invalid notebook. If a given name doesn't exist on your system, that's ok as jupyter will try to find a kernel based on kernelspec.language (which is python). However, git will then see changes, and if you commit, it'll create trouble.
  • git refusing to merge seem almost unavoidable. Scenario:
    1. checkout and run a notebook, which generates some output
    2. git will think the file has changed, even though git diff says it hasn't
    3. git merge will refuse to do anything.
  • we can git round the git reset --hard problem by adding nbstripout as a smudge filter. No idea what that isn't done by default.

We're therefore still in the case that all commits have to be made via a system that has the same kernelspec. sigh.

KrisThielemans avatar Apr 16 '19 21:04 KrisThielemans