Skip to content

Do not type-check non-python cells in Jupyter Notebooks #1282

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.

Sign up for GitHub

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
aersam wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Do not type-check non-python cells in Jupyter Notebooks #1282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
519da05
try to use %sql
aersam May 13, 2025
b9eec16
prettier
aersam May 13, 2025
5d030f5
adjust test
aersam May 13, 2025
746f65e
empty string
aersam May 13, 2025
a8b2eed
remove console.log
aersam May 13, 2025
1d29fd5
Merge pull request #2 from DetachHead/main
aersam May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/pyright-internal/src/analyzer/sourceFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,15 @@ export class SourceFile {
try {
//TODO: this isnt ideal because it re-reads the file for each cell which is unnecessary
source = getIPythonCells(this.fileSystem, this.getRealUri(), this._console)?.[cellIndex]?.source;
// Make sure we don't return any non-python cells
if (
source &&
['%sql', '%%sql', '%sh', '%pip', '%run', '%fs', '%load', '%matplotlib', '%who', '%env'].includes(
Copy link
Owner

@DetachHead DetachHead May 18, 2025

Choose a reason for hiding this comment

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

i don't think this is the correct spot for this because this method is only called when running basedpyright from the CLI. when using the language server, the file contents get passed to the SourceFile object by writing to this._writableData.clientDocumentContents

Copy link
Author

@aersam aersam May 27, 2025

Choose a reason for hiding this comment

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

Ok, is there a method or so where this filtering would make sense? How do I test the language server?

Copy link
Owner

@DetachHead DetachHead May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this only checks the first line of the file. is %sql only valid on the first line? what if there's a newline or something before the line? if we need more complicated logic for stuff like that, then i think this logic should go in the parser with the rest of the ipython magics support, see

basedpyright/packages/pyright-internal/src/parser/tokenizer.ts

Lines 1240 to 1261 in 8b0f262

private _getIPythonMagicsKind(): 'line' | 'cell' | undefined {
if (!isMagicChar(this._cs.currentChar)) {
return undefined;
}
const prevToken = this._tokens.length > 0 ? this._tokens[this._tokens.length - 1] : undefined;
if (prevToken !== undefined && !isWhitespace(prevToken)) {
return undefined;
}
if (this._cs.nextChar === this._cs.currentChar) {
// Eat up next magic char.
this._cs.moveNext();
return 'cell';
}
return 'line';
function isMagicChar(ch: number) {
return ch === Char.Percent || ch === Char.ExclamationMark;
}
}

is there a way i can run databricks notebooks locally so i can mess around with it myself to learn how they work? it looks like i have to make an account and run it on their site or something?

Copy link
Author

@aersam aersam May 27, 2025

Choose a reason for hiding this comment

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

%sql is valid on the first line of a cell, and this method is called per cell so I think it's ok. Also the test I added is in the 3rd cell.

There is no way of detecting databricks notebooks easily, you can also author them with vs code and it will work just fine. It's really just a jupyter notebook where you see %sql in the first cell. So I do not think we can distinguish between Databricks / Non-Databricks

Copy link
Owner

@DetachHead DetachHead May 18, 2025

Choose a reason for hiding this comment

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

i'm also a bit skeptical about this change because it prevents errors from correctly appearing for users that are just working with regular notebooks. we still want to report an error for this invalid syntax on regular notebooks.

i think if we do this we need a new setting called databricksNotebooks or something

Copy link
Author

@aersam aersam May 27, 2025

Choose a reason for hiding this comment

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

I think a more generic setting allowing to skip cells based on magic would be good

source[0].split(/[\s]/)[0]
)
) {
return '';
}
} catch (e) {
this._console.error(e instanceof Error ? e.message : String(e));
}
Expand Down
5 changes: 4 additions & 1 deletion packages/pyright-internal/src/tests/notebooks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ErrorTrackingNullConsole, typeAnalyzeSampleFiles, validateResultsButBas

test('symbol from previous cell', () => {
const analysisResults = typeAnalyzeSampleFiles(['notebook.ipynb']);
tExpect(analysisResults.length).toStrictEqual(2);
tExpect(analysisResults.length).toStrictEqual(3);
validateResultsButBased(analysisResults[0], {
errors: [],
});
Expand All @@ -18,6 +18,9 @@ test('symbol from previous cell', () => {
},
],
});
validateResultsButBased(analysisResults[2], {
errors: [],
});
});

test('non-python cell', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/pyright-internal/src/tests/samples/notebook.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@
"source": [
"bar: str = foo"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"%sql\n",
"SELECT 1 AS foo, 2 AS bar"
]
}
],
"metadata": {
Expand Down
Loading