Save as improvements #267

andrewfulton9 · 2023-11-10T17:42:32Z

Fixes for #266

Adds functionality for inferring new notebook name from current notebook and creating directories from "save as".

No backwards incompatibilities as far as I am aware.

nbclassic_save_as_fixes.mp4

andrewfulton9 · 2023-11-10T17:48:07Z

I am running into issues running tests both locally and in CI on my fork that I am unsure how to fix. I am unsure if they will replicate in the CI here.

RRosio · 2023-12-04T20:24:09Z

Thank you for submitting this PR @andrewfulton9, the new CI failures that are present seem to be the Linux JS Tests which I believe could be failing due to a change in the JS dependencies. I am trying to pinpoint which dependency could be the . Locally, I also see the test_save_readonly_as.py failing, but this test fails locally in latest main branch as well and iirc is also the reason for the Playwright test failures in the latest main branch CI run.

I am wondering, after trying some different paths, using the new function, makeParentDirectory, how can we inform users when the path they are attempting to save the notebook to is not a valid path? From my local testing, at one point when I entered a path, I was returned to the prompt for the path after clicking the create button, but saw no message in the UI.

andrewfulton9 · 2023-12-04T22:00:04Z

Thanks for taking a look at this @RRosio!

Thank you for submitting this PR @andrewfulton9, the new CI failures that are present seem to be the Linux JS Tests which I believe could be failing due to a change in the JS dependencies. I am trying to pinpoint which dependency could be the . Locally, I also see the test_save_readonly_as.py failing, but this test fails locally in latest main branch as well and iirc is also the reason for the Playwright test failures in the latest main branch CI run.

I agree that this seems to be an issue with JS dependencies. I have been trying to figure out a fix, but have yet to find one. I was able to get the tests to go through CI by hard pinning the all the versions, but oddly this isn't working for me locally. Once I get that figured out I will fix and update test_save_readonly_as.py to handle testing the cases, but I'll admit I'm pretty stuck on this dependency issue. What would the next step be for the dependency issue? Should another issue be opened to specifically address that?

I am wondering, after trying some different paths, using the new function, makeParentDirectory, how can we inform users when the path they are attempting to save the notebook to is not a valid path? From my local testing, at one point when I entered a path, I was returned to the prompt for the path after clicking the create button, but saw no message in the UI.

Good catch. So if an invalid path is given inmakeParentDirectory, it's currently just failing silently. Am I understanding that correctly?

RRosio · 2023-12-04T22:47:31Z

I was able to get the tests to go through CI by hard pinning the all the versions, but oddly this isn't working for me locally.

Have you by any chance cleared the bower cache located in ~/.cache/bower? I remember experiencing issues locally due to some of the package versions cached in this location. I can give it a try locally as well!

Should another issue be opened to specifically address that?

That is a good idea, I went ahead and opened #268.

So if an invalid path is given inmakeParentDirectory, it's currently just failing silently. Am I understanding that correctly?

Yes that is correct!

RRosio · 2024-01-19T04:19:22Z

Closing and reopening to rerun CI checks

RRosio · 2024-01-19T04:20:04Z

Closing and reopening to rerun CI checks
Pressed the wrong button.

krassowski · 2024-05-09T09:22:54Z

@andrewfulton9 it looks like the only failure in Playwright tests is in test_save_as_nb so that sounds related to the changes in this PR (FYI on main branch all tests are passing with green status):

