Skip to content

Fix cache outputs retrieval for invalidated notebooks#108

Merged
choldgraf merged 1 commit intomasterfrom
fix-caching
Mar 30, 2020
Merged

Fix cache outputs retrieval for invalidated notebooks#108
choldgraf merged 1 commit intomasterfrom
fix-caching

Conversation

@chrisjsewell
Copy link
Copy Markdown
Member

@chrisjsewell chrisjsewell commented Mar 29, 2020

The previous implementation fails when an existing notebook is changed with failing code.

For example make the MyST-NB documentation, then add raise Exception to a cell in use/basic.ipynb, and re-run:

image

With the new (simplified) code:

image

@AakashGfude is there any reason why you were trying to find the cache record by URI? (something you really shouldn't be trying to do 😬)

The previous implementation fails when an existing notebook is changed with failing code
@choldgraf
Copy link
Copy Markdown
Member

This all seems good to me - I'll hold off on merging in case @AakashGfude had a specific reason for the old pattern

@AakashGfude
Copy link
Copy Markdown
Member

AakashGfude commented Mar 30, 2020

@chrisjsewell Ooh, I was trying to get the list of records with the same URI and getting the latest record from it. Also, probably I thought of calling merge_match _into_notebook , only if there is a cache record, to do error handlings here. But, I see where the error, and dispute can be now. Rule of thumb is to use pk or hashkey.
The code looks good to me as well. Thanks @chrisjsewell

@choldgraf
Copy link
Copy Markdown
Member

Let's merge away then!

@choldgraf choldgraf merged commit 9b3b8ba into master Mar 30, 2020
@chrisjsewell chrisjsewell deleted the fix-caching branch March 30, 2020 14:23
phaustin pushed a commit to phaustin/MyST-NB that referenced this pull request Apr 1, 2020
The previous implementation fails when an existing notebook is changed with failing code
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.

3 participants