Skip to content
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

Jump to bottom

Add linting support to notebooks (Closes #1277) #1313

Merged
merged 5 commits into from
Apr 23, 2024
Merged

Add linting support to notebooks (Closes #1277) #1313

merged 5 commits into from
Apr 23, 2024

Conversation

orpuente-MS
Copy link
Contributor

This PR adds a few lines of code when collecting errors from a notebook cell to also include lints.

@github-actions GitHub Actions
Copy link

Benchmark for 8fd482a

Click to view benchmark
Test Base PR %
Array append evaluation 357.7±2.65µs 343.7±2.40µs -3.91%
Array literal evaluation 192.0±0.48µs 173.8±2.35µs -9.48%
Array update evaluation 441.2±4.27µs 426.4±2.27µs -3.35%
Deutsch-Jozsa evaluation 5.2±0.05ms 5.2±0.05ms 0.00%
Large file parity evaluation 33.6±0.08ms 33.6±0.32ms 0.00%
Large input file 29.5±0.57ms 30.1±1.07ms +2.03%
Large nested iteration 34.7±0.23ms 33.5±0.42ms -3.46%
Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample 1631.2±126.61µs 1579.4±76.85µs -3.18%
Perform Runtime Capabilities Analysis (RCA) on large file sample 7.8±0.10ms 7.7±0.09ms -1.28%
Perform Runtime Capabilities Analysis (RCA) on teleport sample 1519.8±102.47µs 1479.1±61.65µs -2.68%
Perform Runtime Capabilities Analysis (RCA) on the core and std libraries 21.4±0.16ms 21.4±0.21ms 0.00%
Standard library 16.5±0.07ms 16.6±0.46ms +0.61%
Teleport evaluation 83.5±4.26µs 83.4±5.38µs -0.12%

@orpuente-MS orpuente-MS changed the title Add linting support to notebooks Add linting support to notebooks (Closes #1277) Mar 26, 2024
@orpuente-MS orpuente-MS linked an issue Mar 26, 2024 that may be closed by this pull request
Add linting support for notebooks after Linter is merged. #1277
Closed
@minestarks
Copy link
Member

In VS Code, have you tested end-to-end that the qsharp.json lint settings are picked up by the language service ?

In a Python notebook, if you execute qsharp.init(project_root=some_dir) , the squiggles in the cells should get updated with the qsharp.json for that project. (See GIF in this PR to see how this works for the Target Profile setting #863)

@github-actions GitHub Actions
Copy link

Benchmark for edbd029

Click to view benchmark
Test Base PR %
Array append evaluation 355.0±2.59µs 348.9±16.01µs -1.72%
Array literal evaluation 193.7±2.86µs 173.9±1.00µs -10.22%
Array update evaluation 441.3±23.05µs 428.2±2.49µs -2.97%
Deutsch-Jozsa evaluation 5.2±0.07ms 5.2±0.06ms 0.00%
Large file parity evaluation 33.7±0.12ms 33.6±0.33ms -0.30%
Large input file 29.8±1.50ms 34.3±0.97ms +15.10%
Large nested iteration 34.6±0.21ms 33.6±0.32ms -2.89%
Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample 1626.8±90.68µs 1620.6±148.83µs -0.38%
Perform Runtime Capabilities Analysis (RCA) on large file sample 7.8±0.08ms 8.0±0.16ms +2.56%
Perform Runtime Capabilities Analysis (RCA) on teleport sample 1516.1±87.23µs 1519.6±57.83µs +0.23%
Perform Runtime Capabilities Analysis (RCA) on the core and std libraries 21.6±0.32ms 21.7±0.41ms +0.46%
Standard library 16.9±1.16ms 18.8±0.99ms +11.24%
Teleport evaluation 85.6±7.82µs 84.3±5.84µs -1.52%

@orpuente-MS
Copy link
Contributor Author

In VS Code, have you tested end-to-end that the qsharp.json lint settings are picked up by the language service ?

In a Python notebook, if you execute qsharp.init(project_root=some_dir) , the squiggles in the cells should get updated with the qsharp.json for that project. (See GIF in this PR to see how this works for the Target Profile setting #863)

I see. I just realized there is a Python interface under pip/qsharp. I guess I will have to play with that.

@orpuente-MS
Copy link
Contributor Author

In VS Code, have you tested end-to-end that the qsharp.json lint settings are picked up by the language service ?

In a Python notebook, if you execute qsharp.init(project_root=some_dir) , the squiggles in the cells should get updated with the qsharp.json for that project. (See GIF in this PR to see how this works for the Target Profile setting #863)

As you suspected this was not working. It should work after merging #1330.

@orpuente-MS orpuente-MS mentioned this pull request Apr 4, 2024
Merged
minestarks
minestarks approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@minestarks minestarks left a comment

Choose a reason for hiding this comment

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

Looks good assuming the one comment is addressed.

I'll approve the unblock the PR, but the end to end scenario won't work until the PR you linked (plumbing the manifest into updateNotebookMetadata) is merged.

language_service/src/compilation.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 8, 2024

Benchmark for db957fa

Click to view benchmark
Test Base PR %
Array append evaluation 338.6±4.06µs 341.3±3.25µs +0.80%
Array literal evaluation 179.2±4.38µs 175.3±3.55µs -2.18%
Array update evaluation 421.1±3.51µs 420.8±2.74µs -0.07%
Core + Standard library compilation 17.2±0.36ms 17.4±0.41ms +1.16%
Deutsch-Jozsa evaluation 5.1±0.05ms 5.1±0.05ms 0.00%
Large file parity evaluation 33.8±0.12ms 33.6±0.11ms -0.59%
Large input file compilation 11.7±0.14ms 11.8±0.49ms +0.85%
Large input file compilation (interpreter) 46.5±1.05ms 46.2±0.93ms -0.65%
Large nested iteration 33.0±0.33ms 33.1±0.24ms +0.30%
Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample 1497.4±58.12µs 1495.7±90.68µs -0.11%
Perform Runtime Capabilities Analysis (RCA) on large file sample 7.8±0.08ms 7.7±0.10ms -1.28%
Perform Runtime Capabilities Analysis (RCA) on teleport sample 1405.8±37.87µs 1415.1±130.30µs +0.66%
Perform Runtime Capabilities Analysis (RCA) on the core and std libraries 21.6±0.27ms 21.5±0.19ms -0.46%
Teleport evaluation 81.2±3.53µs 82.2±5.34µs +1.23%

github-merge-queue bot pushed a commit that referenced this pull request Apr 23, 2024
@orpuente-MS @minestarks
Parse manifest in Rust instead of Python (#1330)
28817e4 
This PR regains some rust real-state. It delegates the parsing of the
manifest in `pip/qsharp/_qsharp.py/init()` to rust, avoiding duplicate
manifest-parsing logic in the project.

This change is needed to unblock #1313

---------

Co-authored-by: Mine Starks <16928427+minestarks@users.noreply.github.com>
@github-actions GitHub Actions
Copy link

Benchmark for 5f13265

Click to view benchmark
Test Base PR %
Array append evaluation 341.1±4.15µs 343.6±12.47µs +0.73%
Array literal evaluation 199.0±1.08µs 201.6±5.51µs +1.31%
Array update evaluation 425.7±2.52µs 425.4±6.54µs -0.07%
Core + Standard library compilation 16.6±0.15ms 17.0±0.40ms +2.41%
Deutsch-Jozsa evaluation 5.0±0.14ms 5.0±0.26ms 0.00%
Large file parity evaluation 33.7±0.17ms 33.7±0.80ms 0.00%
Large input file compilation 11.5±0.17ms 11.5±0.52ms 0.00%
Large input file compilation (interpreter) 44.8±1.37ms 44.8±1.53ms 0.00%
Large nested iteration 33.4±1.07ms 33.4±1.53ms 0.00%
Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample 1502.4±48.01µs 1495.4±27.90µs -0.47%
Perform Runtime Capabilities Analysis (RCA) on large file sample 7.8±0.14ms 7.8±0.11ms 0.00%
Perform Runtime Capabilities Analysis (RCA) on teleport sample 1415.7±31.15µs 1417.3±54.46µs +0.11%
Perform Runtime Capabilities Analysis (RCA) on the core and std libraries 21.3±0.26ms 21.2±0.99ms -0.47%
Teleport evaluation 80.5±4.18µs 80.6±4.05µs +0.12%

@orpuente-MS orpuente-MS added this pull request to the merge queue Apr 23, 2024
Merged via the queue into main with commit 438d937 Apr 23, 2024
17 checks passed
@orpuente-MS orpuente-MS deleted the oscarpuente/linting-for-notebooks branch April 23, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minestarks minestarks minestarks approved these changes

@billti billti Awaiting requested review from billti billti is a code owner

@idavis idavis Awaiting requested review from idavis idavis is a code owner

@ScottCarda-MS ScottCarda-MS Awaiting requested review from ScottCarda-MS ScottCarda-MS is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add linting support for notebooks after Linter is merged.
2 participants
@orpuente-MS @minestarks