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.

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

Ipynb input #874

Merged
merged 17 commits into from
Nov 22, 2023
Merged

Ipynb input #874

merged 17 commits into from
Nov 22, 2023

Conversation

nhirschey
Copy link
Collaborator

Fixes #806. This code should be independent of everything else; only gets triggered on .ipynb input files. Could merge this into the modern theme branch for alphas or wait and merge into main after modern theme is complete.

The general strategy is to convert a notebook to markdown and then pass that markdown through FSharp.Formatting's existing markdown tools.

Parsing: This uses a custom parser to read notebooks as json. I tried using .NET Interactive's parser, but it requires mapping their undocumented internal C# classes to F# and at the end of the day it was easier to do the parsing from scratch.

Evaluation: relies on dotnet-repl, a personal project of someone on the .NET Interactive team. As recently as August they recommended it for commandline execution, so let's use it. The downside is that users must install this extra tool, but I don't think it's a big burden.

One thing @kMutagene, we have to decide what happens when we evaluate a notebook. What I did was have fsdocs --eval trigger dotnet-repl on the actual source notebook in the docs folder. This means that evaluating docs will change the content of the source notebooks (for example dotnet-repl puts some additional metadata in cells indicating time of execution). This seemed reasonable for now.

Testing: I added tests, but one thing I couldn't test well was dotnet-repl evaluation. I couldn't get the test project to evaluate the notebooks using dotnet-repl. Maybe test projects cannot call external dotnet tools? I don't know. I left the test in there but it's commented out.

@nhirschey nhirschey requested review from nojaf and kMutagene November 17, 2023 21:20
Copy link
Collaborator

@nojaf nojaf 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 a great work! I have some nitpicks and some places I would code a little more defensive. But overall this is solid!

src/FSharp.Formatting.Literate/ParsePynb.fs Outdated Show resolved Hide resolved
src/FSharp.Formatting.Literate/ParsePynb.fs Outdated Show resolved Hide resolved
src/FSharp.Formatting.Literate/ParsePynb.fs Outdated Show resolved Hide resolved
src/FSharp.Formatting.Literate/ParsePynb.fs Outdated Show resolved Hide resolved
src/FSharp.Formatting.Literate/ParsePynb.fs Outdated Show resolved Hide resolved
src/fsdocs-tool/BuildCommand.fs Outdated Show resolved Hide resolved
src/fsdocs-tool/BuildCommand.fs Show resolved Hide resolved
@@ -225,3 +254,38 @@ let ``Parses frontmatter correctly `` () =
twoTowersHtml |> shouldContainText "<a href=\"fellowship.html\">Previous</a>"
twoTowersHtml |> shouldContainText "<a href=\"return.html\">Next</a>"
returnHtml |> shouldContainText "<a href=\"two-tower.html\">Previous</a>"


(* Cannot get this test to evaluate the notebook
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not? What goes wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have evaluate = true set for DocContent, which should trigger the external call to dotnet repl that runs the notebook and produces outputs for the cell . But for some reason the code calling out to dotnet repl isn't getting triggered inside this test. dotnet fsdocs --eval triggers notebook evaluation in a docs folder, but I have not been able to get it to work inside this test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure? This test is working for me in WSL.

@nojaf nojaf mentioned this pull request Nov 20, 2023
Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Great work!
Feel free to add another alpha release entry in RELEASE_NOTES.md.
Once merged to main this will publish another alpha.

@nhirschey
Copy link
Collaborator Author

Sounds good, I'll bump the alpha release when I merge.

Thanks for the code review and all your work on the modern theme. You've done incredible work the past month!

@nhirschey
Copy link
Collaborator Author

@nojaf, just to double check that I don't mess up any of your in-progress work. Are you ok with this being merged into main now? I'm done and I've updated release notes to bump the release

@nojaf
Copy link
Collaborator

nojaf commented Nov 22, 2023

You've done incredible work the past month!

Why thank you 😊

Are you ok with this being merged into main now?

Yes, go for it!

@nhirschey nhirschey merged commit acdf616 into fsprojects:main Nov 22, 2023
3 checks passed
@jonsequitur
Copy link

Parsing: This uses a custom parser to read notebooks as json. I tried using .NET Interactive's parser, but it requires mapping their undocumented internal C# classes to F# and at the end of the day it was easier to do the parsing from scratch.

If there are changes you'd suggest making to Microsoft.DotNet.Interactive.Documents`, please let us know. Ideally you'd be able to use this effectively from F# without needing to duplicate the effort.

@jonsequitur
Copy link

Evaluation: relies on dotnet-repl, a personal project of someone on the .NET Interactive team. As recently as August they recommended it for commandline execution, so let's use it. The downside is that users must install this extra tool, but I don't think it's a big burden.

I'm the owner of dotnet-repl. It's intended as a prototype for command line functionality that could find its way into the core of .NET Interactive. I'm glad you're finding it useful. I'm also more than happy to work with people on contributions to it.

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.

Using polyglot notebooks (dotnet interactive) as input
3 participants