2024-05-08T19:11:01.0775134Z =================================== FAILURES ===================================
2024-05-08T19:11:01.0776443Z _______________________________ test_save_as_nb ________________________________
2024-05-08T19:11:01.0777452Z Traceback (most recent call last):
2024-05-08T19:11:01.0779217Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 341, in from_call
2024-05-08T19:11:01.0780068Z     result: Optional[TResult] = func()
2024-05-08T19:11:01.0781011Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 262, in <lambda>
2024-05-08T19:11:01.0781893Z     lambda: ihook(item=item, **kwds), when=when, reraise=reraise
2024-05-08T19:11:01.0782907Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_hooks.py", line 433, in __call__
2024-05-08T19:11:01.0783823Z     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
2024-05-08T19:11:01.0784894Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_manager.py", line 112, in _hookexec
2024-05-08T19:11:01.0785814Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-05-08T19:11:01.0787192Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 155, in _multicall
2024-05-08T19:11:01.0787980Z     return outcome.get_result()
2024-05-08T19:11:01.0788874Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_result.py", line 108, in get_result
2024-05-08T19:11:01.0789704Z     raise exc.with_traceback(exc.__traceback__)
2024-05-08T19:11:01.0790591Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 80, in _multicall
2024-05-08T19:11:01.0791292Z     res = hook_impl.function(*args)
2024-05-08T19:11:01.0792155Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 177, in pytest_runtest_call
2024-05-08T19:11:01.0793229Z     raise e
2024-05-08T19:11:01.0794077Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/runner.py", line 169, in pytest_runtest_call
2024-05-08T19:11:01.0794788Z     item.runtest()
2024-05-08T19:11:01.0795531Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/_pytest/python.py", line 1792, in runtest
2024-05-08T19:11:01.0796272Z     self.ihook.pytest_pyfunc_call(pyfuncitem=self)
2024-05-08T19:11:01.0797312Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_hooks.py", line 433, in __call__
2024-05-08T19:11:01.0798142Z     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
2024-05-08T19:11:01.0799112Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_manager.py", line 112, in _hookexec
2024-05-08T19:11:01.0799949Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2024-05-08T19:11:01.0800903Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 116, in _multicall
2024-05-08T19:11:01.0801715Z     raise exception.with_traceback(exception.__traceback__)
2024-05-08T19:11:01.0802610Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pluggy/_callers.py", line 80, in _multicall
2024-05-08T19:11:01.0803293Z     res = hook_impl.function(*args)
2024-05-08T19:11:01.0804192Z   File "/opt/hostedtoolcache/Python/3.7.17/x64/lib/python3.7/site-packages/pytest_tornasync/plugin.py", line 45, in pytest_pyfunc_call
2024-05-08T19:11:01.0804961Z     pyfuncitem.obj(**testargs)
2024-05-08T19:11:01.0805757Z   File "/home/runner/work/nbclassic/nbclassic/nbclassic/tests/end_to_end/test_save_as_notebook.py", line 115, in test_save_as_nb
2024-05-08T19:11:01.0806737Z     notebook_frontend.wait_for_condition(attempt_form_fill_and_save, timeout=900, period=1)
2024-05-08T19:11:01.0807692Z   File "/home/runner/work/nbclassic/nbclassic/nbclassic/tests/end_to_end/utils.py", line 694, in wait_for_condition
2024-05-08T19:11:01.0808404Z     raise EndToEndTimeout()
2024-05-08T19:11:01.0808791Z nbclassic.tests.end_to_end.utils.EndToEndTimeout
2024-05-08T19:11:01.0809285Z ==================================== PASSES ====================================

andrewfulton9 · 2024-05-10T20:30:24Z

Thanks for the heads up @krassowski, I should have some time next week to look at this.

RRosio · 2024-05-24T19:31:35Z

After re-running the CI Playwright Tests, one of the three failures passed but two others are still failing. To me it seems that these failures may not actually be related to the PR itself. However it has been difficult to consistently reproduce these failures locally, where the entire test suite passes with no notable issues.

RRosio · 2024-05-24T19:50:48Z

I opened #277 after some local runs of the tests, and I see test failures in #277 in a different test (test_display_isolation.py) to what I modified, but test_save_as_notebook.py is passing there. I am not really confident that the CI is reliably indicating issues.

andrewfulton9 · 2024-11-07T21:46:56Z

@RRosio, I finally got a chance to come back to this. I added error handling and tests to this PR and they are are passing locally. I am still getting some CI failures on my branch, but as far as I can tell they aren't related.

andrewfulton9 · 2024-11-11T23:16:06Z

It looks like some tests are still failing, but I am not able to reproduce locally.

peytondmurray · 2025-04-16T20:42:15Z

Looks like #329 should solve the dependency issues here; I'm reverting my changes.

krassowski · 2025-04-17T14:28:52Z

nbclassic/tests/end_to_end/test_save_as_notebook.py


-    def attempt_form_fill_and_save():
+    def attempt_form_fill_and_save(notebook_path):


Line 111 needs partial as in line 192:

