-
Notifications
You must be signed in to change notification settings - Fork 17
[Perf] Streams 3: Add qd.stream_parallel() context manager #409
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
Changes from all commits
a40ed4c
aa2fa2a
be7ad92
ce83281
880abc7
065a3b7
cfc6f39
e9ce144
007b050
91ca883
8cd793c
e880d07
ad720bb
6351215
470912f
3b0ba29
1c62eae
e55c84f
216f7d5
49dc5af
d7836e3
74604f2
b83b65d
0c552cd
212aeb9
226c7c5
1f471b3
6919fee
7b4e2a4
88f1bf7
ca560b6
158c8fb
388a797
df0b03a
acff351
70eb471
ebd5e11
a6c3852
04e18ba
3af5bc8
55b71fb
dbb055c
af4a306
c50d034
824cabf
24bc67d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,7 +119,11 @@ def build_AnnAssign(ctx: ASTTransformerFuncContext, node: ast.AnnAssign): | |
|
|
||
| @staticmethod | ||
| def build_assign_annotated( | ||
| ctx: ASTTransformerFuncContext, target: ast.Name, value, is_static_assign: bool, annotation: Type | ||
| ctx: ASTTransformerFuncContext, | ||
| target: ast.Name, | ||
| value, | ||
| is_static_assign: bool, | ||
| annotation: Type, | ||
| ): | ||
| """Build an annotated assignment like this: target: annotation = value. | ||
|
|
||
|
|
@@ -165,7 +169,10 @@ def build_Assign(ctx: ASTTransformerFuncContext, node: ast.Assign) -> None: | |
|
|
||
| @staticmethod | ||
| def build_assign_unpack( | ||
| ctx: ASTTransformerFuncContext, node_target: list | ast.Tuple, values, is_static_assign: bool | ||
| ctx: ASTTransformerFuncContext, | ||
| node_target: list | ast.Tuple, | ||
| values, | ||
| is_static_assign: bool, | ||
| ): | ||
| """Build the unpack assignments like this: (target1, target2) = (value1, value2). | ||
| The function should be called only if the node target is a tuple. | ||
|
|
@@ -591,7 +598,8 @@ def build_Return(ctx: ASTTransformerFuncContext, node: ast.Return) -> None: | |
| else: | ||
| raise QuadrantsSyntaxError("The return type is not supported now!") | ||
| ctx.ast_builder.create_kernel_exprgroup_return( | ||
| expr.make_expr_group(return_exprs), _qd_core.DebugInfo(ctx.get_pos_info(node)) | ||
| expr.make_expr_group(return_exprs), | ||
| _qd_core.DebugInfo(ctx.get_pos_info(node)), | ||
| ) | ||
| else: | ||
| ctx.return_data = node.value.ptr | ||
|
|
@@ -1520,6 +1528,24 @@ def build_Continue(ctx: ASTTransformerFuncContext, node: ast.Continue) -> None: | |
| ctx.ast_builder.insert_continue_stmt(_qd_core.DebugInfo(ctx.get_pos_info(node))) | ||
| return None | ||
|
|
||
| @staticmethod | ||
| def build_With(ctx: ASTTransformerFuncContext, node: ast.With) -> None: | ||
| if len(node.items) != 1: | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels only supports a single context manager") | ||
| item = node.items[0] | ||
| if item.optional_vars is not None: | ||
| raise QuadrantsSyntaxError("'with ... as ...' is not supported in Quadrants kernels") | ||
| if not isinstance(item.context_expr, ast.Call): | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels requires a call expression") | ||
| if not FunctionDefTransformer._is_stream_parallel_with(node, ctx.global_vars): | ||
| raise QuadrantsSyntaxError("'with' in Quadrants kernels only supports qd.stream_parallel()") | ||
|
Comment on lines
+1538
to
+1541
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 qd.stream_parallel() context manager accepts but silently drops user-supplied args/kwargs. Extended reasoning...What the bug is
Meanwhile @contextmanager
def stream_parallel():
yieldSo the only legal call shape is Step-by-step proof@qd.kernel
def k():
with qd.stream_parallel(123, num_streams=4): # silently accepted
for i in range(N):
a[i] = 1.0
with qd.stream_parallel(): # also accepted
for j in range(N):
b[j] = 2.0
Why nothing else catches itThe contextmanager body never executes at Python runtime (the AST transformer rewrites the call before ImpactUser-facing UX papercut. A user who reasonably guesses at an API like FixOne-line addition in if item.context_expr.args or item.context_expr.keywords:
raise QuadrantsSyntaxError("qd.stream_parallel() takes no arguments")Three independent verifiers reviewed this and confirmed it is correct, real, and a real (if minor) UX gap; all three suggested nit severity given there is no correctness or performance impact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still applies — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still applies — |
||
| if not ctx.is_kernel: | ||
| raise QuadrantsSyntaxError("qd.stream_parallel() can only be used inside @qd.kernel, not @qd.func") | ||
| ctx.ast_builder.begin_stream_parallel() | ||
| build_stmts(ctx, node.body) | ||
|
Comment on lines
+1531
to
+1545
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Extended reasoning...What the bug is
Result: a Step-by-step proof@qd.kernel
def k(cond: qd.i32):
if cond:
with qd.stream_parallel():
for i in range(N):
a[i] = 1.0
for j in range(N):
b[j] = 2.0
The two for-loops execute serially on the default stream — exactly the behavior Same trigger on other containersIdentical silent-serialization manifests for any non-root container:
Why nothing else catches it
The validator's stated contract ('all top-level statements must be Severity rationaleMarking normal. The validator was specifically introduced in this PR to give clear compile-time errors for misuse patterns that would otherwise be silently confusing. A new user who reasonably writes FixTwo reasonable options, both one helper function: (a) Have (b) Strengthen Either brings the runtime behavior into line with the validator's documented contract. |
||
| ctx.ast_builder.end_stream_parallel() | ||
|
Comment on lines
+1544
to
+1546
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Non-for statements (e.g. Extended reasoning...Bug\n\n
|
||
| return None | ||
|
|
||
| @staticmethod | ||
| def build_Pass(ctx: ASTTransformerFuncContext, node: ast.Pass) -> None: | ||
| return None | ||
|
|
||
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.
Same. Not clear what
itemsis.