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

Applying minor version upgrades to notebook #196

Closed
nicholaswold opened this issue Oct 21, 2020 · 1 comment · Fixed by #205
Closed

Applying minor version upgrades to notebook #196

nicholaswold opened this issue Oct 21, 2020 · 1 comment · Fixed by #205
Assignees

Comments

@nicholaswold
Copy link
Contributor

I ran into this problem while trying to use #189.

Right now, upgrade doesn't actually apply minor version changes to the given notebook (ref: https://github.com/jupyter/nbformat/blob/master/nbformat/v4/convert.py#L67).

I was unable to find a method that could apply minor version updates on existing notebooks without downgrading first (ping @willingc who was on call trying to help me find a method.) writes and reads appear to concern themselves only with major versions, and upgrade_cell is specifically stated to only work with v3 cells and will break v4 cells if provided.

If there is no method to do minor version migrations, then my proposal would be that this block of code should look something like this:

if from_minor == nbformat_minor:
    return

# other versions migration code e.g.
# if from_minor < 3:
# if from_minor < 4:

if from_minor < 5:
    for cell in nb.cells:
        cell.id = random_cell_id()

nb.metadata.orig_nbformat_minor = from_minor
nb.nbformat_minor = nbformat_minor

By doing this, upgrade would be able to apply minor-version patches step-by-step for notebooks within the same major-version.

@willingc willingc mentioned this issue Nov 21, 2020
6 tasks
@MSeal MSeal self-assigned this Jan 6, 2021
@MSeal
Copy link
Contributor

MSeal commented Jan 14, 2021

I had though this had been addressed, causing me to release 5.1.0 -- we should add this fix and do a 5.1.1 release to improve the upgrade to care about minor 4.x versions.

prha added a commit to dagster-io/dagster that referenced this issue Jan 14, 2021
Summary:
jupyter/nbformat#196 does not affect this test, since it just
maintains version behavior for existing notebooks, rather than instantiating new cells.  This
diff just changes the assertion check, by explicitly setting the minor version to 5, and by
using a regex to handle the newly inserted random ids.

Test Plan: ran test

Reviewers: yuhan, max

Reviewed By: yuhan

Differential Revision: https://dagster.phacility.com/D5997
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants