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

Fix tutorial codegen #1925

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

castigli
Copy link

@castigli castigli commented Feb 4, 2025

The tutorial codegen.ipynb has a small bug, namely the ControlFlowRegion is not in the correct namespace (as used in the tutorial).
This PR fixes the issue by importing ControlFlowRegion in the dace 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 and nbconvert.

The above may or may not be the preferred solutions!

Copy link
Contributor

@acalotoiu acalotoiu left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM!

@tbennun tbennun self-requested a review February 5, 2025 22:25
dace/__init__.py Outdated Show resolved Hide resolved
@castigli castigli force-pushed the fix-tutorial-codegen branch from 246d066 to 3f18387 Compare February 7, 2025 10:06
@castigli castigli requested a review from tbennun February 7, 2025 15:00
ep = ExecutePreprocessor(timeout=600)
try:
assert ep.preprocess(nb) is not None, f"Got empty notebook for {notebook}"
except Exception:
Copy link
Collaborator

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.

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.

3 participants