TypeError: attempt_form_fill_and_save() missing 1 required positional argument: 'notebook_path'

Hmm, I think attempt_form_fill_w_dir_and_save was never actually called in this test. Let me fix that now...

danyeaw · 2025-04-18T11:13:23Z

Hi @andrewfulton9 we are getting ready to cut a new release and it would be awesome to get this in. Do you want to rebase your changes? That might fix some of the test failures on CI.

peytondmurray · 2025-04-18T16:10:03Z

Hi @danyeaw, after a discussion with @andrewfulton9 I'm shepherding this one through review, so I'm happy to rebase this. Thanks again for the dependency updates!

krassowski · 2025-04-19T19:19:12Z

nbclassic/static/notebook/js/notebook.js

@@ -2866,6 +2866,7 @@ define([

    Notebook.prototype.save_notebook_as = function() {
        var that = this;
+        var current_notebook_name = $('body').attr('data-notebook-name')


Since tests are passing on main but not here, I suspect the code might have some issues. While I do not see it straightaway, I would add semicolons, both because lack of these can lead to unexpected evaluation in JS, and to match the style in surrounding code.

Arrgh, I can't get the tests to pass locally on main, so I can't really begin to debug this branch locally either.

git clean -fdx; npm run build; OPENSSL_CONF=/etc/ssl python -m nbclassic.jstest base

On main, I still get the same issue, Error adding link: TypeError: undefined is not a function (evaluating 'Number.isFinite(num)').

I'll start by cleaning this up a bit and see if we can't get a better idea of the real problem.

I'm not exactly sure what's going on here, but it seems like there are some instances in the code base here where ES6 functions are being called, like Math.log10 and Math.isFinite. When run on a version of node that doesn't support these, we get the JS errors we've been seeing in the tests. Funny thing is we haven't touched the dependencies here, so I have no idea why this branch is behaving any different in places of the code that weren't touched 🤔

No idea why it manifested this way, but there was a problematic arrow function buried in the click handler for the modal that prompts the user when generating the missing parent directories. Seems like tests are passing now!

Copilot

Pull Request Overview

This PR improves the "Save as" functionality for notebooks by adding the ability to infer the new notebook name from the current one and to create missing directories when needed. Key changes include updating end-to-end tests to pass the notebook path dynamically, modifying UI behavior in the save-as dialog, and introducing recursive directory creation.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
nbclassic/tests/end_to_end/test_save_as_notebook.py	Updated tests to dynamically supply notebook names and test saving notebooks in new directories with adjusted timeouts.
nbclassic/static/notebook/js/notebook.js	Modified save-as logic to infer notebook name and added recursive directory creation when the target directory does not exist.

remove problematic arrow function

nbclassic/tests/end_to_end/test_save_as_notebook.py

peytondmurray · 2025-04-22T20:21:36Z

Okay, I think some of the end to end tests were failing because the frontend doesn't have time to respond when playwright checks for the presence/absence of certain DOM elements. I've added a few waits here and there, and that seems to have resolved the issues on linux.

However test_save_as_notebook was being stubborn on MacOS, so I've made pytest skip it on that platform. There is already precedence for this: test_save_readonly_as is already skipped for that reason on MacOS, and the test structure is very similar.

peytondmurray · 2025-04-23T05:05:42Z

Thanks for retriggering tests - I think the end-to-end tests are just flaky, and in adding another form we're potentially exacerbating the underlying flakiness. Not sure what the best course of action is; looks like it must have gotten stuck here, based on the log: https://github.com/andrewfulton9/nbclassic/blob/2680b1dc1daad44c4021edbb8b19b0aefc1e9067/nbclassic/tests/end_to_end/test_save_as_notebook.py#L123

On the other hand, these tests seem to be able to pass at least intermittently: https://github.com/peytondmurray/nbclassic/actions/runs/14603851073/job/40968241964?pr=1. Maybe the sleep interval after each click isn't large enough on the small github runners that the tests are running on? I'll bump that now - if anyone has other suggestions I'd be grateful for them!

EDIT: Okay, I'm still getting flaky tests on stuff that is unrelated as well, see https://github.com/peytondmurray/nbclassic/actions/runs/14610590297/job/40987766926?pr=1.

RRosio · 2025-04-23T18:07:38Z

Thank you @peytondmurray and @krassowski for all your work here! I just re-ran the Playwright tests and they are all passing. It seems that there may still be some lingering flakiness in the tests. Thanks again, I will go ahead and merge this!

RRosio added the enhancement New feature or request label Nov 29, 2023

krassowski mentioned this pull request Dec 19, 2023

Fix Linux JS Test CI job #269

Merged

RRosio closed this Jan 19, 2024

RRosio reopened this Jan 19, 2024

peytondmurray mentioned this pull request Apr 16, 2025

Update JavaScript dependencies #329

Merged

peytondmurray force-pushed the save_as_improvements branch from 0500f31 to 78d4de9 Compare April 16, 2025 21:42

krassowski reviewed Apr 17, 2025

View reviewed changes

peytondmurray force-pushed the save_as_improvements branch from d50e480 to 5816f8f Compare April 18, 2025 16:10

krassowski reviewed Apr 19, 2025

View reviewed changes

krassowski requested a review from Copilot April 19, 2025 19:19

Copilot AI reviewed Apr 19, 2025

View reviewed changes

andrewfulton9 added 4 commits April 21, 2025 21:41

infers save path and creates new directories

d3f270f

update html body attributes

f05cd13

cleanup

d0ece3a

remove whitespace

5d5b5cd

andrewfulton9 and others added 7 commits April 21, 2025 21:41

error handling and testing

fee1da2

update save_as_notebook test

7051f54

fix formatting

d958495

formatting

be03047

update test

147ff43

Fix lint errors

8c20d0b

Fix usage of test function; reduce timeout from 15min -> 2min;

c134476

remove problematic arrow function

peytondmurray force-pushed the save_as_improvements branch from c3c4a44 to c134476 Compare April 22, 2025 05:02

Fix playwright tests

8be4051

krassowski reviewed Apr 22, 2025

View reviewed changes

nbclassic/tests/end_to_end/test_save_as_notebook.py Outdated Show resolved Hide resolved

krassowski and others added 2 commits April 22, 2025 10:07

Cleanup show_screenshot and breakpoints

8e6d435

Add a few waits to allow the frontend to respond in end-to-end tests

7569f10

peytondmurray force-pushed the save_as_improvements branch from c8deb54 to 7569f10 Compare April 22, 2025 19:59

Skip test_save_as_notebook on darwin

2680b1d

Bump the e2e post-click sleep to 3s

cf6d530

danyeaw mentioned this pull request Apr 23, 2025

Weekly Team Meetings: Jan–Jun 2025 jupyterlab/frontends-team-compass#266

Open

RRosio merged commit 953f5e8 into jupyter:main Apr 23, 2025
38 checks passed


		def attempt_form_fill_and_save():
		def attempt_form_fill_and_save(notebook_path):

Save as improvements #267

Save as improvements #267

Uh oh!

Conversation

andrewfulton9 commented Nov 10, 2023

Uh oh!

andrewfulton9 commented Nov 10, 2023

Uh oh!

RRosio commented Dec 4, 2023

Uh oh!

andrewfulton9 commented Dec 4, 2023

Uh oh!

RRosio commented Dec 4, 2023

Uh oh!

RRosio commented Jan 19, 2024

Uh oh!

RRosio commented Jan 19, 2024

Uh oh!

krassowski commented May 9, 2024

Uh oh!

andrewfulton9 commented May 10, 2024

Uh oh!

RRosio commented May 24, 2024

Uh oh!

RRosio commented May 24, 2024

Uh oh!

andrewfulton9 commented Nov 7, 2024

Uh oh!

andrewfulton9 commented Nov 11, 2024

Uh oh!

peytondmurray commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krassowski Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

peytondmurray Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

danyeaw commented Apr 18, 2025

Uh oh!

peytondmurray commented Apr 18, 2025

Uh oh!

krassowski Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

peytondmurray Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

peytondmurray Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

peytondmurray Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

peytondmurray commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peytondmurray commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RRosio commented Apr 23, 2025

Uh oh!

Uh oh!

Uh oh!

peytondmurray commented Apr 16, 2025 •

edited

Loading

peytondmurray commented Apr 22, 2025 •

edited

Loading

peytondmurray commented Apr 23, 2025 •

edited

Loading