search

ReviewNB / support

Issues and feature requests for ReviewNB
https://reviewnb.com
58 stars 8 forks source link

ReviewNB shows entire notebook has having changed, rather than showing cell-by-cell comparisons #76

Closed MarcoGorelli closed 3 years ago

MarcoGorelli commented 3 years ago

Here's the PR - https://github.com/pymc-devs/pymc3/pull/4215

Rather than showing something like

image

it is instead showing

image

and

image

Any ideas why?

For the second pair of screenshots, I ran the notebook on Kaggle - does any of the metadata they add make it look like all the cells are completely different?

MarcoGorelli commented 3 years ago

Seems that removing papermill from the metadata does the trick

amit1rrr commented 3 years ago

Thanks for reporting. Looks like metadata created by papermill is not being handled well. We're going to dig some deeper and see if we can fix this behaviour.

MarcoGorelli commented 3 years ago

Cool, thanks! To be clear, I just needed to remove papermill from the notebook's metadata, not from each cell's metadata

cfoisy-osisoft commented 3 years ago

We face the same problem but we have no papermill metadata, only long outputs which may throw a wrench in the diff. Do hiding cell outputs within ReviewNB performs a new diff without considering them? (I suspect not). Our PR at https://app.reviewnb.com/cfoisy-osisoft/OSI-Samples-OCS/pull/5/files/

amit1rrr commented 3 years ago

@MarcoGorelli We've deployed a fix to handle notebook level metadata more carefully. You shouldn't see any issues due to papermill metadata going forward. The fix can be seen on this commit: https://app.reviewnb.com/pymc-devs/pymc3/commit/cf435e1560f81c5e3d61d0666c3bbd7fbe12290b/

@cfoisy-osisoft If there are a lot of cell level changes (cells being split, merged, deleted, added) in a given PR/commit then our algorithm might not be able to compute line by line diff. In such cases we resort to simple diff which is what has happened in your case.

Do hiding cell outputs within ReviewNB performs a new diff without considering them? (I suspect not).

No. "Hide Output" is just a UI switch to hide outputs, it doesn't recompute the diff.