-
Notifications
You must be signed in to change notification settings - Fork 133
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
Fix tutorial codegen #1925
base: main
Are you sure you want to change the base?
Fix tutorial codegen #1925
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.
Thank you! LGTM!
246d066
to
3f18387
Compare
tests/tutorials/tutorials_test.py
Outdated
ep = ExecutePreprocessor(timeout=600) | ||
try: | ||
assert ep.preprocess(nb) is not None, f"Got empty notebook for {notebook}" | ||
except Exception: |
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.
Please raise that exception. Otherwise we cannot know what went wrong. It will just raise an assertion error.
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 slow turnaround, I was on vacation.
I cleaned up the error handling.
dc3ebac
to
1b4d8e1
Compare
The tutorial
codegen.ipynb
has a small bug, namely theControlFlowRegion
is not in the correct namespace (as used in the tutorial).This PR fixes the issue by importing
ControlFlowRegion
in thedace
namespace.Additionally I added a test for the notebook tutorials to make sure they are not broken.
At the moment I added only the "getting started" and "codegen" notebooks.
These tests require new dependencies
ipykernel
andnbconvert
.The above may or may not be the preferred solutions!