Simplify dev_tools.notebooks.utils.rewrite_notebook#6030
Simplify dev_tools.notebooks.utils.rewrite_notebook#6030pavoljuhas merged 8 commits intoquantumlib:masterfrom
Conversation
Do not return the OS file descriptor, it was only used to close the file in rewrite_notebook callers. Return only the filename of the rewritten notebook.
pavoljuhas
added
the
kind/health
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
LGTM once temp file leak is fixed.
| notebook_file = os.path.basename(notebook_path) | ||
|
|
||
| rewritten_notebook_descriptor, rewritten_notebook_path = rewrite_notebook(notebook_path) | ||
| rewritten_notebook_path = rewrite_notebook(notebook_path) |
There was a problem hiding this comment.
It seems we are (and have always been) leaking temporary files. Could you fix while you're at it?
There was a problem hiding this comment.
@viathor - thank you for the review. The temporary files cleanup was a bit more involved - can you please take a quick look again?
| notebook_rel_dir = os.path.dirname(os.path.relpath(notebook_path, ".")) | ||
| out_path = f"out/{notebook_rel_dir}/{notebook_file[:-6]}.out.ipynb" | ||
| rewritten_notebook_descriptor, rewritten_notebook_path = rewrite_notebook(notebook_path) | ||
| rewritten_notebook_path = rewrite_notebook(notebook_path) |
Enable xfail-marked test that verify temporary notebooks are cleaned up after rewrite_notebook.
Prevent lingering temporary file.
viathor
left a comment
viathor
left a comment
There was a problem hiding this comment.
Thank you for going the extra mile here and fixing the pre-existing leak. I have a suggestion to make the fix a little more robust.
| notebook_path.endswith('-rewrite.ipynb') | ||
| and os.path.isfile(notebook_path) | ||
| and os.path.dirname(notebook_path) == tempfile.gettempdir() | ||
| ): |
There was a problem hiding this comment.
This implements the pattern wherein we make a decision (here: whether or not we need to rewrite a notebook into a temporary file) and then later try to work out what decision was taken to correlate earlier and later actions. This coding pattern is brittle since it's easy for the code to evolve in a way that breaks perfect correlation between the two decision points.
One way to solve this is to return a bool from rewrite_notebook indicating whether the file is temporary or not. This way we can ensure that the file is deleted if and only if it is temporary while also ensuring that the decision is made once.
If you really dislike two return values you could try to hide the bool in the type by e.g. returning an instance of tempfile.NamedTemporaryFile or a file object (they both have .name attribute I think...). This is fine too as long as it can be made to work (haven't tried).
Not needed if rewrite_notebook always produces temporary files. Just use os.remove to clean up after rewrite_notebook.
3 participants
Do not return the OS file descriptor as it was only used for
closing that file in
rewrite_notebookcallers.Return only the filename of the rewritten notebook.