-
Notifications
You must be signed in to change notification settings - Fork 545
refactor: Move examples for notebook viewer and callbacks to examples/python
#11416
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
refactor: Move examples for notebook viewer and callbacks to examples/python
#11416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the import in notebook_viewer
to be from rerun.notebook
instead of directly from our notebooks package? Otherwise LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the double review. Looking at the CI failure made me realize there were a few more issues but this should be it.
b2acb22
to
9df19b1
Compare
### Related #11416 I requested the notebooks get moved so our automated examples test would make sure the notebooks aren't broken. Unfortunately we don't test any of our notebooks so most were broken. A little bit related to broader notebook testing improvement https://linear.app/rerun/issue/RR-1880/test-our-jupyter-notebooks-on-ci ### What I highly recommend reviewing with rich diff for notebooks enabled. * Adds nbqa so we can run mypy on the notebooks to at least ensure the apis align * I couldn't get the config working so unfortunately I just have the directories to search listed in the pixi command * Adds nbstripout but doesn't yet tie it to our py-fmt. Jupyter notebook outputs are just binary so that will bloat the repo and complicate diffs. Unfortunately nbstripout doesn't have a nice config setup so I just put together a simple wrapper. * I made two actual code changes not just in tests since it made sense to resolve notebook mypy errors 1. I made component_type a class method. We basically have component type raw strings everywhere probably because it wasn't easy before to grab them from the class definition rather than instance 2. If we have a recording string the api should actually be a little different than asking globally. I assume we'll always find the id so we universally return a string. This still doesn't execute our notebooks which is probably a worthwhile followup issue.
### Related #11416 I requested the notebooks get moved so our automated examples test would make sure the notebooks aren't broken. Unfortunately we don't test any of our notebooks so most were broken. A little bit related to broader notebook testing improvement https://linear.app/rerun/issue/RR-1880/test-our-jupyter-notebooks-on-ci ### What I highly recommend reviewing with rich diff for notebooks enabled. * Adds nbqa so we can run mypy on the notebooks to at least ensure the apis align * I couldn't get the config working so unfortunately I just have the directories to search listed in the pixi command * Adds nbstripout but doesn't yet tie it to our py-fmt. Jupyter notebook outputs are just binary so that will bloat the repo and complicate diffs. Unfortunately nbstripout doesn't have a nice config setup so I just put together a simple wrapper. * I made two actual code changes not just in tests since it made sense to resolve notebook mypy errors 1. I made component_type a class method. We basically have component type raw strings everywhere probably because it wasn't easy before to grab them from the class definition rather than instance 2. If we have a recording string the api should actually be a little different than asking globally. I assume we'll always find the id so we universally return a string. This still doesn't execute our notebooks which is probably a worthwhile followup issue.
Related
Closes #10711
What