Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git merge conflicts due to python 2 vs 3 #20

Open
KrisThielemans opened this issue Apr 11, 2019 · 20 comments
Open

git merge conflicts due to python 2 vs 3 #20

KrisThielemans opened this issue Apr 11, 2019 · 20 comments
Labels
wontfix This will not be worked on

Comments

@KrisThielemans
Copy link
Member

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
@casperdcl
Copy link
Member

casperdcl commented Apr 11, 2019

I actually submitted a PR to nbstripout to address this ages ago kynan/nbstripout#92 (I say ages - just realised it's only been 2 months)

@KrisThielemans
Copy link
Member Author

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

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

@KrisThielemans
Copy link
Member Author

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

@casperdcl
Copy link
Member

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

@KrisThielemans
Copy link
Member Author

ah. not yet on pip?

@casperdcl
Copy link
Member

nbstripout>=0.3.4

@KrisThielemans
Copy link
Member Author

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

@casperdcl
Copy link
Member

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'

@KrisThielemans
Copy link
Member Author

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...

@casperdcl
Copy link
Member

@paskino FYI I just updated the VM repo

casperdcl added a commit that referenced this issue Apr 11, 2019
@KrisThielemans
Copy link
Member Author

KrisThielemans commented Apr 11, 2019

~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?

@casperdcl
Copy link
Member

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

@casperdcl
Copy link
Member

@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 \{} \;

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Apr 12, 2019

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
Copy link
Member Author

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
Copy link
Member Author

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

My local SIRF-Exercises is at ba34dd4. 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
Copy link
Member Author

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.

@casperdcl
Copy link
Member

casperdcl commented Apr 13, 2019

@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.

@KrisThielemans
Copy link
Member Author

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
Copy link
Member Author

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 KrisThielemans added this to the v2.1 milestone May 10, 2019
KrisThielemans added a commit that referenced this issue May 12, 2019
removed `kernelspec.display_name` and `kernelspec.name` from `nbstripout` recommendation (see #20)
@KrisThielemans KrisThielemans removed this from the v2.1 milestone Jun 5, 2020
@KrisThielemans KrisThielemans added the wontfix This will not be worked on label Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants