Skip to content

Add notebook.workingDirectory setting #8558

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

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

Conversation

austin3dickey
Copy link
Contributor

@austin3dickey austin3dickey commented Jul 16, 2025

Addresses #7988. Adds a notebook.workingDirectory setting and hides the jupyter.notebookFileRoot setting, which doesn't work in Positron.

Release Notes

New Features

Bug Fixes

  • N/A

QA Notes

@:notebooks @:vscode-settings @:console @:extensions @:interpreter @:win

I added some unit tests but no e2e tests.

Some things to try:

  • The jupyter.notebookFileRoot setting should no longer exist
  • Open a workspace, and a new notebook in a subdirectory of that workspace, with a cell containing %pwd, and see its value for different values of the new setting (note you need to close/open the notebook for the change to take effect):
    • (unset)
    • ${workspaceFolder}
    • ${fileDirname}
    • ${userHome}
    • /some/path/that/exists
    • /some/path/that/doesnt/exist
    • whatever else you want to try (variables are defined here)
  • The Console and Terminal working directories shouldn't change
  • Filename completions should correspond to the correct working directory
  • This should work for R (getwd()) or Python (os.getcwd()) notebooks, and for VSC or Positron notebooks

Copy link

github-actions bot commented Jul 16, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:notebooks @:vscode-settings @:console @:extensions @:interpreter @:win

readme  valid tags

Copilot

This comment was marked as outdated.

Copilot

This comment was marked as resolved.

@austin3dickey austin3dickey force-pushed the aus/nb_pwd_2 branch 2 times, most recently from 723280c to a838fb7 Compare July 23, 2025 18:31
@austin3dickey austin3dickey requested a review from Copilot July 23, 2025 19:28
Copilot

This comment was marked as resolved.

@austin3dickey austin3dickey changed the title WIP: add notebook.workingDirectory setting Add notebook.workingDirectory setting Jul 24, 2025
@austin3dickey austin3dickey marked this pull request as ready for review July 24, 2025 17:42
nstrayer
nstrayer previously approved these changes Jul 24, 2025
Copy link
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some good stuff. The tests are great and you even remembered to update the dates in the headers! (2025/2025 points).

Some comments on the config setting itself.

Comment on lines 1318 to 1324
},
// --- Start Positron ---
[NotebookSetting.workingDirectory]: {
markdownDescription: nls.localize('notebook.workingDirectory', "Default working directory for notebook kernels. Supports variables like `${workspaceFolder}`. When empty, uses the notebook file's directory. Any change to this setting will apply to future opened notebooks."),
type: 'string',
default: '',
scope: ConfigurationScope.RESOURCE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only thought is it may get confusion when we're all-in on positron notebooks that this setting is in the original vscode notebooks contrib. I'm not sure we will respect every one of the original vscode notebook config options. I don't think it should go in the positron notebooks one though... So this is probably good for now and then maybe in the future we will split out to a new Switzerland positron(the editor)-specific-notebook-configs section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, makes sense! We probably will still at least want to use the notebooks. prefix right? (I put the setting here because I thought it should start with that prefix since it only applies to notebooks)

seeM
seeM previously approved these changes Jul 25, 2025
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! Works as expected when changing the setting to ${workspaceFolder} as well as ${workspaceFolder}/mypkg where mypkg exists, in both R and Python.

When I set it to a nonexistent folder like ${workspaceFolder}/mypkg2 or a bad template like ${workspaceFolder2} it seemed to default to ${workspaceFolder}, although I don't see where that's handled. Should we handle and/or test those cases?

@austin3dickey
Copy link
Contributor Author

When I set it to a nonexistent folder like ${workspaceFolder}/mypkg2 or a bad template like ${workspaceFolder2} it seemed to default to ${workspaceFolder}, although I don't see where that's handled. Should we handle and/or test those cases?

Oh nice catch, I thought I tried this but that's different behavior from what I'd expect (I think I'd expect it to fall back to the parent dir). Turns out this is happening in kallichore. If the directory doesn't exist, it defaults to the pwd, which is the workspace root.

Maybe I'll verify that the directory exists at the end of resolveWorkingDirectory() so that if it doesn't we can fall back to the old behavior, the parent dir.

@austin3dickey
Copy link
Contributor Author

Okay, I just moved the default behavior of "if the setting doesn't resolve to an existing directory, use the notebook's parent" up the stack to when we're creating the RuntimeSession metadata (in resolveNotebookWorkingDirectory()). That way we can delete the code that handles that default in KallichoreSession and the Jedi implementation. This fixes the bug @seeM was seeing.

@austin3dickey
Copy link
Contributor Author

I'll have to fix the Python tests and add an e2e test.

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