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

documentation: allow marimo notebooks as documentation #2787

Merged
merged 11 commits into from
Jun 27, 2024

Conversation

superlopuh
Copy link
Member

I like these a lot for local development, and have struggled for a while with getting them to fail properly when the xDSL code updates. This PR introduces both the a sample notebook, but also a new test script that just runs all the notebooks in the marimo folder in headless mode, failing on error. This should give us plenty of coverage and confidence in the notebooks working properly.

@superlopuh superlopuh added the documentation Improvements or additions to documentation label Jun 26, 2024
@superlopuh superlopuh self-assigned this Jun 26, 2024
@@ -26,6 +26,7 @@ dev = [
"nbval<0.12",
"filecheck<0.0.25",
"lit<19.0.0",
"marimo==0.6.10",
Copy link
Member Author

Choose a reason for hiding this comment

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

note that this is a new dev dependency

Comment on lines 26 to 30
@app.cell
def __():
raise Exception("a")
return

Copy link
Member Author

Choose a reason for hiding this comment

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

test code to make sure the CI fails as expected

Copy link
Member Author

Choose a reason for hiding this comment

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

It did fail as expected

@superlopuh
Copy link
Member Author

OK it failed in the way I expected!

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.79%. Comparing base (17ec8ba) to head (9148f4f).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2787   +/-   ##
=======================================
  Coverage   89.78%   89.79%           
=======================================
  Files         382      384    +2     
  Lines       48412    48442   +30     
  Branches     7427     7429    +2     
=======================================
+ Hits        43469    43499   +30     
  Misses       3780     3780           
  Partials     1163     1163           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

I think it's fine to have some marimo files in some projects (like the riscv project for instance), and having way to test them in the CI.

But otherwise, I feel like having both jupyter notebooks and marimo in our documentation just make things more complex for users to see the documentation no?
I'm just saying this because it seems you are writing a book using marimo, and I'm not sure this is the right approach. If this is just a test file, then I would rename it accordingly though, but would not keep it in docs

@superlopuh
Copy link
Member Author

I would propose migrating all the jupyter notebooks. I actually started with Toy, but since that imports the toy module by being in the same folder, that one might be a little difficult. I'd say that all the other notebooks I would rather migrate to the marimo folder.

I'm not sure exactly what you're proposing for this PR, though, would you like me to replace the placeholder notebook with one with more interesting content?

@superlopuh
Copy link
Member Author

My attempt at swapping the notebook for a more interesting one was not successful. I have a small collection of notebooks locally that all require some modifications to the code that I've not merged yet, so I don't actually have a more compelling notebook to add immediately. I'd still like to merge the infrastructure to pave the way for these notebooks being added in the future, after I merge all the dependent changes.

Copy link
Contributor

@tobiasgrosser tobiasgrosser left a comment

Choose a reason for hiding this comment

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

I find this is a perfect patch. LGTM.

@@ -51,8 +51,15 @@ pytest-toy:

tests-toy: filecheck-toy pytest-toy

tests-marimo:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be marked PHONY?

Copy link
Member Author

Choose a reason for hiding this comment

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

defo

@superlopuh
Copy link
Member Author

I discussed this with Mathieu, and we came to an agreement that I would replace the Atrributes placeholder notebook as soon as I find something else.

@superlopuh superlopuh merged commit a96634b into main Jun 27, 2024
10 checks passed
@superlopuh superlopuh deleted the sasha/marimo/init branch June 27, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants