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

Throw a validation error if the worksheet conversion fails #239

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gwincr11
Copy link
Contributor

Motivation:

  • We are seeing a lot of errors in this worksheets process where keys
    are not present that are needed, this pr adds a validation error when
    this process cannot be completed because the key is missing.

Motivation:
  - We are seeing a lot of errors in this worksheets process where keys
    are not present that are needed, this pr adds a validation error when
    this process cannot be completed because the key is missing.
@gwincr11
Copy link
Contributor Author

I could add a test for this, however i am not sure how this relates to real life notebooks. I am happy to add one if someone can help me understand when this is and how it is used. Thank you.

nbformat/v3/rwbase.py Outdated Show resolved Hide resolved
@gwincr11
Copy link
Contributor Author

@MSeal I am curious if you know why these tests are failing. They work for me....

@ivanov
Copy link
Member

ivanov commented Nov 16, 2023

Hey, @gwincr11 - sorry this got dropped on the floor years ago. Since it's been idling here for a while, I'm going to close it.

@ivanov ivanov closed this Nov 16, 2023
@gwincr11
Copy link
Contributor Author

Ok, we still see a few thousand of these errors a month.

@ivanov
Copy link
Member

ivanov commented Nov 16, 2023

@gwincr11 sorry, where is this? I'm confused given what you wrote " i am not sure how this relates to real life notebooks". Can you share more about how v3 notebooks were created and why they are staying at v3 version, and maybe which keys end up missing? I'm not against merging a fix, I just assumed, perhaps wrongly, that the need for this fix has waned below the noise floor.

Reopening, but let's remove the 3.5 compatibility, since that ship has sailed (looks like nbformat these days wants python 3.8)

@ivanov ivanov reopened this Nov 16, 2023
@gwincr11
Copy link
Contributor Author

@gwincr11 sorry, where is this? I'm confused given what you wrote " i am not sure how this relates to real life notebooks". Can you share more about how v3 notebooks were created and why they are staying at v3 version, and maybe which keys end up missing? I'm not against merging a fix, I just assumed, perhaps wrongly, that the need for this fix has waned below the noise floor.

So these are errors we have seen on GitHub's notebook renderer. I do not know what is source is producing the notebooks, but can say we see them happen frequently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants